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

[Chris Li]

On Sat, Mar 16, 2024 at 11:12 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Sat, Mar 16, 2024 at 6:33 AM Johannes Weiner <hannes@xxxxxxxxxxx> 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