RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Dey, Megha >Sent: Friday, May 11, 2018 6:22 PM >To: Herbert Xu >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > > > >>-Original Message- >>From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >>Sent: Thursday, May 10, 2018 9:46 PM >>To: Dey, Megha >>Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >>da...@davemloft.net >>Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption >>infrastructure support >> >>On Fri, May 11, 2018 at 01:24:42AM +, Dey, Megha wrote: >>> >>> Are you suggesting that the SIMD wrapper, will do what is currently >>> being >>done by the ' mcryptd_queue_worker ' function (assuming FPU is not >>disabled) i.e dispatching the job to the inner algorithm? >>> >>> I have got rid of the mcryptd layer( have an inner layer, outer SIMD >>> layer, >>handled the pointers and completions accordingly), but still facing >>some issues after removing the per cpu mcryptd_cpu_queue. >> >>Why don't you post what you've got and we can work it out together? > >Hi Herbert, > >Sure, I will post an RFC patch. (crypto: Remove mcryptd). Hi Herbert, I had posted an RFC patch about a month back (2018/05/11). Could you please have a look? [RFC] crypto: Remove mcryptd > >> >>Thanks, >>-- >>Email: Herbert Xu Home Page: >>http://gondor.apana.org.au/~herbert/ >>PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Thursday, May 10, 2018 9:46 PM >To: Dey, Megha >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > >On Fri, May 11, 2018 at 01:24:42AM +, Dey, Megha wrote: >> >> Are you suggesting that the SIMD wrapper, will do what is currently being >done by the ' mcryptd_queue_worker ' function (assuming FPU is not disabled) >i.e dispatching the job to the inner algorithm? >> >> I have got rid of the mcryptd layer( have an inner layer, outer SIMD layer, >handled the pointers and completions accordingly), but still facing some issues >after removing the per cpu mcryptd_cpu_queue. > >Why don't you post what you've got and we can work it out together? Hi Herbert, Sure, I will post an RFC patch. (crypto: Remove mcryptd). > >Thanks, >-- >Email: Herbert Xu Home Page: >http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Fri, May 11, 2018 at 01:24:42AM +, Dey, Megha wrote: > > Are you suggesting that the SIMD wrapper, will do what is currently being > done by the ' mcryptd_queue_worker ' function (assuming FPU is not disabled) > i.e dispatching the job to the inner algorithm? > > I have got rid of the mcryptd layer( have an inner layer, outer SIMD layer, > handled the pointers and completions accordingly), but still facing some > issues after removing the per cpu mcryptd_cpu_queue. Why don't you post what you've got and we can work it out together? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Monday, May 7, 2018 2:35 AM >To: Dey, Megha >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > >On Tue, May 01, 2018 at 10:39:15PM +, Dey, Megha wrote: >> >> crypto/simd.c provides a simd_skcipher_create_compat. I have used the >> same template to introduce simd_ahash_create_compat which would wrap >around the inner hash algorithm. >> >> Hence we would still register 2 algs, outer and inner. > >Right. > >> Currently we have outer_alg -> mcryptd alg -> inner_alg >> >> Mcryptd is mainly providing the following: >> 1. Ensuring the lanes(8 in case of AVX2) are full before dispatching to the >lower inner algorithm. This is obviously why we would expect better >performance for multi-buffer as opposed to the present single-buffer >algorithms. >> 2. If there no new incoming jobs, issue a flush. >> 3. A glue layer which sends the correct pointers and completions. >> >> If we get rid of mcryptd, these functions needs to be done by someone. Since >all multi-buffer algorithms would require this tasks, where do you suggest >these >helpers live, if not the current mcryptd.c? > >That's the issue. I don't think mcryptd is doing any of these claimed >functions >except for hosting the flush helper which could really live anywhere. > >All these functions are actually being carried out in the inner algorithm >already. > >> I am not sure if you are suggesting that we need to get rid of the mcryptd >work queue itself. In that case, we would need to execute in the context of the >job requesting the crypto transformation. > >Which is fine as long as you can disable the FPU. If not the simd wrapper will >defer the job to kthread context as required. Hi Herbert, Are you suggesting that the SIMD wrapper, will do what is currently being done by the ' mcryptd_queue_worker ' function (assuming FPU is not disabled) i.e dispatching the job to the inner algorithm? I have got rid of the mcryptd layer( have an inner layer, outer SIMD layer, handled the pointers and completions accordingly), but still facing some issues after removing the per cpu mcryptd_cpu_queue. > >Cheers, >-- >Email: Herbert Xu Home Page: >http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Tue, May 01, 2018 at 10:39:15PM +, Dey, Megha wrote: > > crypto/simd.c provides a simd_skcipher_create_compat. I have used the same > template to introduce simd_ahash_create_compat > which would wrap around the inner hash algorithm. > > Hence we would still register 2 algs, outer and inner. Right. > Currently we have outer_alg -> mcryptd alg -> inner_alg > > Mcryptd is mainly providing the following: > 1. Ensuring the lanes(8 in case of AVX2) are full before dispatching to the > lower inner algorithm. This is obviously why we would expect better > performance for multi-buffer as opposed to the present single-buffer > algorithms. > 2. If there no new incoming jobs, issue a flush. > 3. A glue layer which sends the correct pointers and completions. > > If we get rid of mcryptd, these functions needs to be done by someone. Since > all multi-buffer algorithms would require this tasks, where do you suggest > these helpers live, if not the current mcryptd.c? That's the issue. I don't think mcryptd is doing any of these claimed functions except for hosting the flush helper which could really live anywhere. All these functions are actually being carried out in the inner algorithm already. > I am not sure if you are suggesting that we need to get rid of the mcryptd > work queue itself. In that case, we would need to execute in the context of > the job requesting the crypto transformation. Which is fine as long as you can disable the FPU. If not the simd wrapper will defer the job to kthread context as required. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Thursday, April 26, 2018 2:45 AM >To: Dey, Megha >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > >On Wed, Apr 25, 2018 at 01:14:26AM +, Dey, Megha wrote: >> >> Is there any existing implementation of async crypto algorithm that uses the >above approach? The ones I could find are either sync, have an outer and >inner algorithm or use cryptd. >> >> I tried removing the mcryptd layer and the outer algorithm and some >> plumbing to pass the correct structures, but see crashes.(obviously >> some errors in the plumbing) > >OK, you can't just remove it because the inner algorithm requires >kernel_fpu_begin/kernel_fpu_end. So we do need two layers but I don't think >we need cryptd or mcryptd. > >The existing simd wrapper should work just fine on the inner algorithm, >provided that we add hash support to it. Hi Herbert, crypto/simd.c provides a simd_skcipher_create_compat. I have used the same template to introduce simd_ahash_create_compat which would wrap around the inner hash algorithm. Hence we would still register 2 algs, outer and inner. > >> I am not sure if we remove mcryptd, how would we queue work, flush >partially completed jobs or call completions (currently done by mcryptd) if we >simply call the inner algorithm. > >I don't think mcryptd is providing any real facility to the flushing apart >from a >helper. That same helper can live anywhere. Currently we have outer_alg -> mcryptd alg -> inner_alg Mcryptd is mainly providing the following: 1. Ensuring the lanes(8 in case of AVX2) are full before dispatching to the lower inner algorithm. This is obviously why we would expect better performance for multi-buffer as opposed to the present single-buffer algorithms. 2. If there no new incoming jobs, issue a flush. 3. A glue layer which sends the correct pointers and completions. If we get rid of mcryptd, these functions needs to be done by someone. Since all multi-buffer algorithms would require this tasks, where do you suggest these helpers live, if not the current mcryptd.c? I am not sure if you are suggesting that we need to get rid of the mcryptd work queue itself. In that case, we would need to execute in the context of the job requesting the crypto transformation. > >Cheers, >-- >Email: Herbert Xu Home Page: >http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Wed, Apr 25, 2018 at 01:14:26AM +, Dey, Megha wrote: > > Is there any existing implementation of async crypto algorithm that uses the > above approach? The ones I could find are either sync, have an outer and > inner algorithm or use cryptd. > > I tried removing the mcryptd layer and the outer algorithm and some plumbing > to pass the correct structures, but see crashes.(obviously some errors in the > plumbing) OK, you can't just remove it because the inner algorithm requires kernel_fpu_begin/kernel_fpu_end. So we do need two layers but I don't think we need cryptd or mcryptd. The existing simd wrapper should work just fine on the inner algorithm, provided that we add hash support to it. > I am not sure if we remove mcryptd, how would we queue work, flush partially > completed jobs or call completions (currently done by mcryptd) if we simply > call the inner algorithm. I don't think mcryptd is providing any real facility to the flushing apart from a helper. That same helper can live anywhere. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Wednesday, April 18, 2018 8:25 PM >To: Dey, Megha >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > >On Thu, Apr 19, 2018 at 12:54:16AM +, Dey, Megha wrote: >> >> Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c >completely and avoid the extra layer of indirection to call the underlying >algorithm, instead call it directly, correct? >> >> So currently we have 3 algorithms registered for every multibuffer algorithm: >> name : __sha1-mb >> driver : mcryptd(__intel_sha1-mb) >> >> name : sha1 >> driver : sha1_mb >> >> name : __sha1-mb >> driver : __intel_sha1-mb >> >> If we remove mcryptd, then we will have just the 2? > >It should be down to just one, i.e., the current inner algorithm. >It's doing all the scheduling work already so I don't really see why it needs >the >wrappers around it. Hi Herbert, Is there any existing implementation of async crypto algorithm that uses the above approach? The ones I could find are either sync, have an outer and inner algorithm or use cryptd. I tried removing the mcryptd layer and the outer algorithm and some plumbing to pass the correct structures, but see crashes.(obviously some errors in the plumbing) I am not sure if we remove mcryptd, how would we queue work, flush partially completed jobs or call completions (currently done by mcryptd) if we simply call the inner algorithm. Are you suggesting these are not required at all? I am not sure how to move forward. > >Cheers, >-- >Email: Herbert Xu Home Page: >http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Thu, Apr 19, 2018 at 12:54:16AM +, Dey, Megha wrote: > > Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c > completely and avoid the extra layer of indirection to call the underlying > algorithm, instead call it directly, correct? > > So currently we have 3 algorithms registered for every multibuffer algorithm: > name : __sha1-mb > driver : mcryptd(__intel_sha1-mb) > > name : sha1 > driver : sha1_mb > > name : __sha1-mb > driver : __intel_sha1-mb > > If we remove mcryptd, then we will have just the 2? It should be down to just one, i.e., the current inner algorithm. It's doing all the scheduling work already so I don't really see why it needs the wrappers around it. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Wednesday, April 18, 2018 4:01 AM >To: Dey, Megha >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > >On Tue, Apr 17, 2018 at 06:40:17PM +, Dey, Megha wrote: >> >> >> >-Original Message- >> >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >> >Sent: Friday, March 16, 2018 7:54 AM >> >To: Dey, Megha >> >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >> >da...@davemloft.net >> >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption >> >infrastructure support >> > >> >I have taken a deeper look and I'm even more convinced now that >> >mcryptd is simply not needed in your current model. >> > >> >The only reason you would need mcryptd is if you need to limit the >> >rate of requests going into the underlying mb algorithm. >> > >> >However, it doesn't do that all. Even though it seems to have a >> >batch size of 10, but because it immediately reschedules itself after >> >the batch runs out, it's essentially just dumping all requests at the >> >underlying algorithm as fast as they're coming in. The underlying >> >algorithm doesn't have need throttling anyway because it'll do the work >when the queue is full synchronously. >> > >> >So why not just get rid of mcryptd completely and expose the >> >underlying algorithm as a proper async skcipher/hash? >> >> Hi Herbert, >> >> Most part of the cryptd.c and mcryptd.c are similar, except the logic >> used to flush out partially completed jobs in the case of multibuffer >algorithms. >> >> I think I will try to merge the cryptd and mcryptd adding necessary quirks >> for >multibuffer where needed. > >I think you didn't quite get my point. From what I'm seeing you don't need >either cryptd or mcryptd. You just need to expose the underlying mb >algorithm directly. Hi Herbert, Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c completely and avoid the extra layer of indirection to call the underlying algorithm, instead call it directly, correct? So currently we have 3 algorithms registered for every multibuffer algorithm: name : __sha1-mb driver : mcryptd(__intel_sha1-mb) name : sha1 driver : sha1_mb name : __sha1-mb driver : __intel_sha1-mb If we remove mcryptd, then we will have just the 2? The outer algorithm:sha1-mb, will > >So I'm not sure what we would gain from merging cryptd and mcryptd. > >Cheers, >-- >Email: Herbert Xu Home Page: >http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Tue, Apr 17, 2018 at 06:40:17PM +, Dey, Megha wrote: > > > >-Original Message- > >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] > >Sent: Friday, March 16, 2018 7:54 AM > >To: Dey, Megha > >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; > >da...@davemloft.net > >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure > >support > > > >I have taken a deeper look and I'm even more convinced now that mcryptd is > >simply not needed in your current model. > > > >The only reason you would need mcryptd is if you need to limit the rate of > >requests going into the underlying mb algorithm. > > > >However, it doesn't do that all. Even though it seems to have a batch size > >of > >10, but because it immediately reschedules itself after the batch runs out, > >it's essentially just dumping all requests at the underlying algorithm as > >fast > >as they're coming in. The underlying algorithm doesn't have need throttling > >anyway because it'll do the work when the queue is full synchronously. > > > >So why not just get rid of mcryptd completely and expose the underlying > >algorithm as a proper async skcipher/hash? > > Hi Herbert, > > Most part of the cryptd.c and mcryptd.c are similar, except the logic used to > flush out partially completed jobs > in the case of multibuffer algorithms. > > I think I will try to merge the cryptd and mcryptd adding necessary quirks > for multibuffer where needed. I think you didn't quite get my point. From what I'm seeing you don't need either cryptd or mcryptd. You just need to expose the underlying mb algorithm directly. So I'm not sure what we would gain from merging cryptd and mcryptd. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Friday, March 16, 2018 7:54 AM >To: Dey, Megha >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > >On Thu, Jan 18, 2018 at 04:44:21PM -0800, Megha Dey wrote: >> >> > So the mcryptd template is in fact completely superfluous. You can >> > remove it and just have all the main encrypt/decrypt functions >> > invoke the underlying encrypt/decrypt function directly and achieve >> > the same result. >> > >> > Am I missing something? >> >> Hi Herbert, >> >> After discussing with Tim, it seems like the mcryptd is responsible >> for queuing up the encrypt requests and dispatching them to the actual >> multi-buffer raw algorithm. It also flushes the queue if we wait too >> long without new requests coming in to force dispatch of the requests >> in queue. >> >> Its function is analogous to cryptd but it has its own multi-lane >> twists so we haven't reused the cryptd interface. > >I have taken a deeper look and I'm even more convinced now that mcryptd is >simply not needed in your current model. > >The only reason you would need mcryptd is if you need to limit the rate of >requests going into the underlying mb algorithm. > >However, it doesn't do that all. Even though it seems to have a batch size of >10, but because it immediately reschedules itself after the batch runs out, >it's essentially just dumping all requests at the underlying algorithm as fast >as they're coming in. The underlying algorithm doesn't have need throttling >anyway because it'll do the work when the queue is full synchronously. > >So why not just get rid of mcryptd completely and expose the underlying >algorithm as a proper async skcipher/hash? Hi Herbert, Most part of the cryptd.c and mcryptd.c are similar, except the logic used to flush out partially completed jobs in the case of multibuffer algorithms. I think I will try to merge the cryptd and mcryptd adding necessary quirks for multibuffer where needed. Also, in cryptd.c, I see shash interface being used for hash digests, update, finup, setkey etc. whereas we have shifted to ahash interface for mcryptd. Is this correct? Thanks, Megha > >Thanks, >-- >Email: Herbert Xu Home Page: >http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Thu, Jan 18, 2018 at 04:44:21PM -0800, Megha Dey wrote: > > > So the mcryptd template is in fact completely superfluous. You > > can remove it and just have all the main encrypt/decrypt functions > > invoke the underlying encrypt/decrypt function directly and achieve > > the same result. > > > > Am I missing something? > > Hi Herbert, > > After discussing with Tim, it seems like the mcryptd is responsible for > queuing up the encrypt requests and dispatching them to the actual > multi-buffer raw algorithm. It also flushes the queue > if we wait too long without new requests coming in to force dispatch of > the requests in queue. > > Its function is analogous to cryptd but it has its own multi-lane twists > so we haven't reused the cryptd interface. I have taken a deeper look and I'm even more convinced now that mcryptd is simply not needed in your current model. The only reason you would need mcryptd is if you need to limit the rate of requests going into the underlying mb algorithm. However, it doesn't do that all. Even though it seems to have a batch size of 10, but because it immediately reschedules itself after the batch runs out, it's essentially just dumping all requests at the underlying algorithm as fast as they're coming in. The underlying algorithm doesn't have need throttling anyway because it'll do the work when the queue is full synchronously. So why not just get rid of mcryptd completely and expose the underlying algorithm as a proper async skcipher/hash? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Thu, 2018-01-18 at 22:39 +1100, Herbert Xu wrote:
> On Tue, Jan 09, 2018 at 04:09:04PM -0800, Megha Dey wrote:
> >
> > +static void mcryptd_skcipher_encrypt(struct crypto_async_request *base,
> > + int err)
> > +{
> > + struct skcipher_request *req = skcipher_request_cast(base);
> > + struct mcryptd_skcipher_request_ctx *rctx = skcipher_request_ctx(req);
> > + struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> > + struct mcryptd_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
> > + struct crypto_skcipher *child = ctx->child;
> > + struct skcipher_request subreq;
> > +
> > + if (unlikely(err == -EINPROGRESS))
> > + goto out;
> > +
> > + /* set up the skcipher request to work on */
> > + skcipher_request_set_tfm(&subreq, child);
> > + skcipher_request_set_callback(&subreq,
> > + CRYPTO_TFM_REQ_MAY_SLEEP, 0, 0);
> > + skcipher_request_set_crypt(&subreq, req->src, req->dst,
> > + req->cryptlen, req->iv);
> > +
> > + /*
> > +* pass addr of descriptor stored in the request context
> > +* so that the callee can get to the request context
> > +*/
> > + rctx->desc = subreq;
> > + err = crypto_skcipher_encrypt(&rctx->desc);
> > +
> > + if (err) {
> > + req->base.complete = rctx->complete;
> > + goto out;
> > + }
> > + return;
> > +
> > +out:
> > + mcryptd_skcipher_complete(req, err);
> > +}
>
> OK this looks better but it's still abusing the crypto API interface.
> In particular, you're sharing data with the underlying algorithm
> behind the crypto API's back. Also, the underlying algorithm does
> callback completion behind the API's back through the shared data
> context.
>
> It seems to me that the current mcryptd scheme is flawed. You
> want to batch multiple requests and yet this isn't actually being
> done by mcryptd at all. The actual batching happens at the very
> lowest level, i.e., in the crypto algorithm below mcryptd. For
> example, with your patch, the batching appears to happen in
> aes_cbc_job_mgr_submit.
>
> So the mcryptd template is in fact completely superfluous. You
> can remove it and just have all the main encrypt/decrypt functions
> invoke the underlying encrypt/decrypt function directly and achieve
> the same result.
>
> Am I missing something?
Hi Herbert,
After discussing with Tim, it seems like the mcryptd is responsible for
queuing up the encrypt requests and dispatching them to the actual
multi-buffer raw algorithm. It also flushes the queue
if we wait too long without new requests coming in to force dispatch of
the requests in queue.
Its function is analogous to cryptd but it has its own multi-lane twists
so we haven't reused the cryptd interface.
>
> Cheers,
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Tue, Jan 09, 2018 at 04:09:04PM -0800, Megha Dey wrote:
>
> +static void mcryptd_skcipher_encrypt(struct crypto_async_request *base,
> + int err)
> +{
> + struct skcipher_request *req = skcipher_request_cast(base);
> + struct mcryptd_skcipher_request_ctx *rctx = skcipher_request_ctx(req);
> + struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> + struct mcryptd_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
> + struct crypto_skcipher *child = ctx->child;
> + struct skcipher_request subreq;
> +
> + if (unlikely(err == -EINPROGRESS))
> + goto out;
> +
> + /* set up the skcipher request to work on */
> + skcipher_request_set_tfm(&subreq, child);
> + skcipher_request_set_callback(&subreq,
> + CRYPTO_TFM_REQ_MAY_SLEEP, 0, 0);
> + skcipher_request_set_crypt(&subreq, req->src, req->dst,
> + req->cryptlen, req->iv);
> +
> + /*
> + * pass addr of descriptor stored in the request context
> + * so that the callee can get to the request context
> + */
> + rctx->desc = subreq;
> + err = crypto_skcipher_encrypt(&rctx->desc);
> +
> + if (err) {
> + req->base.complete = rctx->complete;
> + goto out;
> + }
> + return;
> +
> +out:
> + mcryptd_skcipher_complete(req, err);
> +}
OK this looks better but it's still abusing the crypto API interface.
In particular, you're sharing data with the underlying algorithm
behind the crypto API's back. Also, the underlying algorithm does
callback completion behind the API's back through the shared data
context.
It seems to me that the current mcryptd scheme is flawed. You
want to batch multiple requests and yet this isn't actually being
done by mcryptd at all. The actual batching happens at the very
lowest level, i.e., in the crypto algorithm below mcryptd. For
example, with your patch, the batching appears to happen in
aes_cbc_job_mgr_submit.
So the mcryptd template is in fact completely superfluous. You
can remove it and just have all the main encrypt/decrypt functions
invoke the underlying encrypt/decrypt function directly and achieve
the same result.
Am I missing something?
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
