> -----Original Message-----
> From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, March 4, 2025 5:51 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx>
> Cc: Linux Crypto Mailing List <linux-crypto@xxxxxxxxxxxxxxx>; linux-
> mm@xxxxxxxxx; Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxx>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
