Received-SPF: pass (google.com: domain of linux-kernel+bounces-17904-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231024142706.195517-1-hezhongkun.hzk@bytedance.com>
 <CAKEwX=OiNB+pPhb-3Tf7O=F7psKE3EOpwmbPSeLSOyuHpj3i+Q@mail.gmail.com>
 <CACSyD1P6HmH9tSvONnNxYv8P+am_hH2dK3UJQd9_+o6EWkPsXA@mail.gmail.com>
 <CAKEwX=PC3C-PrWAH3XiYGyR4ujqBJQBBX6uRa2jXKCy9VMyRCQ@mail.gmail.com>
 <CACSyD1O7t0+BXUujJ81RAdEys3MUnmpu0sRADLazoyvayx5DLA@mail.gmail.com> <CAKEwX=P5AC+ubnunnZr5vMiC6fFU+E_E7jg_FZztWwZRYSxTWQ@mail.gmail.com>
In-Reply-To: <CAKEwX=P5AC+ubnunnZr5vMiC6fFU+E_E7jg_FZztWwZRYSxTWQ@mail.gmail.com>
From: Zhongkun He <hezhongkun.hzk@bytedance.com>
Date: Fri, 5 Jan 2024 22:10:06 +0800
Message-ID: <CACSyD1Nnc_w3epbt6+EMt7a-4pAzgW1hbE=G5Fy5Tc5R5+uxKw@mail.gmail.com>
Subject: Re: [External] Re: [PATCH] mm: zswap: fix the lack of page lru flag
 in zswap_writeback_entry
To: Nhat Pham <nphamcs@gmail.com>
Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, yosryahmed@google.com, 
	sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com, 
	linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	Chris Li <chrisl@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

>
> Hmm originally I was thinking of doing an (unconditional)
> lru_add_drain() outside of zswap_writeback_entry() - once in
> shrink_worker() and/or zswap_shrinker_scan(), before we write back any
> of the entries. Not sure if it would work/help here tho - haven't
> tested that idea yet.
>

The pages are allocated by __read_swap_cache_async() in
 zswap_writeback_entry() and it must be newly allocated=EF=BC=8C
not cached in swap.
Please see the code below in zswap_writeback_entry()

page =3D __read_swap_cache_async(swpentry, GFP_KERNEL, mpol,
                NO_INTERLEAVE_INDEX, &page_was_allocated);
    if (!page) {
        goto fail;}
    /* Found an existing page, we raced with load/swapin */
    if (!page_was_allocated) {
        put_page(page);
        ret =3D -EEXIST;
        goto fail;
    }

So when it comes to SetPageReclaim(page),
The page has just been allocated and is still in the percpu batch,
which has not been added to the LRU.

Therefore=EF=BC=8Clru_add_drain() did not work outside the
zswap_writeback_entry(=EF=BC=89

> >
> > New test:
> > This patch will add the execution of folio_rotate_reclaimable(not execu=
ted
> > without this patch) and lru_add_drain,including percpu lock competition=
.
> > I bind a new task to allocate memory and use the same batch lock to com=
pete
> > with the target process, on the same CPU.
> > context:
> > 1:stress --vm 1 --vm-bytes 1g    (bind to cpu0)
> > 2:stress --vm 1 --vm-bytes 5g --vm-hang 0=EF=BC=88bind to cpu0=EF=BC=89
> > 3:reclaim pages, and writeback 5G zswap_entry in cpu0 and node 0.
> >
> > Average time of five tests
> >
> > Base      patch            patch + compete
> > 4.947      5.0676          5.1336
> >                 +2.4%          +3.7%
> > compete means: a new stress run in cpu0 to compete with the writeback p=
rocess.
> > PID USER        %CPU  %MEM     TIME+ COMMAND                         P
> >  1367 root         49.5      0.0       1:09.17 bash     =EF=BC=88writeb=
ack=EF=BC=89            0
> >  1737 root         49.5      2.2       0:27.46 stress      (use percpu
> > lock)    0
> >
> > around 2.4% increase in real time,including the execution of
> > folio_rotate_reclaimable(not executed without this patch) and lru_add_d=
rain,but
> > no lock contentions.
>
> Hmm looks like the regression is still there, no?

Yes, it cannot be eliminated.

>
> >
> > around 1.3% additional  increase in real time with lock contentions on =
the same
> > cpu.
> >
> > There is another option here, which is not to move the page to the
> > tail of the inactive
> > list after end_writeback and delete the following code in
> > zswap_writeback_entry(),
> > which did not work properly. But the pages will not be released first.
> >
> > /* move it to the tail of the inactive list after end_writeback */
> > SetPageReclaim(page);
>
> Or only SetPageReclaim on pages on LRU?

No, all the pages are newly allocted and not on LRU.

This patch should add lru_add_drain() directly, remove the
if statement.
The purpose of writing back data is to release the page,
so I think it is necessary to fix it.

Thanks for your time, Nhat.