crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Dmitry Kasatkin]

Hi,

One interesting issue

Many clients in the kernel set CRYPTO_TFM_REQ_MAY_SLEEP to desc.flags.
It is used by crypto_yeld().

But the flags also available in the driver.
One can assume that it is possible to sleep, but is not, because crypto
walk will
    walk->data = crypto_kmap(walk->pg, 0);

------------------
    for (nbytes = crypto_hash_walk_first_compat(hdesc, &walk, sg, len);
         nbytes > 0; nbytes = crypto_hash_walk_done(&walk, nbytes))
        nbytes = crypto_shash_update(desc, walk.data, nbytes);
------------------

That is quite confusing...
I would expect that driver could sleep while hw is doing calculation.

Seems most of the clients uses sync API (linux/net has only sync).

Any comments? Ideas?

Thanks.

- Dmitry

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Herbert Xu]

Dmitry Kasatkin <dmitry.kasatkin@nokia.com> wrote:
> 
> One interesting issue
> 
> Many clients in the kernel set CRYPTO_TFM_REQ_MAY_SLEEP to desc.flags.
> It is used by crypto_yeld().

That flag is really only meaningful for synchronous implementations.

For hardware crypto that is asynchronous, you can simply ignore it
in most cases (exceptions include invoking a syncrhonous backup, for
example).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Dmitry Kasatkin]

Hi,

Ok...

But normally kernel client code uses sync API as shash.

And I provide async hw driver via async ahash API.
That is simply will not be used at all.

Client must be changed to use async ahash API in order to benefit from
HW crypto.

Right?

Thanks,
Dmitry


ext Herbert Xu wrote:
> Dmitry Kasatkin <dmitry.kasatkin@nokia.com> wrote:
>   
>> One interesting issue
>>
>> Many clients in the kernel set CRYPTO_TFM_REQ_MAY_SLEEP to desc.flags.
>> It is used by crypto_yeld().
>>     
>
> That flag is really only meaningful for synchronous implementations.
>
> For hardware crypto that is asynchronous, you can simply ignore it
> in most cases (exceptions include invoking a syncrhonous backup, for
> example).
>
> Cheers,
>   

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Herbert Xu]

On Tue, Feb 09, 2010 at 10:45:31AM +0200, Dmitry Kasatkin wrote:
> Hi,
> 
> Ok...
> 
> But normally kernel client code uses sync API as shash.
> 
> And I provide async hw driver via async ahash API.
> That is simply will not be used at all.
> 
> Client must be changed to use async ahash API in order to benefit from
> HW crypto.
> 
> Right?

Yes we will convert hash users over to ahash where appropriate,
starting with authenc which covers IPsec.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Dmitry Kasatkin]

Hi,

Anyway, crypto driver is called with CRYPTO_TFM_REQ_MAY_SLEEP flag set.
It basically mean it can sleep.
But it is not because of kmap_atomic()

So no possibility to know if to use GFP_KERNEL or GFP_ATOMIC.

I guess that is not correct way.

If driver cannot sleep then CRYPTO_TFM_REQ_MAY_SLEEP should not be set
when calling it.

Even sync implementation might use HW and sleep a bit while HW is doing
calculation...


- Dmitry

ext Herbert Xu wrote:
> Dmitry Kasatkin <dmitry.kasatkin@nokia.com> wrote:
>   
>> One interesting issue
>>
>> Many clients in the kernel set CRYPTO_TFM_REQ_MAY_SLEEP to desc.flags.
>> It is used by crypto_yeld().
>>     
>
> That flag is really only meaningful for synchronous implementations.
>
> For hardware crypto that is asynchronous, you can simply ignore it
> in most cases (exceptions include invoking a syncrhonous backup, for
> example).
>
> Cheers,
>   

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Dmitry Kasatkin]

Hi,

Alg code and drivers uses req/desc->flags to use GFP_KERNEL or GFP_ATOMIC.

crypto_kmap() -> Does not it make a sense to also use kmap/kmap_atomic
instead of just kmap_atomic()?

-----------------------------------
static inline void *crypto_kmap(struct page *page, int out)
{
    return kmap_atomic(page, crypto_kmap_type(out));
}
-------------------------------------------------

It is now not only about sync but also async code.
Would be nice to "wait" for hash completion in some cases...

What do you think?
What is the reason to have only kmap_atomic()?

- Dmitry


