| 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
| ||
|
Message-ID: <CAJD7tkZMzHXWDPsAhd+JTm24LfKG+MT+kOm167EjzuYjCiBq5A@mail.gmail.com>
Date: Mon, 2 Dec 2024 21:34:13 -0800
From: Yosry Ahmed <yosryahmed@...gle.com>
To: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.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>,
"Feghali, Wajdi K" <wajdi.k.feghali@...el.com>, "Gopal, Vinodh" <vinodh.gopal@...el.com>
Subject: Re: [PATCH v1 1/2] mm: zswap: Modified zswap_store_page() to process
multiple pages in a folio.
On Mon, Dec 2, 2024 at 5:13 PM Sridhar, Kanchana P
<kanchana.p.sridhar@...el.com> wrote:
>
>
> > -----Original Message-----
> > From: Yosry Ahmed <yosryahmed@...gle.com>
> > Sent: Monday, December 2, 2024 11:34 AM
> > 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; Feghali, Wajdi K <wajdi.k.feghali@...el.com>;
> > Gopal, Vinodh <vinodh.gopal@...el.com>
> > Subject: Re: [PATCH v1 1/2] mm: zswap: Modified zswap_store_page() to
> > process multiple pages in a folio.
> >
> > On Wed, Nov 27, 2024 at 2:53 PM Kanchana P Sridhar
> > <kanchana.p.sridhar@...el.com> wrote:
> > >
> > > Modified zswap_store() to store the folio in batches of
> > > SWAP_CRYPTO_BATCH_SIZE pages. Accordingly, refactored
> > zswap_store_page()
> > > into zswap_store_pages() that processes a range of pages in the folio.
> > > zswap_store_pages() is a vectorized version of zswap_store_page().
> > >
> > > For now, zswap_store_pages() will sequentially compress these pages with
> > > zswap_compress().
> > >
> > > These changes are follow-up to code review comments received for [1], and
> > > are intended to set up zswap_store() for batching with Intel IAA.
> > >
> > > [1]: https://patchwork.kernel.org/project/linux-
> > mm/patch/20241123070127.332773-11-kanchana.p.sridhar@...el.com/
> > >
> > > Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@...el.com>
> > > ---
> > > include/linux/zswap.h | 1 +
> > > mm/zswap.c | 154 ++++++++++++++++++++++++------------------
> > > 2 files changed, 88 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> > > index d961ead91bf1..05a81e750744 100644
> > > --- a/include/linux/zswap.h
> > > +++ b/include/linux/zswap.h
> > > @@ -7,6 +7,7 @@
> > >
> > > struct lruvec;
> > >
> > > +#define SWAP_CRYPTO_BATCH_SIZE 8UL
> > > extern atomic_long_t zswap_stored_pages;
> > >
> > > #ifdef CONFIG_ZSWAP
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index f6316b66fb23..b09d1023e775 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -1409,78 +1409,96 @@ 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)
> > > +/*
> > > + * Store multiple pages in @folio, starting from the page at index @si up to
> > > + * and including the page at index @ei.
> > > + */
> > > +static ssize_t zswap_store_pages(struct folio *folio,
> > > + long si,
> > > + long ei,
> > > + struct obj_cgroup *objcg,
> > > + struct zswap_pool *pool)
> > > {
> > > - swp_entry_t page_swpentry = page_swap_entry(page);
> > > + struct page *page;
> > > + swp_entry_t page_swpentry;
> > > struct zswap_entry *entry, *old;
> > > + size_t compressed_bytes = 0;
> > > + u8 nr_pages = ei - si + 1;
> > > + u8 i;
> > > +
> > > + for (i = 0; i < nr_pages; ++i) {
> > > + page = folio_page(folio, si + i);
> > > + page_swpentry = page_swap_entry(page);
> > > +
> > > + /* allocate entry */
> > > + entry = zswap_entry_cache_alloc(GFP_KERNEL,
> > page_to_nid(page));
> > > + if (!entry) {
> > > + zswap_reject_kmemcache_fail++;
> > > + return -EINVAL;
> > > + }
> >
> > I think this patch is wrong on its own, for example if an allocation
> > fails in the above loop we exit without cleaning up previous
> > allocations. I think it's fixed in patch 2 but we cannot introduce
>
> I think there might be a misunderstanding. zswap_store_pages() will
> clean up local resources allocated during an iteration of the for loop,
> upon an error in that iteration. If you see the "goto store_failed" and
> "goto compress_failed" this would explain what I mean. If an allocation
> fails for an iteration, we simply return -EINVAL, and zswap_store()
> will goto the "put_pool" label with "ret" still false, which will delete
> all zswap entries for the folio (allocated up until the error iteration in
> zswap_store_pages(); or potentially already in the xarray).
>
> Hence, there is no bug and each of the two patches are correct by
> themselves AFAICT, but please let me know if I am missing anything. Thanks!
Uh yes, the cleanup is in zswap_store().
>
> > bugs in-between patches. I think the helpers in patch 2 don't really
> > help as I mentioned. Please combine the changes and keep them in the
> > main series (unless you have a reason not to).
>
> Sure. As noted in my earlier response to comments received for patch 2,
> I can either inline all iterations or create helpers for all iterations over
> the pages in a batch. Appreciate your suggestions on which would be a
> better approach.
I think leaving them open-coded will be clearer for now. We can
revisit the code organization later if it gets out of hand.
Powered by blists - more mailing lists