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]

Message-ID: <20240316133302.GB372017@cmpxchg.org>
Date: Sat, 16 Mar 2024 09:33:02 -0400
From: Johannes Weiner <hannes@...xchg.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Chris Li <chrisl@...nel.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 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? ^_^

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.

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.

So in the next version, please just remove this now stale one-liner.