Message-ID: <SJ0PR11MB56786A69A03DD1E5780D3347C9122@SJ0PR11MB5678.namprd11.prod.outlook.com>
Date: Wed, 8 Jan 2025 03:57:24 +0000
From: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>, "hannes@...xchg.org"
	<hannes@...xchg.org>, "nphamcs@...il.com" <nphamcs@...il.com>,
	"chengming.zhou@...ux.dev" <chengming.zhou@...ux.dev>,
	"usamaarif642@...il.com" <usamaarif642@...il.com>, "ryan.roberts@....com"
	<ryan.roberts@....com>, "21cnbao@...il.com" <21cnbao@...il.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
	"davem@...emloft.net" <davem@...emloft.net>, "clabbe@...libre.com"
	<clabbe@...libre.com>, "ardb@...nel.org" <ardb@...nel.org>,
	"ebiggers@...gle.com" <ebiggers@...gle.com>, "surenb@...gle.com"
	<surenb@...gle.com>, "Accardi, Kristen C" <kristen.c.accardi@...el.com>,
	"Feghali, Wajdi K" <wajdi.k.feghali@...el.com>, "Gopal, Vinodh"
	<vinodh.gopal@...el.com>, "Sridhar, Kanchana P"
	<kanchana.p.sridhar@...el.com>
Subject: RE: [PATCH v5 11/12] mm: zswap: Restructure & simplify zswap_store()
 to make it amenable for batching.


> -----Original Message-----
> From: Yosry Ahmed <yosryahmed@...gle.com>
> Sent: Monday, January 6, 2025 5:17 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> Cc: linux-kernel@...r.kernel.org; linux-mm@...ck.org;
> hannes@...xchg.org; nphamcs@...il.com; chengming.zhou@...ux.dev;
> usamaarif642@...il.com; ryan.roberts@....com; 21cnbao@...il.com;
> akpm@...ux-foundation.org; linux-crypto@...r.kernel.org;
> herbert@...dor.apana.org.au; davem@...emloft.net;
> clabbe@...libre.com; ardb@...nel.org; ebiggers@...gle.com;
> surenb@...gle.com; Accardi, Kristen C <kristen.c.accardi@...el.com>;
> Feghali, Wajdi K <wajdi.k.feghali@...el.com>; Gopal, Vinodh
> <vinodh.gopal@...el.com>
> Subject: Re: [PATCH v5 11/12] mm: zswap: Restructure & simplify
> zswap_store() to make it amenable for batching.
> 
> On Fri, Dec 20, 2024 at 10:31 PM Kanchana P Sridhar
> <kanchana.p.sridhar@...el.com> wrote:
> >
> > This patch introduces zswap_store_folio() that implements all the computes
> > done earlier in zswap_store_page() for a single-page, for all the pages in
> > a folio. This allows us to move the loop over the folio's pages from
> > zswap_store() to zswap_store_folio().
> >
> > A distinct zswap_compress_folio() is also added, that simply calls
> > zswap_compress() for each page in the folio it is called with.
> 
> The git diff looks funky, it may make things clearer to introduce
> zswap_compress_folio() in a separate patch.

Ok, will do so.

