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] [thread-next>] [day] [month] [year] [list]

Date: Sat, 16 Mar 2024 23:12:13 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Johannes Weiner <hannes@...xchg.org>, Chris Li <chrisl@...nel.org>
Cc: 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 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!