Message-ID: <SA3PR11MB812018111A1903EC4889523EC9C82@SA3PR11MB8120.namprd11.prod.outlook.com>
Date: Tue, 4 Mar 2025 21:14:40 +0000
From: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
To: Herbert Xu <herbert@...dor.apana.org.au>
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>, "yosry.ahmed@...ux.dev" <yosry.ahmed@...ux.dev>,
	"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>, "linux-crypto@...r.kernel.org"
	<linux-crypto@...r.kernel.org>, "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>, "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 v7 01/15] crypto: acomp - Add synchronous/asynchronous
 acomp request chaining.


> -----Original Message-----
> From: Herbert Xu <herbert@...dor.apana.org.au>
> Sent: Monday, March 3, 2025 9:20 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> Cc: linux-kernel@...r.kernel.org; linux-mm@...ck.org;
> hannes@...xchg.org; yosry.ahmed@...ux.dev; 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; linux-
> crypto@...r.kernel.org; davem@...emloft.net; clabbe@...libre.com;
> ardb@...nel.org; ebiggers@...gle.com; surenb@...gle.com; Accardi,
> Kristen C <kristen.c.accardi@...el.com>; Feghali, Wajdi K
> <wajdi.k.feghali@...el.com>; Gopal, Vinodh <vinodh.gopal@...el.com>
> Subject: Re: [PATCH v7 01/15] crypto: acomp - Add
> synchronous/asynchronous acomp request chaining.
> 
> On Fri, Feb 28, 2025 at 02:00:10AM -0800, Kanchana P Sridhar wrote:
> >
> >  Step 2: Process the request chain using the specified compress/decompress
> >          "op":
> >
> >   2.a) Synchronous: the chain of requests is processed in series:
> >
> >        int acomp_do_req_chain(struct acomp_req *req,
> >                               int (*op)(struct acomp_req *req));
> >
> >   2.b) Asynchronous: the chain of requests is processed in parallel using a
> >        submit-poll paradigm:
> >
> >        int acomp_do_async_req_chain(struct acomp_req *req,
> >                                     int (*op_submit)(struct acomp_req *req),
> >                                     int (*op_poll)(struct acomp_req *req));
> >
> > Request chaining will be used in subsequent patches to implement
> > compress/decompress batching in the iaa_crypto driver for the two
> supported
> > IAA driver sync_modes:
> >
> >   sync_mode = 'sync' will use (2.a),
> >   sync_mode = 'async' will use (2.b).
> 
> There shouldn't be any sync/async toggle.  The whole zswap code is
> synchronous only and it makes zero sense to expose this to the user.
> Just do whatever is the fastest from the driver's point of view.

These sync/async modes are mentioned here to distinguish between how 
request chaining will be supported from the iaa_crypto driver's perspective.
As you are aware, the iaa_crypto driver supports a fully synchronous mode
(sync_mode = 'sync') and a fully asynchronous, non-irq mode (sync_mode = 'async').

As mentioned in the cover letter, from zswap's perspective, the calls to crypto
are exactly the same whether or not batching (with request chaining) or sequential
compressions are used:

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


> 
> I've actually implemented acomp chaining in my tree and I will be
> reposting soon.

Thanks for the update. I took at look at your v2 [1], and will provide some
code review comments in [1]. My main open question is, how is this supposed
to work for iaa_crypto's submit-poll paradigm which is crucial to derive the 
benefits of hardware parallelism? IIUC, your v2 acomp request chaining only
works in fully synchronous mode, correct me if I am wrong.

In fact, at the start of "acomp_do_req_chain()", if the algorithm has opted in to
request chaining, it returns after processing the first request, without processing
the chained requests. These are some of the issues I had to resolve when using
the ahash reference implementation you provided, to develop my version of
acomp_do_request_chain() and acomp_do_async_request_chain() in patch 1
of my series.

I see that in your v2, you have introduced support for virtual addresses. One
suggestion I have is, can you please incorporate my implementation of acomp
request chaining (for both the above APIs) in a v3 of your patch-series that
enables both, acomp_do_async_request_chain() and acomp_do_request_chain(),
fixes some of the issues I pointed out above, and adds in the virtual address
support. Please let me know if this would be a good way for us to proceed in
getting zswap to realize the benefits of IAA batch compressions.

[1] https://patchwork.kernel.org/project/linux-mm/patch/a11883ded326c4f4f80dcf0307ad05fd8e31abc7.1741080140.git.herbert@gondor.apana.org.au/

Thanks,
Kanchana

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

Powered by blists - more mailing lists