Message-ID: <CY4PR04MB375123D338D4CB4FD586FE61E7950@CY4PR04MB3751.namprd04.prod.outlook.com>
Date:   Wed, 24 Jun 2020 07:49:38 +0000
From:   Damien Le Moal <Damien.LeMoal@....com>
To:     Eric Biggers <ebiggers@...nel.org>,
        Ignat Korchagin <ignat@...udflare.com>
CC:     "snitzer@...hat.com" <snitzer@...hat.com>,
        "kernel-team@...udflare.com" <kernel-team@...udflare.com>,
        "dm-crypt@...ut.de" <dm-crypt@...ut.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>,
        "agk@...hat.com" <agk@...hat.com>
Subject: Re: [dm-devel] [dm-crypt] [RFC PATCH 1/1] Add DM_CRYPT_FORCE_INLINE
 flag to dm-crypt target

On 2020/06/24 14:05, Eric Biggers wrote:
> On Fri, Jun 19, 2020 at 05:41:32PM +0100, Ignat Korchagin wrote:
>> Sometimes extra thread offloading imposed by dm-crypt hurts IO latency. This is
>> especially visible on busy systems with many processes/threads. Moreover, most
>> Crypto API implementaions are async, that is they offload crypto operations on
>> their own, so this dm-crypt offloading is excessive.
> 
> This really should say "some Crypto API implementations are async" instead of
> "most Crypto API implementations are async".
> 
> Notably, the AES-NI implementation of AES-XTS is synchronous if you call it in a
> context where SIMD instructions are usable.  It's only asynchronous when SIMD is
> not usable.  (This seems to have been missed in your blog post.)
> 
>> This adds a new flag, which directs dm-crypt not to offload crypto operations
>> and process everything inline. For cases, where crypto operations cannot happen
>> inline (hard interrupt context, for example the read path of the NVME driver),
>> we offload the work to a tasklet rather than a workqueue.
> 
> This patch both removes some dm-crypt specific queueing, and changes decryption
> to use softIRQ context instead of a workqueue.  It would be useful to know how
> much of a difference the workqueue => softIRQ change makes by itself.  Such a
> change could be useful for fscrypt as well.  (fscrypt uses a workqueue for
> decryption, but besides that doesn't use any other queueing.)
> 
>> @@ -127,7 +128,7 @@ struct iv_elephant_private {
>>   * and encrypts / decrypts at the same time.
>>   */
>>  enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
>> -	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD };
>> +	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD, DM_CRYPT_FORCE_INLINE = (sizeof(unsigned long) * 8 - 1) };
> 
> Assigning a specific enum value isn't necessary.
> 
>> @@ -1458,13 +1459,18 @@ static void crypt_alloc_req_skcipher(struct crypt_config *cc,
>>  
>>  	skcipher_request_set_tfm(ctx->r.req, cc->cipher_tfm.tfms[key_index]);
>>  
>> -	/*
>> -	 * Use REQ_MAY_BACKLOG so a cipher driver internally backlogs
>> -	 * requests if driver request queue is full.
>> -	 */
>> -	skcipher_request_set_callback(ctx->r.req,
>> -	    CRYPTO_TFM_REQ_MAY_BACKLOG,
>> -	    kcryptd_async_done, dmreq_of_req(cc, ctx->r.req));
>> +	if (test_bit(DM_CRYPT_FORCE_INLINE, &cc->flags))
>> +		/* make sure we zero important fields of the request */
>> +		skcipher_request_set_callback(ctx->r.req,
>> +	        0, NULL, NULL);
>> +	else
>> +		/*
>> +		 * Use REQ_MAY_BACKLOG so a cipher driver internally backlogs
>> +		 * requests if driver request queue is full.
>> +		 */
>> +		skcipher_request_set_callback(ctx->r.req,
>> +	        CRYPTO_TFM_REQ_MAY_BACKLOG,
>> +	        kcryptd_async_done, dmreq_of_req(cc, ctx->r.req));
>>  }
> 
> This looks wrong.  Unless type=0 and mask=CRYPTO_ALG_ASYNC are passed to
> crypto_alloc_skcipher(), the skcipher implementation can still be asynchronous,
> in which case providing a callback is required.

Another point: for a skcipher implementation that is asynchronous, for the
regular case/not-inline, can't we just issue the request directly without using
the workqueue ? If yes, that would save one context switch, since queueing of
the request can be handled by the crypto API when the request callback is set
with CRYPTO_TFM_REQ_MAY_BACKLOG. At least that is how I understood the
documentation & comments. I may be wrong here...

> 
> Do you intend that the "force_inline" option forces the use of a synchronous
> skcipher (alongside the other things it does)?  Or should it still allow
> asynchronous ones?
> 
> We may not actually have a choice in that matter, since xts-aes-aesni has the
> CRYPTO_ALG_ASYNC bit set (as I mentioned) despite being synchronous in most
> cases; thus, the crypto API won't give you it if you ask for a synchronous
> cipher.  So I think you still need to allow async skciphers?  That means a
> callback is still always required.
> 
> - Eric
> 
> --
> dm-devel mailing list
> dm-devel@...hat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 
> 


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists