> -----Original Message----- > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > Sent: Monday, December 2, 2024 9:34 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; ryan.roberts@xxxxxxx; 21cnbao@xxxxxxxxx; > akpm@xxxxxxxxxxxxxxxxxxxx; Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; > Gopal, Vinodh <vinodh.gopal@xxxxxxxxx> > 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@xxxxxxxxx> wrote: > > > > > > > -----Original Message----- > > > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > > Sent: Monday, December 2, 2024 11:34 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; ryan.roberts@xxxxxxx; 21cnbao@xxxxxxxxx; > > > akpm@xxxxxxxxxxxxxxxxxxxx; Feghali, Wajdi K > <wajdi.k.feghali@xxxxxxxxx>; > > > Gopal, Vinodh <vinodh.gopal@xxxxxxxxx> > > > 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@xxxxxxxxx> 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@xxxxxxxxx/ > > > > > > > > Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@xxxxxxxxx> > > > > --- > > > > 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. Sounds good! Thanks, Kanchana
