Re: [RFC 2/3] crypto: Introduce CRYPTO_ALG_BULK flag

[Milan Broz]

On 05/27/2016 09:04 AM, Baolin Wang wrote:
> Hi Milan,
> 
> On 27 May 2016 at 14:31, Milan Broz <gmazyland@xxxxxxxxx> wrote:
>> On 05/25/2016 08:12 AM, Baolin Wang wrote:
>>> Now some cipher hardware engines prefer to handle bulk block rather than one
>>> sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
>>> the intermediate values (IV) by themselves in one bulk block. This means we
>>> can increase the size of the request by merging request rather than always 512
>>> bytes and thus increase the hardware engine processing speed.
>>
>> Hi,
>>
>> could you please elaborate how exactly you are processing independently
>> encrypted sectors? For example with XTS mode. Do you play internally with
>> tweak calculation? Does this keep 512 bytes sector encryption blocks independent?
>>
>> (If not, it is breaking compatibility everywhere and you are reinventing
>> disk encryption logic here - just for performance reason for some hw
>> not designed for this task... But that was said several times already.)
> 
> These are what the cipher hardware engine and engine driver should do,
> for software we just need send one initial IV and bulk data to crypto
> layer, which is enough.

Hi,

Thanks for answer.

So this is just doing some kind of batch processing optimization inside the driver?
I still do not understand why it is not possible to "batch" these requests inside
you driver (with async API) then and process them in one go without any changes
to dmcrypt though. (But I am not familiar with these drivers.)

If I understand it correctly, subsequent IVs are calculated inside
your driver just based on some initial value?
This can work only for sequential IVs (or, better said, for predictable
IVs like plain64, implemented inside your hw/driver).

It cannot work for IVs/tweaks that are randomized or keyed (like ESSIV).
Yes, I know that using ESSIV for XTS is considered overkill but I do not
see you are checking this condition anywhere in code (we can definitely
configure such a mapping).

>>> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
>>> mode.
>>
>> What exactly skcipher will do if this flag is set?
> 
> I think that depends on how to implement the cipher engine driver.

I do not care about implementation details, I just would like to see
some high-level description for the new API flag.
(Or at least read the code that implements it :)

>> Which drivers it should use? I do not see any posted patch that uses this flag yet.
>> How we can test it?
> 
> Some cipher engine drivers which support bulk mode should use this
> flag. Yeah, we need upstream one cipher driver with this flag for
> testing.

I think if you introduce new flag, you should also post drivers that uses it.
Otherwise it is just unused code for mainline.

And I definitely would like to test it somewhere and see real
performance data (not just simple dd).

Thanks,
Milan

> 
>>
>> Milan
>>
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>> ---
>>>  include/crypto/skcipher.h |    7 +++++++
>>>  include/linux/crypto.h    |    6 ++++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
>>> index 0f987f5..d89d29a 100644
>>> --- a/include/crypto/skcipher.h
>>> +++ b/include/crypto/skcipher.h
>>> @@ -519,5 +519,12 @@ static inline void skcipher_request_set_crypt(
>>>       req->iv = iv;
>>>  }
>>>
>>> +static inline unsigned int skcipher_is_bulk_mode(struct crypto_skcipher *sk_tfm)
>>> +{
>>> +     struct crypto_tfm *tfm = crypto_skcipher_tfm(sk_tfm);
>>> +
>>> +     return crypto_tfm_alg_bulk(tfm);
>>> +}
>>> +
>>>  #endif       /* _CRYPTO_SKCIPHER_H */
>>>
>>> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
>>> index 6e28c89..a315487 100644
>>> --- a/include/linux/crypto.h
>>> +++ b/include/linux/crypto.h
>>> @@ -63,6 +63,7 @@
>>>  #define CRYPTO_ALG_DEAD                      0x00000020
>>>  #define CRYPTO_ALG_DYING             0x00000040
>>>  #define CRYPTO_ALG_ASYNC             0x00000080
>>> +#define CRYPTO_ALG_BULK                      0x00000100
>>>
>>>  /*
>>>   * Set this bit if and only if the algorithm requires another algorithm of
>>> @@ -623,6 +624,11 @@ static inline u32 crypto_tfm_alg_type(struct crypto_tfm *tfm)
>>>       return tfm->__crt_alg->cra_flags & CRYPTO_ALG_TYPE_MASK;
>>>  }
>>>
>>> +static inline unsigned int crypto_tfm_alg_bulk(struct crypto_tfm *tfm)
>>> +{
>>> +     return tfm->__crt_alg->cra_flags & CRYPTO_ALG_BULK;
>>> +}
>>> +
>>>  static inline unsigned int crypto_tfm_alg_blocksize(struct crypto_tfm *tfm)
>>>  {
>>>       return tfm->__crt_alg->cra_blocksize;
>>>
>>
> 
> 
>