> 
> >
> > zswap_store_folio() starts by allocating all zswap entries required to
> > store the folio. Next, it calls zswap_compress_folio() and finally, adds
> > the entries to the xarray and LRU.
> >
> > The error handling and cleanup required for all failure scenarios that can
> > occur while storing a folio in zswap is now consolidated to a
> > "store_folio_failed" label in zswap_store_folio().
> >
> > These changes facilitate developing support for compress batching in
> > zswap_store_folio().
> >
> > Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@...el.com>
> > ---
> >  mm/zswap.c | 183 +++++++++++++++++++++++++++++++++-----------------
> ---
> >  1 file changed, 116 insertions(+), 67 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 99cd78891fd0..1be0f1807bfc 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1467,77 +1467,129 @@ static void shrink_worker(struct work_struct
> *w)
> >  * main API
> >  **********************************/
> >
> > -static ssize_t zswap_store_page(struct page *page,
> > -                               struct obj_cgroup *objcg,
> > -                               struct zswap_pool *pool)
> > +static bool zswap_compress_folio(struct folio *folio,
> > +                                struct zswap_entry *entries[],
> > +                                struct zswap_pool *pool)
> >  {
> > -       swp_entry_t page_swpentry = page_swap_entry(page);
> > -       struct zswap_entry *entry, *old;
> > +       long index, nr_pages = folio_nr_pages(folio);
> >
> > -       /* allocate entry */
> > -       entry = zswap_entry_cache_alloc(GFP_KERNEL, page_to_nid(page));
> > -       if (!entry) {
> > -               zswap_reject_kmemcache_fail++;
> > -               return -EINVAL;
> > +       for (index = 0; index < nr_pages; ++index) {
> > +               struct page *page = folio_page(folio, index);
> > +
> > +               if (!zswap_compress(page, entries[index], pool))
> > +                       return false;
> >         }
> >
> > -       if (!zswap_compress(page, entry, pool))
> > -               goto compress_failed;
> > +       return true;
> > +}
> >
> > -       old = xa_store(swap_zswap_tree(page_swpentry),
> > -                      swp_offset(page_swpentry),
> > -                      entry, GFP_KERNEL);
> > -       if (xa_is_err(old)) {
> > -               int err = xa_err(old);
> > +/*
> > + * Store all pages in a folio.
> > + *
> > + * The error handling from all failure points is consolidated to the
> > + * "store_folio_failed" label, based on the initialization of the zswap
> entries'
> > + * handles to ERR_PTR(-EINVAL) at allocation time, and the fact that the
> > + * entry's handle is subsequently modified only upon a successful
> zpool_malloc()
> > + * after the page is compressed.
> > + */
> > +static ssize_t zswap_store_folio(struct folio *folio,
> > +                                struct obj_cgroup *objcg,
> > +                                struct zswap_pool *pool)
> > +{
> > +       long index, nr_pages = folio_nr_pages(folio);
> > +       struct zswap_entry **entries = NULL;
> > +       int node_id = folio_nid(folio);
> > +       size_t compressed_bytes = 0;
> >
> > -               WARN_ONCE(err != -ENOMEM, "unexpected xarray error: %d\n",
> err);
> > -               zswap_reject_alloc_fail++;
> > -               goto store_failed;
> > +       entries = kmalloc(nr_pages * sizeof(*entries), GFP_KERNEL);
> 
> We can probably use kcalloc() here.

I am a little worried about the latency penalty of kcalloc() in the reclaim path,
especially since I am not relying on zero-initialized memory for "entries"..

> 
> > +       if (!entries)
> > +               return -ENOMEM;
> > +
> > +       /* allocate entries */
> 
> This comment can be dropped.

Sure.

> 
> > +       for (index = 0; index < nr_pages; ++index) {
> > +               entries[index] = zswap_entry_cache_alloc(GFP_KERNEL,
> node_id);
> > +
> > +               if (!entries[index]) {
> > +                       zswap_reject_kmemcache_fail++;
> > +                       nr_pages = index;
> > +                       goto store_folio_failed;
> > +               }
> > +
> > +               entries[index]->handle = (unsigned long)ERR_PTR(-EINVAL);
> >         }
> >
> > -       /*
> > -        * We may have had an existing entry that became stale when
> > -        * the folio was redirtied and now the new version is being
> > -        * swapped out. Get rid of the old.
> > -        */
> > -       if (old)
> > -               zswap_entry_free(old);
> > +       if (!zswap_compress_folio(folio, entries, pool))
> > +               goto store_folio_failed;
> >
> > -       /*
> > -        * The entry is successfully compressed and stored in the tree, there is
> > -        * no further possibility of failure. Grab refs to the pool and objcg.
> > -        * These refs will be dropped by zswap_entry_free() when the entry is
> > -        * removed from the tree.
> > -        */
> > -       zswap_pool_get(pool);
> > -       if (objcg)
> > -               obj_cgroup_get(objcg);
> > +       for (index = 0; index < nr_pages; ++index) {
> > +               swp_entry_t page_swpentry = page_swap_entry(folio_page(folio,
> index));
> > +               struct zswap_entry *old, *entry = entries[index];
> > +
> > +               old = xa_store(swap_zswap_tree(page_swpentry),
> > +                              swp_offset(page_swpentry),
> > +                              entry, GFP_KERNEL);
> > +               if (xa_is_err(old)) {
> > +                       int err = xa_err(old);
> > +
> > +                       WARN_ONCE(err != -ENOMEM, "unexpected xarray error:
> %d\n", err);
> > +                       zswap_reject_alloc_fail++;
> > +                       goto store_folio_failed;
> > +               }
> >
> > -       /*
> > -        * We finish initializing the entry while it's already in xarray.
> > -        * This is safe because:
> > -        *
> > -        * 1. Concurrent stores and invalidations are excluded by folio lock.
> > -        *
> > -        * 2. Writeback is excluded by the entry not being on the LRU yet.
> > -        *    The publishing order matters to prevent writeback from seeing
> > -        *    an incoherent entry.
> > -        */
> > -       entry->pool = pool;
> > -       entry->swpentry = page_swpentry;
> > -       entry->objcg = objcg;
> > -       entry->referenced = true;
> > -       if (entry->length) {
> > -               INIT_LIST_HEAD(&entry->lru);
> > -               zswap_lru_add(&zswap_list_lru, entry);
> > +               /*
> > +                * We may have had an existing entry that became stale when
> > +                * the folio was redirtied and now the new version is being
> > +                * swapped out. Get rid of the old.
> > +                */
> > +               if (old)
> > +                       zswap_entry_free(old);
> > +
> > +               /*
> > +                * The entry is successfully compressed and stored in the tree,
> there is
> > +                * no further possibility of failure. Grab refs to the pool and objcg.
> > +                * These refs will be dropped by zswap_entry_free() when the
> entry is
> > +                * removed from the tree.
> > +                */
> > +               zswap_pool_get(pool);
> > +               if (objcg)
> > +                       obj_cgroup_get(objcg);
> > +
> > +               /*
> > +                * We finish initializing the entry while it's already in xarray.
> > +                * This is safe because:
> > +                *
> > +                * 1. Concurrent stores and invalidations are excluded by folio
> lock.
> > +                *
> > +                * 2. Writeback is excluded by the entry not being on the LRU yet.
> > +                *    The publishing order matters to prevent writeback from seeing
> > +                *    an incoherent entry.
> > +                */
> > +               entry->pool = pool;
> > +               entry->swpentry = page_swpentry;
> > +               entry->objcg = objcg;
> > +               entry->referenced = true;
> > +               if (entry->length) {
> > +                       INIT_LIST_HEAD(&entry->lru);
> > +                       zswap_lru_add(&zswap_list_lru, entry);
> > +               }
> > +
> > +               compressed_bytes += entry->length;
> >         }
> >
> > -       return entry->length;
> > +       kfree(entries);
> > +
> > +       return compressed_bytes;
> > +
> > +store_folio_failed:
> > +       for (index = 0; index < nr_pages; ++index) {
> > +               if (!IS_ERR_VALUE(entries[index]->handle))
> > +                       zpool_free(pool->zpool, entries[index]->handle);
> > +
> > +               zswap_entry_cache_free(entries[index]);
> > +       }
> 
> If there is a failure in xa_store() halfway through the entries, this
> loop will free all the compressed objects and entries. But, some of
> the entries are already in the xarray, and zswap_store() will try to
> free them again. This seems like a bug, or did I miss something here?

Thanks, great catch! Yes, this is a bug. I have a simple fix implemented,
that I am currently testing and will include in v6.

> 
> > +
> > +       kfree(entries);
> >
> > -store_failed:
> > -       zpool_free(pool->zpool, entry->handle);
> > -compress_failed:
> > -       zswap_entry_cache_free(entry);
> >         return -EINVAL;
> >  }
> >
> > @@ -1549,8 +1601,8 @@ bool zswap_store(struct folio *folio)
> >         struct mem_cgroup *memcg = NULL;
> >         struct zswap_pool *pool;
> >         size_t compressed_bytes = 0;
> > +       ssize_t bytes;
> >         bool ret = false;
> > -       long index;
> >
> >         VM_WARN_ON_ONCE(!folio_test_locked(folio));
> >         VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
> > @@ -1584,15 +1636,11 @@ bool zswap_store(struct folio *folio)
> >                 mem_cgroup_put(memcg);
> >         }
> >
> > -       for (index = 0; index < nr_pages; ++index) {
> > -               struct page *page = folio_page(folio, index);
> > -               ssize_t bytes;
> > +       bytes = zswap_store_folio(folio, objcg, pool);
> > +       if (bytes < 0)
> > +               goto put_pool;
> >
> > -               bytes = zswap_store_page(page, objcg, pool);
> > -               if (bytes < 0)
> > -                       goto put_pool;
> > -               compressed_bytes += bytes;
> > -       }
> > +       compressed_bytes = bytes;
> 
> What's the point of having both compressed_bytes and bytes now?

The main reason was to cleanly handle a negative error value returned in "bytes"
(declared as ssize_t), as against a true total "compressed_bytes" (declared as size_t)
for the folio to use for objcg charging. This is similar to the current mainline
code where zswap_store() calls zswap_store_page(). I was hoping to avoid potential
issues with overflow/underflow, and for maintainability. Let me know if this is Ok.

Thanks,
Kanchana

> 
> >
> >         if (objcg) {
> >                 obj_cgroup_charge_zswap(objcg, compressed_bytes);
> > @@ -1622,6 +1670,7 @@ bool zswap_store(struct folio *folio)
> >                 pgoff_t offset = swp_offset(swp);
> >                 struct zswap_entry *entry;
> >                 struct xarray *tree;
> > +               long index;
> >
> >                 for (index = 0; index < nr_pages; ++index) {
> >                         tree = swap_zswap_tree(swp_entry(type, offset + index));
> > --
> > 2.27.0
> >

Powered by blists - more mailing lists