Kasatkin Dmitry (Nokia-D/Helsinki) wrote:
> Hi,
>
> Anyway, crypto driver is called with CRYPTO_TFM_REQ_MAY_SLEEP flag set.
> It basically mean it can sleep.
> But it is not because of kmap_atomic()
>
> So no possibility to know if to use GFP_KERNEL or GFP_ATOMIC.
>
> I guess that is not correct way.
>
> If driver cannot sleep then CRYPTO_TFM_REQ_MAY_SLEEP should not be set
> when calling it.
>
> Even sync implementation might use HW and sleep a bit while HW is doing
> calculation...
>
>
> - Dmitry
>
> ext Herbert Xu wrote:
>   
>> Dmitry Kasatkin <dmitry.kasatkin@nokia.com> wrote:
>>   
>>     
>>> One interesting issue
>>>
>>> Many clients in the kernel set CRYPTO_TFM_REQ_MAY_SLEEP to desc.flags.
>>> It is used by crypto_yeld().
>>>     
>>>       
>> That flag is really only meaningful for synchronous implementations.
>>
>> For hardware crypto that is asynchronous, you can simply ignore it
>> in most cases (exceptions include invoking a syncrhonous backup, for
>> example).
>>
>> Cheers,
>>   
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Herbert Xu]

On Mon, Feb 15, 2010 at 09:20:35PM +0200, Dmitry Kasatkin wrote:
> Hi,
> 
> Alg code and drivers uses req/desc->flags to use GFP_KERNEL or GFP_ATOMIC.
> 
> crypto_kmap() -> Does not it make a sense to also use kmap/kmap_atomic
> instead of just kmap_atomic()?

crypto_kmap is not meant for async drivers.  Why do you need it?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Dmitry Kasatkin]

Hi,

we have HW accelerator which can be accessed without DMA.
CPU just sequentially writes data to the port and then reads result.

But before it can read result Interrupt will happen to notify processing
completion.

Instead of looping over "ready" bit the process would just go to sleep:
wait_for_completion().

Can you please shortly tell why crypto_kmap() cannot be also like
GFP_KERNEL or GFP_ATOMIC?

Is it really important to have kmap_atomic()?

Thanks,
Dmitry


ext Herbert Xu wrote:
> On Mon, Feb 15, 2010 at 09:20:35PM +0200, Dmitry Kasatkin wrote:
>   
>> Hi,
>>
>> Alg code and drivers uses req/desc->flags to use GFP_KERNEL or GFP_ATOMIC.
>>
>> crypto_kmap() -> Does not it make a sense to also use kmap/kmap_atomic
>> instead of just kmap_atomic()?
>>     
>
> crypto_kmap is not meant for async drivers.  Why do you need it?
>
> Cheers,
>   

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Herbert Xu]

On Tue, Feb 16, 2010 at 08:36:08AM +0200, Dmitry Kasatkin wrote:
> Hi,
> 
> we have HW accelerator which can be accessed without DMA.
> CPU just sequentially writes data to the port and then reads result.

Well if that's the case then you definitely don't need anything
other than kmap_atomic.  You should be able to map it atomically,
write the data out, unmap it, and then wait for the completion.

Non-atomic kmaps should be avoided if at all possible.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Dmitry Kasatkin]

Hi,

How is it possible to wait for completion here?: shash.c

shash_compat_digest()
{
....
        data = crypto_kmap(sg_page(sg), 0);
        err = crypto_shash_digest(desc, data + offset, nbytes, out);
        crypto_kunmap(data, 0);
....
}

It happens as single operation/call.

Thanks,
Dmitry


ext Herbert Xu wrote:
> On Tue, Feb 16, 2010 at 08:36:08AM +0200, Dmitry Kasatkin wrote:
>   
>> Hi,
>>
>> we have HW accelerator which can be accessed without DMA.
>> CPU just sequentially writes data to the port and then reads result.
>>     
>
> Well if that's the case then you definitely don't need anything
> other than kmap_atomic.  You should be able to map it atomically,
> write the data out, unmap it, and then wait for the completion.
>
> Non-atomic kmaps should be avoided if at all possible.
>
> Cheers,
>   

Re: crypto_shash_update & CRYPTO_TFM_REQ_MAY_SLEEP

[Herbert Xu]

On Tue, Feb 16, 2010 at 09:05:10AM +0200, Dmitry Kasatkin wrote:
> Hi,
> 
> How is it possible to wait for completion here?: shash.c
> 
> shash_compat_digest()
> {
> ....
>         data = crypto_kmap(sg_page(sg), 0);
>         err = crypto_shash_digest(desc, data + offset, nbytes, out);
>         crypto_kunmap(data, 0);
> ....
> }
> 
> It happens as single operation/call.

Well you'd do it as an ahash algorithm, not shash.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt