RE: [v2 PATCH 3/7] crypto: acomp - Add request chaining and virtual addresses

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

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Tuesday, March 4, 2025 5:51 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> Cc: Linux Crypto Mailing List <linux-crypto@vger.kernel.org>; linux-
> mm@kvack.org; Yosry Ahmed <yosry.ahmed@linux.dev>
> Subject: Re: [v2 PATCH 3/7] crypto: acomp - Add request chaining and virtual
> addresses
> 
> On Tue, Mar 04, 2025 at 09:59:59PM +0000, Sridhar, Kanchana P wrote:
> >
> > > +static int acomp_reqchain_finish(struct acomp_req_chain *state,
> > > +				 int err, u32 mask)
> > > +{
> > > +	struct acomp_req *req0 = state->req0;
> > > +	struct acomp_req *req = state->cur;
> > > +	struct acomp_req *n;
> > > +
> > > +	acomp_reqchain_virt(state, err);
> >
> > Unless I am missing something, this seems to be future-proofing, based
> > on the initial checks you've implemented in acomp_do_req_chain().
> >
> > > +
> > > +	if (req != req0)
> > > +		list_add_tail(&req->base.list, &req0->base.list);
> > > +
> > > +	list_for_each_entry_safe(req, n, &state->head, base.list) {
> > > +		list_del_init(&req->base.list);
> > > +
> > > +		req->base.flags &= mask;
> > > +		req->base.complete = acomp_reqchain_done;
> > > +		req->base.data = state;
> > > +		state->cur = req;
> > > +
> > > +		if (acomp_request_isvirt(req)) {
> > > +			unsigned int slen = req->slen;
> > > +			unsigned int dlen = req->dlen;
> > > +			const u8 *svirt = req->svirt;
> > > +			u8 *dvirt = req->dvirt;
> > > +
> > > +			state->src = svirt;
> > > +			state->dst = dvirt;
> > > +
> > > +			sg_init_one(&state->ssg, svirt, slen);
> > > +			sg_init_one(&state->dsg, dvirt, dlen);
> > > +
> > > +			acomp_request_set_params(req, &state->ssg,
> > > &state->dsg,
> > > +						 slen, dlen);
> > > +		}
> > > +
> > > +		err = state->op(req);
> > > +
> > > +		if (err == -EINPROGRESS) {
> > > +			if (!list_empty(&state->head))
> > > +				err = -EBUSY;
> > > +			goto out;
> > > +		}
> > > +
> > > +		if (err == -EBUSY)
> > > +			goto out;
> >
> > This is a fully synchronous way of processing the request chain, and
> > will not work for iaa_crypto's submit-then-poll-for-completions paradigm,
> > essential for us to process the compressions in parallel in hardware.
> > Without parallelism, we will not derive the full benefits of IAA.
> 
> This function is not for chaining drivers at all.  It's for existing
> drivers that do *not* support chaining.
> 
> If your driver supports chaining, then it should not come through
> acomp_reqchain_finish in the first place.  The acomp_reqchain code
> translates chained requests to simple unchained ones for the
> existing drivers.  If the driver supports chaining natively, then
> it will bypass all this go straight to the driver, where you can do
> whatever you want with the chained request.

Hi Herbert,

Can you please take a look at patches 1 (only the acomp_do_async_req_chain() interface),
2 and 4 in my latest v8 "zswap IAA compress batching" series [2], wherein I have tried to
address your comments [1] given in v6, and let me know if this implements
batching with request chaining as you envision?

[1] https://patchwork.kernel.org/comment/26246560/
[2] https://patchwork.kernel.org/project/linux-mm/list/?series=939487

If this architecture looks Ok from your perspective, then can you please
let me know if "acomp_do_async_req_chain()" would be helpful in general,
outside of the iaa_crypto driver, or would your recommendation be for
this to be specific to iaa_crypto?

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