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

[Johannes Weiner <hannes@cmpxchg.org>]

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.