linux-kernel - Re: [PATCH v6] zswap: replace RB tree with xarray

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Tue, 19 Mar 2024 17:26:43 -0700
From: Chris Li <chrisl@...nel.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Johannes Weiner <hannes@...xchg.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	Nhat Pham <nphamcs@...il.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>, 
	Chengming Zhou <zhouchengming@...edance.com>, Barry Song <v-songbaohua@...o.com>, 
	Barry Song <baohua@...nel.org>, Chengming Zhou <chengming.zhou@...ux.dev>
Subject: Re: [PATCH v6] zswap: replace RB tree with xarray

On Sat, Mar 16, 2024 at 11:12 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> On Sat, Mar 16, 2024 at 6:33 AM Johannes Weiner <hannes@...xchg.org> wrote:
> >
> > On Fri, Mar 15, 2024 at 06:30:37PM -0700, Yosry Ahmed wrote:
> > > [..]
> > > >
> > > > @@ -1555,28 +1473,35 @@ bool zswap_store(struct folio *folio)
> > > >  insert_entry:
> > > >         entry->swpentry = swp;
> > > >         entry->objcg = objcg;
> > > > -       if (objcg) {
> > > > -               obj_cgroup_charge_zswap(objcg, entry->length);
> > > > -               /* Account before objcg ref is moved to tree */
> > >
> > >
> > > I do not understand this comment, but it seems to care about the
> > > charging happening before the entry is added to the tree. This patch
> > > will move it after the tree insertion.
> > >
> > > Johannes, do you mind elaborating what this comment is referring to?
> > > It should be clarified, updated, or removed as part of this movement.
> >
> > Wait, I wrote that? ^_^
>
> Well, past Johannes did :)
>
> >
> > The thinking was this: the objcg reference acquired in this context is
> > passed on to the tree. Once the entry is in the tree and the
> > tree->lock released, the entry is public and the current context
> > doesn't have its own pin on objcg anymore. Ergo, objcg is no longer
> > safe to access from this context.
> >
> > This is a conservative take, though, considering the wider context:
> > the swapcache itself, through folio lock, prevents invalidation; and
> > reclaim/writeback cannot happen before the entry is on the LRU.
>
> Actually, I think just the folio being locked in the swapcache is
> enough protection. Even if the entry is added to the LRU, the
> writeback code will find it in the swapcache and abort.
>
> >
> > After Chris's patch, the tree is no longer a serialization point for
> > stores. The swapcache and the LRU are. I had asked Chris upthread to
> > add an explicit comment about that. I think once he does that, the
> > objcg situation should be self-evident as well.
>
> Perhaps it should be clarified that the swapcache on its own is enough
> protection against both invalidation and reclaim/writeback, and the
> entry not being on the LRU is *additional* protection on top of that
> against reclaim/writeback. Right?
>
> >
> > So in the next version, please just remove this now stale one-liner.
>
> Thanks for confirming. Chris, please remove this comment and update
> the comment Johannes asked you to add as mentioned above. Thanks!

Will do.

Chris