Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231113130601.3350915-1-hezhongkun.hzk@bytedance.com>
 <CAJD7tkY8SwROmNEaBAhkS4OKj33g-6fHsKFeYKW3afT+yAbvxA@mail.gmail.com>
 <CAF8kJuPonfuOtipdifXwBny2H7cy6m6BL8mWFVXzfb9JSdYq3Q@mail.gmail.com>
 <CAJD7tkYMiJiXTTgAN34TP8QTr-ViAuEFddYes=ac+1ErenjCZw@mail.gmail.com>
 <CAF8kJuOC30feLGs0bNHOxMjSZ3uqF1y7eUdJ4p-w-myP8c1cFg@mail.gmail.com>
 <CACSyD1NJ4ycr74pXHNus3ZjA=LvZP+aMNyz=iWyHuDODpx7aww@mail.gmail.com>
 <CAF8kJuPymUX+DwouwgH6og0BO6ZYheGXsk=GYqYuMjKMz-Xqbw@mail.gmail.com>
 <CACSyD1P-6mQ7n+ghsZQ0Gp4AxyTTi6=UZHz2DnXWuCWke2fkPw@mail.gmail.com> <CAF8kJuOR+POJx4wT7jbZkoiwGjrjVH4uuJ5GgoWBOf20JUf9qw@mail.gmail.com>
In-Reply-To: <CAF8kJuOR+POJx4wT7jbZkoiwGjrjVH4uuJ5GgoWBOf20JUf9qw@mail.gmail.com>
From:   Zhongkun He <hezhongkun.hzk@bytedance.com>
Date:   Mon, 20 Nov 2023 11:16:11 +0800
Message-ID: <CACSyD1Omro1h2w6bSP0_gB21u=KwFDu+DHjN4+K82H5Pv7yMig@mail.gmail.com>
Subject: Re: [External] Re: [PATCH] mm:zswap: fix zswap entry reclamation
 failure in two scenarios
To:     Chris Li <chrisl@kernel.org>
Cc:     Yosry Ahmed <yosryahmed@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Nhat Pham <nphamcs@gmail.com>,
        Seth Jennings <sjenning@redhat.com>,
        Dan Streetman <ddstreet@ieee.org>,
        Vitaly Wool <vitaly.wool@konsulko.com>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>, Ying <ying.huang@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

>
> Thanks for the clarification. Keep in mind that memory freeing from
> and zswap entry and zpool does not directly translate into page free.
> If the page has other none freed zswap entry or zsmalloc usage, those
> pages will not be free to the system. That is the fragmentation cost I
> was talking about.

Yes, it may need to be compacted.

>
> With this consideration, do you know many extra pages it can release
> back to the system by this patch in your usage case? If the difference
> is very small, it might not be worth the extra complexity to release
> those.
>

The original intention of this patch is to make shrink work properly=EF=BC=
=8C
not to release cache and related memory.

> > The original intention of this patch is to solve the problem that
> > shrink_work() fails to reclaim memory in two situations.
> >
> > For case (1),  the zswap_writeback_entry() will failed for the
> > __read_swap_cache_async return NULL because the swap has been
> > freed but cached in swap_slots_cache, so the memory come from
> > the zswap entry struct and compressed page.
> In those cases, if we drop the swap_slots_cache, it will also free
> those zswap entries and compressed pages (zpool), right?
>
> > Count =3D SWAP_BATCH * ncpu.
>
> That is the upper limit. Not all CPUs have swap batches fully loaded.

Yes.

>
> > Solution: move the zswap_invalidate() out of batches, free it once the =
swap
> > count equal to 0.
> Per previous discussion, this will have an impact on the
> swap_slot_cache behavior.
> We need some data points for cost benefit analysis.
>
> > For case (2),  the zswap_writeback_entry() will failed for !page_was_al=
located
> > because zswap_load will have two copies of the same page in memory
> >   (compressed and uncompressed) after faulting in a page from zswap whe=
n
> > zswap_exclusive_loads disabled. The amount of memory is greater but dep=
ends
> > on the usage.
>
> That is basically disable the future swap out page IO write
> optimization that skip the write if the page hasn't changed. If the
> system are low on memory, that is justifiable. Again, it seems we can
> have a pass to drop the compressed memory if the swap count is zero
> (and mark page dirty).
>

OK.

> >
> > Why do we need  to release them?
> > Consider this scenario,there is a lot of data cached in memory and zswa=
p,
> > hit the limit=EF=BC=8Cand shrink_worker will fail. The new coming data =
will be written
>
> Yes, the shrink_worker will need to allocate a page to store
> uncompressed data for write back.
>
> > directly to swap due to zswap_store failure. Should we free the last on=
e
> > to store the latest one in zswap.
>
> The "last one" you mean the most recent zswap entry written into zswap?
> Well, you need to allocate a page to write it out, that is an async proce=
ss.
> Shrink the zpool now is kind of too late already.
>

The last zswap_entry in zswap_pool->lru.

> > According to the previous discussion, the writeback is inevitable.
> > So I want to make zswap_exclusive_loads_enabled the default behavior
> > or make it the only way to do zswap loads. It only makes sense when
>
> We need some data point for how often we swap it out to zswap again,
> where the zswap out write can be saved by using the existing compressed d=
ata.
>
> It is totally possible this page IO write out optimization is not
> worthwhile for zswap.
> We need some data to support that claim.
>

Got it. I will find it.

> > the page is read and no longer dirty. If the page is read frequently, i=
t
> > should stay in cache rather than zswap. The benefit of doing this is
> > very small, i.e. two copies of the same page in memory.
>
> If the benefit of doing this is very small, that seems to be the
> argument against this patch?
> Again we need some data points for cost and benefit analysis.
>

Yes, it is the new idea to make zswap_exclusive_loads_enabled the
default behavior or make it the only way to do zswap loads.

> Chris