RE: [PATCH v9 6/7] mm: zswap: Support large folios in zswap_store().

[Sridhar, Kanchana P]

> -----Original Message-----
> From: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx>
> Sent: Tuesday, October 1, 2024 10:09 AM
> To: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> hannes@xxxxxxxxxxx; nphamcs@xxxxxxxxx; chengming.zhou@xxxxxxxxx;
> usamaarif642@xxxxxxxxx; shakeel.butt@xxxxxxxxx; ryan.roberts@xxxxxxx;
> Huang, Ying <ying.huang@xxxxxxxxx>; 21cnbao@xxxxxxxxx; akpm@linux-
> foundation.org; willy@xxxxxxxxxxxxx; Zou, Nanhai <nanhai.zou@xxxxxxxxx>;
> Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh
> <vinodh.gopal@xxxxxxxxx>; Sridhar, Kanchana P
> <kanchana.p.sridhar@xxxxxxxxx>
> Subject: RE: [PATCH v9 6/7] mm: zswap: Support large folios in zswap_store().
> 
> > -----Original Message-----
> > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> > Sent: Tuesday, October 1, 2024 10:01 AM
> > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> > hannes@xxxxxxxxxxx; nphamcs@xxxxxxxxx; chengming.zhou@xxxxxxxxx;
> > usamaarif642@xxxxxxxxx; shakeel.butt@xxxxxxxxx;
> ryan.roberts@xxxxxxx;
> > Huang, Ying <ying.huang@xxxxxxxxx>; 21cnbao@xxxxxxxxx; akpm@linux-
> > foundation.org; willy@xxxxxxxxxxxxx; Zou, Nanhai <nanhai.zou@xxxxxxxxx>;
> > Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh
> > <vinodh.gopal@xxxxxxxxx>
> > Subject: Re: [PATCH v9 6/7] mm: zswap: Support large folios in
> zswap_store().
> >
> > On Tue, Oct 1, 2024 at 9:58 AM Sridhar, Kanchana P
> > <kanchana.p.sridhar@xxxxxxxxx> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> > > > Sent: Monday, September 30, 2024 11:00 PM
> > > > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx>
> > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> > > > hannes@xxxxxxxxxxx; nphamcs@xxxxxxxxx;
> > chengming.zhou@xxxxxxxxx;
> > > > usamaarif642@xxxxxxxxx; shakeel.butt@xxxxxxxxx;
> > ryan.roberts@xxxxxxx;
> > > > Huang, Ying <ying.huang@xxxxxxxxx>; 21cnbao@xxxxxxxxx;
> akpm@linux-
> > > > foundation.org; willy@xxxxxxxxxxxxx; Zou, Nanhai
> > <nanhai.zou@xxxxxxxxx>;
> > > > Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh
> > > > <vinodh.gopal@xxxxxxxxx>
> > > > Subject: Re: [PATCH v9 6/7] mm: zswap: Support large folios in
> > zswap_store().
> > > >
> > > > [..]
> > > > > > >  store_failed:
> > > > > > >         zpool_free(entry->pool->zpool, entry->handle);
> > > > > > > -put_pool:
> > > > > > > -       zswap_pool_put(entry->pool);
> > > > > > > -freepage:
> > > > > > > +put_pool_objcg:
> > > > > > > +       zswap_pool_put(pool);
> > > > > > > +       obj_cgroup_put(objcg);
> > > > > >
> > > > > > I think if we reorder the function we can drop these calls, make the
> > > > > > comments positioned a bit better, and centralize the entry
> > > > > > initializations. I am also not a fan of passing a semi-initialized
> > > > > > entry to zswap_compress() to get the pool pointer.
> > > > > >
> > > > > > Does the following diff improve things or did I miss something?
> > > > >
> > > > > We shouldn’t be adding the entry to the xarray before initializing its
> pool
> > > > > and objcg, right? Please let me know if I am misunderstanding what
> > you're
> > > > > proposing in the diff.
> > > >
> > > > It should be safe. We already initialize entry->lru after we insert
> > > > the entry in the tree. See the comment above the call to
> > > > zswap_lru_add(). Basically we are protected against concurrent
> > > > stores/loads through the folio lock, and are protected against
> > > > writeback because the entry is not on the LRU yet.
> > >
> > > Thanks for the clarification, Yosry. Since this is a change in the entry
> > > initialization wrt the mainline, is it Ok if this is done in a follow-up patch?
> >
> > Sure. We can discuss it separately. Do you want me to send a patch or
> > do you intend to?
> 
> Thanks Yosry! I will send the patch separately.

Hi Yosry,

I am preparing the follow-up patch so I can submit it once this patch-series is
merged to mm-unstable (since these changes have dependencies on my
existing patch).

Is my understanding correct that the folio lock also protects against swapoff
happening in between addition of the entry to the xarray and initializing its
members, which will need to be valid for
swapoff --> ... -> free_swap_slot() --> zswap_invalidate() ? Would appreciate
it if you can confirm.

Thanks,
Kanchana