Re: [PATCH] crypto: algif - change algif_skcipher to be asynchronous

Re: [PATCH] crypto: algif - change algif_skcipher to be asynchronous

[Herbert Xu]

On Tue, Jan 13, 2015 at 12:28:23PM -0800, Tadeusz Struk wrote:
> The way the algif_skcipher works currently is that on sendmsg/sendpage it
> builds an sgl for the input data and then on read/recvmsg it sends the job
> for encryption putting the user to sleep till the data is processed.
> This way it can only handle one job at a given time.
> This patch changes it to be asynchronous.
> The idea is to allow enqueue multiple jobs to get most of available crypto HW
> accelerators and then read when the data is processed without blocking.
> To allow that both the input and output sgl need to be know at sendmsg/sendpage
> or the operation needs to happen "in place" in the input sgl. The approach here
> is to use the "in place" operation and process the data in the sgl provided in
> sendmsg. To allow that new user visible flags are introduced:
> ALG_SET_OP_TYPE
> ALG_OP_OUTOF_PLACE
> ALG_OP_IN_PLACE
> By default the operation type is ALG_OP_OUTOF_PLACE, which works the same way as
> without the change and allows existing application working without any update.
> 
> Using the test application from https://lkml.org/lkml/2011/8/28/87 with small
> modification to support in place operation, and reading after every 16th
> sendmsg these are the results:

What you want is AIO so we should try to use that interface rather
than creating some funky crypto-specific interface.

Dave, the AIO hooks in net/socket.c is currently simply pointing
to the sync implementation.  What are you thoughts on allowing
socket implementations to supply these hooks?

The algif interface can then use these hooks to implement AIO
which is useful for maximising the hardware performance without
resorting to loads of threads.

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

Re: [PATCH] crypto: algif - change algif_skcipher to be asynchronous

[Tadeusz Struk]

Hi Herbert,
On 01/13/2015 09:38 PM, Herbert Xu wrote:
> What you want is AIO so we should try to use that interface rather
> than creating some funky crypto-specific interface.
> 
> Dave, the AIO hooks in net/socket.c is currently simply pointing
> to the sync implementation.  What are you thoughts on allowing
> socket implementations to supply these hooks?
> 
> The algif interface can then use these hooks to implement AIO
> which is useful for maximising the hardware performance without
> resorting to loads of threads.

But then would you like to extend AIO interface to take the IV and
something that would indicate the encrypt/decrypt operation on
aio_write()? Also as far as I can see AIO doesn't support splice()
operation for zero copy, which is the main thing here.

>From the other hand it shouldn't be a problem to add crypto specific
stuff to include/uapi/linux/if_alg.h, because it is all about crypto
anyway, is it not?

If you have a better way how to indicate that data processing should
start on the last page in sendpage() instead of ALG_OP_IN_PLACE I would
be happy to use it.
Thanks,
Tadeusz

Re: [PATCH] crypto: algif - change algif_skcipher to be asynchronous

[Herbert Xu]

On Wed, Jan 14, 2015 at 09:36:57AM -0800, Tadeusz Struk wrote:
> 
> But then would you like to extend AIO interface to take the IV and
> something that would indicate the encrypt/decrypt operation on
> aio_write()? Also as far as I can see AIO doesn't support splice()

Any metadata such as the IV can still go through the existing
sendmsg interface, just as you would do a sendmsg before a sendfile
to set things up.

> operation for zero copy, which is the main thing here.

The AIO interface itself can accomodate zero-copy.  It's just that
we currently don't have any support for it in the network socket
API.

> >From the other hand it shouldn't be a problem to add crypto specific
> stuff to include/uapi/linux/if_alg.h, because it is all about crypto
> anyway, is it not?

Yes but you're violating the meaning of sendpage().  The latter
is not crypto-specific so you shouldn't be adding things that
prevent future optimisations to it.

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

Re: [PATCH] crypto: algif - change algif_skcipher to be asynchronous

[Tadeusz Struk]

On 01/15/2015 06:00 PM, Herbert Xu wrote:
>> But then would you like to extend AIO interface to take the IV and
>> > something that would indicate the encrypt/decrypt operation on
>> > aio_write()? Also as far as I can see AIO doesn't support splice()
> Any metadata such as the IV can still go through the existing
> sendmsg interface, just as you would do a sendmsg before a sendfile
> to set things up.
> 
>> > operation for zero copy, which is the main thing here.
> The AIO interface itself can accomodate zero-copy.  It's just that
> we currently don't have any support for it in the network socket
> API.
> 
Hi,
Ok, It looks to me that we *do* have all we need to implement zero copy
and AIO with algif_skcipher. The only thing we need to do is to add
support for it in skcipher_recvmsg(). I think no change is required in
neither skcipher_sendmsg(), skcipher_sendpage(), nor the if_alg interface.
Then to start using the interface asynchronously an application will
need to call aio_read() or lio_listio() instead of read(), but if
someone will use read(), then it will still work in the same
(synchronous) way as it is today.
How does this sound to you, Herbert?
I'll send a v2 shortly.
Thanks,
Tadeusz

Re: [PATCH] crypto: algif - change algif_skcipher to be asynchronous

[Herbert Xu]

On Fri, Jan 23, 2015 at 02:40:56PM -0800, Tadeusz Struk wrote:
>
> Ok, It looks to me that we *do* have all we need to implement zero copy
> and AIO with algif_skcipher. The only thing we need to do is to add
> support for it in skcipher_recvmsg(). I think no change is required in
> neither skcipher_sendmsg(), skcipher_sendpage(), nor the if_alg interface.
> Then to start using the interface asynchronously an application will
> need to call aio_read() or lio_listio() instead of read(), but if
> someone will use read(), then it will still work in the same
> (synchronous) way as it is today.
> How does this sound to you, Herbert?

It sounds good but let's see the code first :)
-- 
Email: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt