Re: [PATCH v13 21/22] mm: zswap: zswap_store() will process a large folio in batches.

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Fri, Nov 07, 2025 at 02:28:23AM +0000, Sridhar, Kanchana P wrote:
> 
> > -----Original Message-----
> > From: Nhat Pham <nphamcs@gmail.com>
> > Sent: Thursday, November 6, 2025 9:46 AM
> > To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> > hannes@cmpxchg.org; yosry.ahmed@linux.dev; chengming.zhou@linux.dev;
> > usamaarif642@gmail.com; ryan.roberts@arm.com; 21cnbao@gmail.com;
> > ying.huang@linux.alibaba.com; akpm@linux-foundation.org;
> > senozhatsky@chromium.org; sj@kernel.org; kasong@tencent.com; linux-
> > crypto@vger.kernel.org; herbert@gondor.apana.org.au;
> > davem@davemloft.net; clabbe@baylibre.com; ardb@kernel.org;
> > ebiggers@google.com; surenb@google.com; Accardi, Kristen C
> > <kristen.c.accardi@intel.com>; Gomes, Vinicius <vinicius.gomes@intel.com>;
> > Feghali, Wajdi K <wajdi.k.feghali@intel.com>; Gopal, Vinodh
> > <vinodh.gopal@intel.com>
> > Subject: Re: [PATCH v13 21/22] mm: zswap: zswap_store() will process a
> > large folio in batches.
> > 
> > On Tue, Nov 4, 2025 at 1:12 AM Kanchana P Sridhar
> > <kanchana.p.sridhar@intel.com> wrote:
> > >
> > > This patch makes two major changes:
> > >
> > > First, we allocate pool batching resources if the compressor supports
> > > batching:
> > >
> > >   This patch sets up zswap for allocating per-CPU resources optimally
> > >   for non-batching and batching compressors.
> > >
> > >   A new ZSWAP_MAX_BATCH_SIZE constant is defined as 8U, to set an upper
> > >   limit on the number of pages in large folios that will be batch
> > >   compressed.
> > >
> > >   It is up to the compressor to manage multiple requests, as needed, to
> > >   accomplish batch parallelism. zswap only needs to allocate the per-CPU
> > >   dst buffers according to the batch size supported by the compressor.
> > >
> > >   A "u8 compr_batch_size" member is added to "struct zswap_pool", as per
> > >   Yosry's suggestion. pool->compr_batch_size is set as the minimum of
> > >   the compressor's max batch-size and ZSWAP_MAX_BATCH_SIZE.
> > Accordingly,
> > >   pool->compr_batch_size compression dst buffers are allocated in the
> > >   per-CPU acomp_ctx.
> > >
> > >   zswap does not use more than one dst buffer yet. Follow-up patches
> > >   will actually utilize the multiple acomp_ctx buffers for batch
> > >   compression/decompression of multiple pages.
> > >
> > >   Thus, ZSWAP_MAX_BATCH_SIZE limits the amount of extra memory used
> > for
> > >   batching. There is a small extra memory overhead of allocating
> > >   the acomp_ctx->buffers array for compressors that do not support
> > >   batching: On x86_64, the overhead is 1 pointer per-CPU (i.e. 8 bytes).
> > >
> > > Next, we store the folio in batches:
> > >
> > >   This patch modifies zswap_store() to store a batch of pages in large
> > >   folios at a time, instead of storing one page at a time. It does this by
> > >   calling a new procedure zswap_store_pages() with a range of indices in
> > >   the folio: for batching compressors, this range contains up to
> > >   pool->compr_batch_size pages. For non-batching compressors, we send up
> > >   to ZSWAP_MAX_BATCH_SIZE pages to be sequentially compressed and
> > stored
> > >   in zswap_store_pages().
> > >
> > >   zswap_store_pages() implements all the computes done earlier in
> > >   zswap_store_page() for a single-page, for multiple pages in a folio,
> > >   namely the "batch":
> > >
> > >   1) It starts by allocating all zswap entries required to store the
> > >      batch. New procedures, zswap_entries_cache_alloc_batch() and
> > >      zswap_entries_cache_free_batch() call kmem_cache_[free]alloc_bulk()
> > >      to optimize the performance of this step.
> > >
> > >   2) The entry doesn't have to be allocated on the same node as the page
> > >      being stored in zswap: we let the slab allocator decide this in
> > >      kmem_cache_alloc_bulk(). However, to make sure the current zswap
> > >      LRU list/shrinker behavior is preserved, we store the folio's nid as
> > >      a new @nid member in the entry to enable adding it to the correct
> > >      LRU list (and deleting it from the right LRU list). This ensures
> > >      that when the folio's allocating NUMA node is under memory
> > >      pressure, the entries corresponding to its pages are written back.
> > >
> > >      The memory footprint of struct zswap_entry remains unchanged at
> > >      56 bytes with the addition of the "int nid" member by condensing
> > >      "length" and "referenced" into 4 bytes using bit fields and using
> > >      the 4 bytes available after "referenced" for the "int nid". Thanks
> > >      to Nhat and Yosry for these suggestions!
> > >
> > >   3) Next, the entries fields are written, computes that need to be happen
> > >      anyway, without modifying the zswap xarray/LRU publishing order. This
> > >      avoids bringing the entries into the cache for writing in different
> > >      code blocks within this procedure, hence improves latency.
> > >
> > >   4) Next, it calls zswap_compress() to sequentially compress each page in
> > >      the batch.
> > >
> > >   5) Finally, it adds the batch's zswap entries to the xarray and LRU,
> > >      charges zswap memory and increments zswap stats.
> > >
> > >   6) The error handling and cleanup required for all failure scenarios
> > >      that can occur while storing a batch in zswap are consolidated to a
> > >      single "store_pages_failed" label in zswap_store_pages(). Here again,
> > >      we optimize performance by calling kmem_cache_free_bulk().
> > >
> > > This commit also makes a minor optimization in zswap_compress(), that
> > > takes a "bool wb_enabled" argument; computed once in zswap_store()
> > > rather than for each page in the folio.
> > >
> > > Suggested-by: Nhat Pham <nphamcs@gmail.com>
> > > Suggested-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> > > Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@intel.com>
> > > ---
> > >  mm/zswap.c | 336 ++++++++++++++++++++++++++++++++++++-------------
> > ----
> > >  1 file changed, 232 insertions(+), 104 deletions(-)
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index cb384eb7c815..257567edc587 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -82,6 +82,9 @@ static bool zswap_pool_reached_full;
> > >
> > >  #define ZSWAP_PARAM_UNSET ""
> > >
> > > +/* Limit the batch size to limit per-CPU memory usage for dst buffers. */
> > > +#define ZSWAP_MAX_BATCH_SIZE 8U
> > > +
> > >  static int zswap_setup(void);
> > >
> > >  /* Enable/disable zswap */
> > > @@ -139,7 +142,7 @@ struct crypto_acomp_ctx {
> > >         struct crypto_acomp *acomp;
> > >         struct acomp_req *req;
> > >         struct crypto_wait wait;
> > > -       u8 *buffer;
> > > +       u8 **buffers;
> > >         struct mutex mutex;
> > >         bool is_sleepable;
> > >  };
> > > @@ -149,6 +152,9 @@ struct crypto_acomp_ctx {
> > >   * The only case where lru_lock is not acquired while holding tree.lock is
> > >   * when a zswap_entry is taken off the lru for writeback, in that case it
> > >   * needs to be verified that it's still valid in the tree.
> > > + *
> > > + * @compr_batch_size: The max batch size of the compression algorithm,
> > > + *                    bounded by ZSWAP_MAX_BATCH_SIZE.
> > >   */
> > >  struct zswap_pool {
> > >         struct zs_pool *zs_pool;
> > > @@ -158,6 +164,7 @@ struct zswap_pool {
> > >         struct work_struct release_work;
> > >         struct hlist_node node;
> > >         char tfm_name[CRYPTO_MAX_ALG_NAME];
> > > +       u8 compr_batch_size;
> > >  };
> > >
> > >  /* Global LRU lists shared by all zswap pools. */
> > > @@ -182,6 +189,7 @@ static struct shrinker *zswap_shrinker;
> > >   *              writeback logic. The entry is only reclaimed by the writeback
> > >   *              logic if referenced is unset. See comments in the shrinker
> > >   *              section for context.
> > > + * nid - NUMA node id of the page for which this is the zswap entry.
> > >   * pool - the zswap_pool the entry's data is in
> > >   * handle - zsmalloc allocation handle that stores the compressed page data
> > >   * objcg - the obj_cgroup that the compressed memory is charged to
> > > @@ -189,8 +197,11 @@ static struct shrinker *zswap_shrinker;
> > >   */
> > >  struct zswap_entry {
> > >         swp_entry_t swpentry;
> > > -       unsigned int length;
> > > -       bool referenced;
> > > +       struct {
> > > +               unsigned int length:31;
> > > +               bool referenced:1;
> > > +       };
> > 
> > Maybe make these macro-defined constants?
> > 
> > Code mostly LGTM otherwise.
> 
> Thanks, Nhat! With respect to the suggestion to make the bit-fields
> as macro-defined constants, I was browsing through kernel headers
> that use bit-fields, and it appears the convention is to use integers
> rather than constants.

Yeah I think that's the common case, let's keep the numbers as-is.