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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAF8kJuM71XEeYdDOdKkz_c+JNnMhxfPAGDoZ-BqpcYuyHxDFmg@mail.gmail.com>
Date: Tue, 19 Mar 2024 17:20:25 -0700
From: Chris Li <chriscli@...gle.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: Yosry Ahmed <yosryahmed@...gle.com>, 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? ^_^
>
> 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.

Ack.

Chris