linux-kernel - Re: [External] Re: [PATCH] mm: zswap: fix the lack of page lru flag in zswap_writeback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 11 Jan 2024 10:57:00 +0800
From: Zhongkun He <hezhongkun.hzk@...edance.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Nhat Pham <nphamcs@...il.com>, akpm@...ux-foundation.org, hannes@...xchg.org, 
	sjenning@...hat.com, ddstreet@...e.org, vitaly.wool@...sulko.com, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Chris Li <chrisl@...nel.org>, weijie.yang@...sung.com
Subject: Re: [External] Re: [PATCH] mm: zswap: fix the lack of page lru flag
 in zswap_writeback_entry

On Wed, Jan 10, 2024 at 12:30 AM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> On Mon, Jan 8, 2024 at 7:13 PM Zhongkun He <hezhongkun.hzk@...edance.com> wrote:
> >
> > Hi Yosry, glad to hear from you and happy new year!
> >
> > > Sorry for being late to the party. It seems to me that all of this
> > > hassle can be avoided if lru_add_fn() did the right thing in this case
> > > and added the folio to the tail of the lru directly. I am no expert in
> > > how the page flags work here, but it seems like we can do something
> > > like this in lru_add_fn():
> > >
> > > if (folio_test_reclaim(folio))
> > >     lruvec_add_folio_tail(lruvec, folio);
> > > else
> > >     lruvec_add_folio(lruvec, folio);
> > >
> > > I think the main problem with this is that PG_reclaim is an alias to
> > > PG_readahead, so readahead pages will also go to the tail of the lru,
> > > which is probably not good.
> >
> > Agree with you， I will try it.
>
> +Matthew Wilcox
>
> I think we need to figure out if it's okay to do this first, because
> it will affect pages with PG_readahead as well.

Yes, I've tested it and there is one more thing
that needs to be modified.

+       if (folio_test_reclaim(folio))
+               lruvec_add_folio_tail(lruvec, folio);
+       else
+               lruvec_add_folio(lruvec, folio);

@@ -1583,10 +1583,8 @@ void folio_end_writeback(struct folio *folio)
         * a gain to justify taking an atomic operation penalty at the
         * end of every folio writeback.
         */
-       if (folio_test_reclaim(folio)) {
+       if (folio_test_reclaim(folio) && folio_rotate_reclaimable(folio))
                folio_clear_reclaim(folio);
-               folio_rotate_reclaimable(folio);
-       }

-void folio_rotate_reclaimable(struct folio *folio)
+bool folio_rotate_reclaimable(struct folio *folio)
 {
        if (success）
+               return true;
        }
+       return false;
 }

>
> >
> > >
> > > A more intrusive alternative is to introduce a folio_lru_add_tail()
> > > variant that always adds pages to the tail, and optionally call that
> > > from __read_swap_cache_async() instead of folio_lru_add() based on a
> > > new boolean argument. The zswap code can set that boolean argument
> > > during writeback to make sure newly allocated folios are always added
> > > to the tail of the lru.
> >
> > I have the same idea and also find it intrusive. I think the first solution
> > is very good and I will try it. If it works, I will send the next version.
>
> One way to avoid introducing folio_lru_add_tail() and blumping a
> boolean from zswap is to have a per-task context (similar to
> memalloc_nofs_save()), that makes folio_add_lru() automatically add
> folios to the tail of the LRU. I am not sure if this is an acceptable
> approach though in terms of per-task flags and such.

I got it.