RE: [PATCH v5 02/12] crypto: acomp - Define new interfaces for compress/decompress batching.

["Sridhar, Kanchana P" <kanchana.p.sridhar@intel.com>]

Hi Herbert,

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Saturday, December 28, 2024 3:46 AM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> hannes@cmpxchg.org; yosryahmed@google.com; nphamcs@gmail.com;
> chengming.zhou@linux.dev; usamaarif642@gmail.com;
> ryan.roberts@arm.com; 21cnbao@gmail.com; akpm@linux-foundation.org;
> linux-crypto@vger.kernel.org; davem@davemloft.net; clabbe@baylibre.com;
> ardb@kernel.org; ebiggers@google.com; surenb@google.com; Accardi,
> Kristen C <kristen.c.accardi@intel.com>; Feghali, Wajdi K
> <wajdi.k.feghali@intel.com>; Gopal, Vinodh <vinodh.gopal@intel.com>
> Subject: Re: [PATCH v5 02/12] crypto: acomp - Define new interfaces for
> compress/decompress batching.
> 
> On Fri, Dec 20, 2024 at 10:31:09PM -0800, Kanchana P Sridhar wrote:
> > This commit adds get_batch_size(), batch_compress() and
> batch_decompress()
> > interfaces to:
> 
> First of all we don't need a batch compress/decompress interface
> because the whole point of request chaining is to supply the data
> in batches.
> 
> I'm also against having a get_batch_size because the user should
> be supplying as much data as they're comfortable with.  In other
> words if the user is happy to give us 8 requests for iaa then it
> should be happy to give us 8 requests for every implementation.
> 
> The request chaining interface should be such that processing
> 8 requests is always better than doing 1 request at a time as
> the cost is amortised.

Thanks for your comments. Can you please elaborate on how
request chaining would enable cost amortization for software
compressors? With the current implementation, a module like
zswap would need to do the following to invoke request chaining
for software compressors (in addition to pushing the chaining
to the user layer for IAA, as per your suggestion on not needing a
batch compress/decompress interface):

zswap_batch_compress():
   for (i = 0; i < nr_pages_in_batch; ++i) {
      /* set up the acomp_req "reqs[i]". */
      [ ... ]
      if (i)
	acomp_request_chain(reqs[i], reqs[0]);
      else
	acomp_reqchain_init(reqs[0], 0, crypto_req_done, crypto_wait);
   }

   /* Process the request chain in series. */
   err = crypto_wait_req(acomp_do_req_chain(reqs[0], crypto_acomp_compress), crypto_wait);

Internally, acomp_do_req_chain() would sequentially process the
request chain by:
1) adding all requests to a list "state"
2) call "crypto_acomp_compress()" for the next list element
3) when this request completes, dequeue it from the list "state"
4) repeat for all requests in "state"
5) When the last request in "state" completes, call "reqs[0]->base.complete()",
    which notifies crypto_wait.

From what I can understand, the latency cost should be the same for
processing a request chain in series vs. processing each request as it is
done today in zswap, by calling:

  comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->reqs[0]), &acomp_ctx->wait);

It is not clear to me if there is a cost amortization benefit for software
compressors. One of the requirements from Yosry was that there should
be no change for the software compressors, which is what I have
attempted to do in v5.

Can you please help us understand if there is a room for optimizing
the implementation of the synchronous "acomp_do_req_chain()" API?
I would also like to get inputs from the zswap maintainers on using
request chaining for a batching implementation for software compressors.

Thanks,
Kanchana

> 
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt