Message-ID: <SJ2PR11MB8472CE2B46F08804CF5F2158C9CAA@SJ2PR11MB8472.namprd11.prod.outlook.com>
Date: Fri, 14 Nov 2025 19:59:57 +0000
From: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
CC: SeongJae Park <sj@...nel.org>, "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>,
	"ying.huang@...ux.alibaba.com" <ying.huang@...ux.alibaba.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"senozhatsky@...omium.org" <senozhatsky@...omium.org>, "kasong@...cent.com"
	<kasong@...cent.com>, "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>, "Gomes, Vinicius" <vinicius.gomes@...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 v13 22/22] mm: zswap: Batched zswap_compress() with
 compress batching of large folios.


> -----Original Message-----
> From: Yosry Ahmed <yosry.ahmed@...ux.dev>
> Sent: Friday, November 14, 2025 11:44 AM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> Cc: SeongJae Park <sj@...nel.org>; 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;
> ying.huang@...ux.alibaba.com; akpm@...ux-foundation.org;
> senozhatsky@...omium.org; kasong@...cent.com; 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>; Gomes, Vinicius <vinicius.gomes@...el.com>;
> Feghali, Wajdi K <wajdi.k.feghali@...el.com>; Gopal, Vinodh
> <vinodh.gopal@...el.com>
> Subject: Re: [PATCH v13 22/22] mm: zswap: Batched zswap_compress() with
> compress batching of large folios.
> 
> On Fri, Nov 14, 2025 at 07:23:42PM +0000, Sridhar, Kanchana P wrote:
> [..]
>  > > > > >
> > > > > > > > +		if (err && !wb_enabled)
> > > > > > > > +			goto compress_error;
> > > > > > > > +
> > > > > > > > +		for_each_sg(acomp_ctx->sg_outputs->sgl, sg,
> > > nr_comps, k) {
> > > > > > > > +			j = k + i;
> > > > > > >
> [..]
> > > > > >
> > > > > > >
> > > > > > > > +			dst = acomp_ctx->buffers[k];
> > > > > > > > +			dlen = sg->length | *errp;
> > > > > > >
> > > > > > > Why are we doing this?
> > > > > > >
> > > > > > > > +
> > > > > > > > +			if (dlen < 0) {
> > > > > > >
> > > > > > > We should do the incompressible page handling also if dlen is
> > > PAGE_SIZE,
> > > > > > > or if the compression failed (I guess that's the intention of bit
> OR'ing
> > > > > > > with *errp?)
> > > > > >
> > > > > > Yes, indeed: that's the intention of bit OR'ing with *errp.
> > > > >
> > > > > ..and you never really answered my question. In the exising code we
> > > > > store the page as incompressible if writeback is enabled AND
> > > > > crypto_wait_req() fails or dlen is zero or PAGE_SIZE. We check above
> > > > > if crypto_wait_req() fails and writeback is disabled, but what about the
> > > > > rest?
> > > >
> > > > Let me explain this some more. The new code only relies on the
> assumption
> > > > that if dlen is zero or >= PAGE_SIZE, the compressor will not return a 0
> > > > ("successful status"). In other words, the compressor will return an error
> > > status
> > > > in this case, which is expected to be a negative error code.
> > >
> > > I am not sure if all compressors do that, especially for the case where
> > > dlen >= PAGE_SIZE. The existing code does not assume that there will be
> > > an error code in these cases.
> > >
> > > For the dlen == 0 case, the check was introduced recently by commit
> > > dca4437a5861 ("mm/zswap: store <PAGE_SIZE compression failed page
> > > as-is"). Looking through the history it seems like it was introduced in
> > > v4 of that patch but I don't see the reasoning.
> >
> > The existing code did not check for dlen == 0 and dlen >= PAGE_SIZE
> > prior to commit dca4437a5861 ("mm/zswap: store <PAGE_SIZE compression
> > failed page as-is") either. We need SeongJae or Herbert to clarify whether
> > this check is needed, or if it is sufficient to rely on comp_ret, the return from
> > crypto_wait_req().
> >
> > >
> > > SeongJae, did you observe any compressors returning dlen == 0 but no
> > > error code? What was the reasoning behind the dlen == 0 check?
> > >
> > > >
> > > > Under these (hopefully valid) assumptions, the code handles the simple
> case
> > > > of an error compression return status and writeback is disabled, by the
> > > > "goto compress_error".
> > > >
> > > > The rest is handled by these:
> > > >
> > > > 1) First, I need to adapt the use of sg_outputs->sgl->length to represent
> the
> > > > compress length for software compressors, so I do this after
> > > crypto_wait_req()
> > > > returns:
> > > >
> > > >                 acomp_ctx->sg_outputs->sgl->length = acomp_ctx->req->dlen;
> > >
> > > For SW compressors, why is acomp_ctx->sg_outputs->sgl->length not set?
> > > IIUC we are using the same API for SW and HW compressors, why is the
> > > output length in different places for each of them?
> >
> > This is to first implement the SG lists batching interface in iaa_crypto, while
> > maintaining backward compatibility for SW compressors with the new API.
> > I believe we may want to adapt the crypto API to SW compressors
> > at a later point. I also believe this would be outside the scope of this patch.
> > It would be nice if Herbert can share his vision on this aspect.
> >
> > >
> > > >
> > > > I did not want to propose any changes to crypto software compressors
> > > protocols.
> > > >
> > > > 2) After the check for the "if (err && !wb_enabled)" case, the new code
> has
> > > this:
> > > >
> > > >                 for_each_sg(acomp_ctx->sg_outputs->sgl, sg, nr_comps, k) {
> > > >                         j = k + i;
> > > >                         dst = acomp_ctx->buffers[k];
> > > >                         dlen = sg->length | *errp;
> > > >
> > > >                         if (dlen < 0) {
> > > >                                 dlen = PAGE_SIZE;
> > > >                                 dst = kmap_local_page(folio_page(folio, start + j));
> > > >                         }
> > > >
> > > > For batching compressors, namely, iaa_crypto, the individual output SG
> > > > lists sg->length follows the requirements from Herbert: each sg->length
> > > > is the compressed length or the error status (a negative error code).
> > > >
> > > > Then all I need to know whether to store the page as incompressible
> > > > is to either directly test if sg->length is negative (for batching
> compressors),
> > > > or sg->length bit-OR'ed with the crypto_wait_req() return status ("err")
> > > > is negative. This is accomplished by the "dlen = sg->length | *errp;".
> > > >
> > > > I believe this maintains backward compatibility with the existing code.
> > > > Please let me know if you agree.
> > >
> > > For batching compressors, will 'err' be set as well, or just sg->length?
> > > If it's just sg->length, then we need to check again if WB is enabled
> > > here before storing the page uncompressed. Right?
> >
> > iaa_crypto will set 'err' and set the sg->length as per the batching interface
> > spec from Herbert.
> 
> So both 'err' and sg->length will contain the same error? In this case
> why do we need to check if dlen < 0? Shouldn't checking 'err' be
> sufficient? and it would work for both SW and HW and we wouldn't need
> errp. Did I miss something?

Great question. For a batching compressor, 'err' will contain an error if any
page in the batch had a compression error. This allows the early bail-out
path for SW and HW compressors if writeback is not enabled for the folio.

Only the specific pages' sg->length will contain an error code. The other
batch pages that compressed fine will have the compressed length in
sg->length. This enables the post-compression loop with the errp check
bit-ORed with the sg->length, which for SW, has been brought up to date
with the acomp_req->dlen before we get to the wb_enabled code path.

> 
> >
> > >
> > > >
> > > > >
> > > > > We don't check again if writeback is enabled before storing the page is
> > > > > incompressible, and we do not check if dlen is zero or PAGE_SIZE. Are
> > > > > these cases no longer possible?
> > > >
> > > > Hope the above explanation clarifies things some more? These case
> > > > are possible, and as long as they return an error status, they should be
> > > > correctly handled by the new code.
> > >
> > > As mentioned above, I am not sure if that's correct for dlen >=
> > > PAGE_SIZE.
> >
> > We need to get clarity on this from SeongJae/Herbert.
> >
> > >
> > > >
> > > > >
> > > > > Also, why use errp, why not explicitly use the appropriate error code?
> > > > > It's also unclear to me why the error code is always zero with HW
> > > > > compression?
> > > >
> > > > This is because of the sg->length requirements (compressed
> length/error)
> > > > for the batching interface suggested by Herbert. Hence, I upfront define
> > > > err_sg to 0, and, set errp to &err_sg for batching compressors. For
> software
> > > > compressors, errp is set to &err, namely, the above check will always
> apply
> > > > the software compressor's error status to the compressed length via
> > > > the bit-OR to determine if the page needs to be stored uncompressed.
> > >
> > > Thanks for the clarification. I understand that the error code has
> > > different sources for SW and HW compressors, but I do not like using
> > > errp as an indirection. It makes the code unclear. I would rather we
> > > explicitly check err for SW compressors and dlen for HW compressors.
> > >
> > > That being said, I thought what Herbert suggested was that the same API
> > > is used for both SW and HW compressors. IOW, either way we submit a
> > > batch of pages (8 pages for SW compressors), and then the crypto API
> > > would either give the entire batch to the compressor if it supports
> > > batching, or loop over them internally and hand them page-by-page to
> > > the compressor.
> >
> > That was not how I understood Herbert's suggestion for the batching
> interface.
> > He did suggest the following:
> >
> > "Before the call to acomp, the destination SG list should contain as
> > many elements as the number of units.  On return, the dst lengths
> > should be stored in each destination SG entry."
> >
> > I have incorporated this suggestion in the iaa_crypto driver. For SW
> > compressors, I have tried not to propose any API changes, while making
> > sure that the zswap changes for the SG lists batching API work as expected
> > for SW without too much special-casing code.
> >
> > I suppose I always assumed that we would update SW compressors later,
> > and not as part of this patch-set.
> 
> I am not sure I understand what changes lie in the crypto layer and what
> changes lie in the SW compressors. I am not suggesting we do any
> modification to the SW compressors.
> 
> I imagined that the crypto layer would present a uniform API regardless
> of whether or not the compressor supports batching. Ideally zswap would
> pass in a batch to crypto and it would figure out if it needs to break
> them down or not. Then the output length and errors would be presented
> uniformly to the caller.

>From my understanding, this would require changes to the crypto layer for
SW compressors, which again IIUC, does not set the sg->length, only sets
the acomp_req->dlen (IIUC, a temporary state until crypto for SW also uses
SG lists).

Ideally, batching could be handled similarly by crypto for SW. I believe we
will get there, albeit outside the scope of this patch.

> 
> That being said, I am not at all familiar with how crypto works and how
> straightforward that would be. Herbert, WDYT?
> 
> >
> > >
> > > This would simplify usage as we do not have to handle the differences in
> > > zswap.
> >
> > That's the nice thing about SG lists - I think the zswap_compress() calls to
> > the new batching API appears agnostic to SW and HW compressors.
> > Other than the upfront "errp = (pool->compr_batch_size == 1) ? &err :
> &err_sg;"
> > the logical code organization of the new zswap_compress() is quite similar to
> > the existing code. The post-compress "dlen = sg->length | *errp;" handles
> the rest.
> 
> It would be even nicer if the batches are also abstracted by SG lists.
> 
> Also, I don't like how the error codes and output lengths are presented
> differently for HW and SW compressors.

I do believe this is short-term and is the first step in implementing batching
in zswap. We should get Herbert's thoughts on this.

Powered by blists - more mailing lists