[PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Ming Liu]

So far, the encryption/decryption are asynchronously processed in
softirq and cryptd which would result in a implicit order of data,
therefore leads IPSec stack also out of order while encapsulating
or decapsulating packets.

Consider the following scenario:

             DECRYPTION INBOUND
                      |
                      |
             +-----Packet A
             |        |
             |        |
             |     Packet B
             |        |
    (cryptd) |        | (software interrupts)
             |      ......
             |        |
             |        |
             |     Packet B(decrypted)
             |        |
             |        |
             +---> Packet A(decrypted)
                      |
                      |
             DECRYPTION OUTBOUND

Add cryptd_flush_queue function fixing it by being called from softirq
to flush all previous queued elements before processing a new one. To
prevent cryptd_flush_queue() being accessed from software interrupts,
local_bh_disable/enable needs to be relocated in several places.

Signed-off-by: Ming Liu <ming.liu@windriver.com>
---
I was told that I need resend this patch with git, so here it is:

I've figured out a new patch for this issue reported by me previously,
the basic idea is adding a cryptd_flush_queue function fixing it by
being called from softirq to flush all previous queued elements
before processing a new one, and it works very well so far per my test.

 crypto/ablk_helper.c    |  10 ++++-
 crypto/cryptd.c         | 107 ++++++++++++++++++++++++++++++++++++++++--------
 include/crypto/cryptd.h |  13 ++++++
 3 files changed, 111 insertions(+), 19 deletions(-)

diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
index ffe7278..600a70f 100644
--- a/crypto/ablk_helper.c
+++ b/crypto/ablk_helper.c
@@ -70,16 +70,19 @@ int ablk_encrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
 	if (!may_use_simd()) {
 		struct ablkcipher_request *cryptd_req =
 			ablkcipher_request_ctx(req);
 
 		*cryptd_req = *req;
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
+		cryptd_req->base.tfm = req_tfm;
 
 		return crypto_ablkcipher_encrypt(cryptd_req);
 	} else {
+		cryptd_flush_queue(req_tfm, CRYPTD_ENCRYPT);
 		return __ablk_encrypt(req);
 	}
 }
@@ -89,13 +92,15 @@ int ablk_decrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
 	if (!may_use_simd()) {
 		struct ablkcipher_request *cryptd_req =
 			ablkcipher_request_ctx(req);
 
 		*cryptd_req = *req;
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
+		cryptd_req->base.tfm = req_tfm;
 
 		return crypto_ablkcipher_decrypt(cryptd_req);
 	} else {
@@ -105,6 +110,7 @@ int ablk_decrypt(struct ablkcipher_request *req)
 		desc.info = req->info;
 		desc.flags = 0;
 
+		cryptd_flush_queue(req_tfm, CRYPTD_DECRYPT);
 		return crypto_blkcipher_crt(desc.tfm)->decrypt(
 			&desc, req->dst, req->src, req->nbytes);
 	}
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index e592c90..0b387a1 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -119,11 +119,13 @@ static int cryptd_enqueue_request(struct cryptd_queue *queue,
 	int cpu, err;
 	struct cryptd_cpu_queue *cpu_queue;
 
+	local_bh_disable();
 	cpu = get_cpu();
 	cpu_queue = this_cpu_ptr(queue->cpu_queue);
 	err = crypto_enqueue_request(&cpu_queue->queue, request);
 	queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);
 	put_cpu();
+	local_bh_enable();
 
 	return err;
 }
@@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct *work)
 	preempt_disable();
 	backlog = crypto_get_backlog(&cpu_queue->queue);
 	req = crypto_dequeue_request(&cpu_queue->queue);
-	preempt_enable();
-	local_bh_enable();
 
 	if (!req)
-		return;
+		goto out;
 
 	if (backlog)
 		backlog->complete(backlog, -EINPROGRESS);
@@ -159,6 +159,9 @@ static void cryptd_queue_worker(struct work_struct *work)
 
 	if (cpu_queue->queue.qlen)
 		queue_work(kcrypto_wq, &cpu_queue->work);
+out:
+	preempt_enable();
+	local_bh_enable();
 }
 
 static inline struct cryptd_queue *cryptd_get_queue(struct crypto_tfm *tfm)
@@ -209,9 +212,7 @@ static void cryptd_blkcipher_crypt(struct ablkcipher_request *req,
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static void cryptd_blkcipher_encrypt(struct crypto_async_request *req, int err)
@@ -446,9 +447,7 @@ static void cryptd_hash_init(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_init_enqueue(struct ahash_request *req)
@@ -471,9 +470,7 @@ static void cryptd_hash_update(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_update_enqueue(struct ahash_request *req)
@@ -494,9 +491,7 @@ static void cryptd_hash_final(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_final_enqueue(struct ahash_request *req)
@@ -517,9 +512,7 @@ static void cryptd_hash_finup(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_finup_enqueue(struct ahash_request *req)
@@ -546,9 +539,7 @@ static void cryptd_hash_digest(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_digest_enqueue(struct ahash_request *req)
@@ -641,9 +632,7 @@ static void cryptd_aead_crypt(struct aead_request *req,
 	err = crypt( req );
 	req->base.complete = rctx->complete;
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static void cryptd_aead_encrypt(struct crypto_async_request *areq, int err)
@@ -895,6 +884,90 @@ void cryptd_free_ahash(struct cryptd_ahash *tfm)
 }
 EXPORT_SYMBOL_GPL(cryptd_free_ahash);
 
+void cryptd_flush_queue(struct crypto_tfm *tfm, cryptd_type_t type)
+{
+	struct crypto_instance *inst = crypto_tfm_alg_instance(tfm);
+	struct cryptd_instance_ctx *ictx = crypto_instance_ctx(inst);
+	struct cryptd_queue *cryptd_queue = ictx->queue;
+	struct cryptd_cpu_queue *cpu_queue;
+	struct crypto_queue *queue;
+	struct crypto_async_request *req, *tmp, *backlog;
+	crypto_completion_t complete;
+	int cpu;
+	unsigned int len;
+
+	switch (type) {
+	case CRYPTD_BLKCIPHER_ENCRYPT:
+		complete = cryptd_blkcipher_encrypt;
+		break;
+	case CRYPTD_BLKCIPHER_DECRYPT:
+		complete = cryptd_blkcipher_decrypt;
+		break;
+	case CRYPTD_HASH_INIT:
+		complete = cryptd_hash_init;
+		break;
+	case CRYPTD_HASH_UPDATE:
+		complete = cryptd_hash_update;
+		break;
+	case CRYPTD_HASH_FINAL:
+		complete = cryptd_hash_final;
+		break;
+	case CRYPTD_HASH_FINUP:
+		complete = cryptd_hash_finup;
+		break;
+	case CRYPTD_HASH_DIGEST:
+		complete = cryptd_hash_digest;
+		break;
+	case CRYPTD_AEAD_ENCRYPT:
+		complete = cryptd_aead_encrypt;
+		break;
+	case CRYPTD_AEAD_DECRYPT:
+		complete = cryptd_aead_decrypt;
+		break;
+	default:
+		complete = NULL;
+	}
+
+	if (complete == NULL)
+		return;
+
+	local_bh_disable();
+	cpu = get_cpu();
+	cpu_queue = this_cpu_ptr(cryptd_queue->cpu_queue);
+	queue = &cpu_queue->queue;
+	len = queue->qlen;
+
+	if (!len)
+		goto out;
+
+	list_for_each_entry_safe(req, tmp, &queue->list, list) {
+		if (req->complete == complete) {
+			queue->qlen--;
+
+			if (queue->backlog != &queue->list) {
+				backlog = container_of(queue->backlog,
+					struct crypto_async_request, list);
+				queue->backlog = queue->backlog->next;
+			} else
+				backlog = NULL;
+
+			list_del(&req->list);
+
+			if (backlog)
+				backlog->complete(backlog, -EINPROGRESS);
+			req->complete(req, 0);
+		}
+
+		if (--len == 0)
+			goto out;
+	}
+
+out:
+	put_cpu();
+	local_bh_enable();
+}
+EXPORT_SYMBOL_GPL(cryptd_flush_queue);
+
 struct cryptd_aead *cryptd_alloc_aead(const char *alg_name,
 						  u32 type, u32 mask)
 {
diff --git a/include/crypto/cryptd.h b/include/crypto/cryptd.h
index ba98918..a63a296 100644
--- a/include/crypto/cryptd.h
+++ b/include/crypto/cryptd.h
@@ -16,6 +16,18 @@
 #include <linux/kernel.h>
 #include <crypto/hash.h>
 
+typedef enum {
+	CRYPTD_BLKCIPHER_ENCRYPT,
+	CRYPTD_BLKCIPHER_DECRYPT,
+	CRYPTD_HASH_INIT,
+	CRYPTD_HASH_UPDATE,
+	CRYPTD_HASH_FINAL,
+	CRYPTD_HASH_FINUP,
+	CRYPTD_HASH_DIGEST,
+	CRYPTD_AEAD_ENCRYPT,
+	CRYPTD_AEAD_DECRYPT,
+} cryptd_type_t;
+
 struct cryptd_ablkcipher {
 	struct crypto_ablkcipher base;
 };
@@ -48,6 +60,7 @@ struct cryptd_ahash *cryptd_alloc_ahash(const char *alg_name,
 struct crypto_shash *cryptd_ahash_child(struct cryptd_ahash *tfm);
 struct shash_desc *cryptd_shash_desc(struct ahash_request *req);
 void cryptd_free_ahash(struct cryptd_ahash *tfm);
+void cryptd_flush_queue(struct crypto_tfm *tfm, cryptd_type_t type);
 
 struct cryptd_aead {
 	struct crypto_aead base;
-- 
1.8.4.1

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 01:49:31PM +0800, Ming Liu wrote:
> So far, the encryption/decryption are asynchronously processed in
> softirq and cryptd which would result in a implicit order of data,
> therefore leads IPSec stack also out of order while encapsulating
> or decapsulating packets.
> 
> Consider the following scenario:
> 
>              DECRYPTION INBOUND
>                       |
>                       |
>              +-----Packet A
>              |        |
>              |        |
>              |     Packet B
>              |        |
>     (cryptd) |        | (software interrupts)
>              |      ......
>              |        |
>              |        |
>              |     Packet B(decrypted)
>              |        |
>              |        |
>              +---> Packet A(decrypted)
>                       |
>                       |
>              DECRYPTION OUTBOUND
> 
> Add cryptd_flush_queue function fixing it by being called from softirq
> to flush all previous queued elements before processing a new one. To
> prevent cryptd_flush_queue() being accessed from software interrupts,
> local_bh_disable/enable needs to be relocated in several places.
> 
> Signed-off-by: Ming Liu <ming.liu@windriver.com>
> ---
> I was told that I need resend this patch with git, so here it is:
> 
> I've figured out a new patch for this issue reported by me previously,
> the basic idea is adding a cryptd_flush_queue function fixing it by
> being called from softirq to flush all previous queued elements
> before processing a new one, and it works very well so far per my test.

I doubt that this approach can work. The cryptd encrypt/decrypt functions
assume to be called from a workqueue worker, they can't be called from
atomic context.

Can't we just use cryptd unconditionally to fix this reordering problem?

> 
>  crypto/ablk_helper.c    |  10 ++++-
>  crypto/cryptd.c         | 107 ++++++++++++++++++++++++++++++++++++++++--------
>  include/crypto/cryptd.h |  13 ++++++
>  3 files changed, 111 insertions(+), 19 deletions(-)
> 
> diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
> index ffe7278..600a70f 100644
> --- a/crypto/ablk_helper.c
> +++ b/crypto/ablk_helper.c
> @@ -70,16 +70,19 @@ int ablk_encrypt(struct ablkcipher_request *req)
>  {
>  	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
>  	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
> +	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
> +		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
>  
>  	if (!may_use_simd()) {
>  		struct ablkcipher_request *cryptd_req =
>  			ablkcipher_request_ctx(req);
>  
>  		*cryptd_req = *req;
> -		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
> +		cryptd_req->base.tfm = req_tfm;
>  
>  		return crypto_ablkcipher_encrypt(cryptd_req);
>  	} else {
> +		cryptd_flush_queue(req_tfm, CRYPTD_ENCRYPT);

Where is CRYPTD_ENCRYPT defined?
This does not even compile when applied to the cryptodev tree.

>  		return __ablk_encrypt(req);
>  	}
>  }
> @@ -89,13 +92,15 @@ int ablk_decrypt(struct ablkcipher_request *req)
>  {
>  	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
>  	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
> +	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
> +		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
>  
>  	if (!may_use_simd()) {
>  		struct ablkcipher_request *cryptd_req =
>  			ablkcipher_request_ctx(req);
>  
>  		*cryptd_req = *req;
> -		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
> +		cryptd_req->base.tfm = req_tfm;
>  
>  		return crypto_ablkcipher_decrypt(cryptd_req);
>  	} else {
> @@ -105,6 +110,7 @@ int ablk_decrypt(struct ablkcipher_request *req)
>  		desc.info = req->info;
>  		desc.flags = 0;
>  
> +		cryptd_flush_queue(req_tfm, CRYPTD_DECRYPT);

Same here.

>  		return crypto_blkcipher_crt(desc.tfm)->decrypt(
>  			&desc, req->dst, req->src, req->nbytes);
>  	}
> diff --git a/crypto/cryptd.c b/crypto/cryptd.c
> index e592c90..0b387a1 100644
> --- a/crypto/cryptd.c
> +++ b/crypto/cryptd.c
> @@ -119,11 +119,13 @@ static int cryptd_enqueue_request(struct cryptd_queue *queue,
>  	int cpu, err;
>  	struct cryptd_cpu_queue *cpu_queue;
>  
> +	local_bh_disable();
>  	cpu = get_cpu();
>  	cpu_queue = this_cpu_ptr(queue->cpu_queue);
>  	err = crypto_enqueue_request(&cpu_queue->queue, request);
>  	queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);
>  	put_cpu();
> +	local_bh_enable();
>  
>  	return err;
>  }
> @@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct *work)
>  	preempt_disable();
>  	backlog = crypto_get_backlog(&cpu_queue->queue);
>  	req = crypto_dequeue_request(&cpu_queue->queue);
> -	preempt_enable();
> -	local_bh_enable();

Everything below the local_bh_enable() should not run in atomic context
as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.

>  
>  	if (!req)
> -		return;
> +		goto out;
>  
>  	if (backlog)
>  		backlog->complete(backlog, -EINPROGRESS);
> @@ -159,6 +159,9 @@ static void cryptd_queue_worker(struct work_struct *work)
>  
>  	if (cpu_queue->queue.qlen)
>  		queue_work(kcrypto_wq, &cpu_queue->work);
> +out:
> +	preempt_enable();
> +	local_bh_enable();
>  }

...

>  
> +void cryptd_flush_queue(struct crypto_tfm *tfm, cryptd_type_t type)
> +{
> +	struct crypto_instance *inst = crypto_tfm_alg_instance(tfm);
> +	struct cryptd_instance_ctx *ictx = crypto_instance_ctx(inst);
> +	struct cryptd_queue *cryptd_queue = ictx->queue;
> +	struct cryptd_cpu_queue *cpu_queue;
> +	struct crypto_queue *queue;
> +	struct crypto_async_request *req, *tmp, *backlog;
> +	crypto_completion_t complete;
> +	int cpu;
> +	unsigned int len;
> +
> +	switch (type) {
> +	case CRYPTD_BLKCIPHER_ENCRYPT:
> +		complete = cryptd_blkcipher_encrypt;
> +		break;
> +	case CRYPTD_BLKCIPHER_DECRYPT:
> +		complete = cryptd_blkcipher_decrypt;
> +		break;
> +	case CRYPTD_HASH_INIT:
> +		complete = cryptd_hash_init;
> +		break;
> +	case CRYPTD_HASH_UPDATE:
> +		complete = cryptd_hash_update;
> +		break;
> +	case CRYPTD_HASH_FINAL:
> +		complete = cryptd_hash_final;
> +		break;
> +	case CRYPTD_HASH_FINUP:
> +		complete = cryptd_hash_finup;
> +		break;
> +	case CRYPTD_HASH_DIGEST:
> +		complete = cryptd_hash_digest;
> +		break;
> +	case CRYPTD_AEAD_ENCRYPT:
> +		complete = cryptd_aead_encrypt;
> +		break;
> +	case CRYPTD_AEAD_DECRYPT:
> +		complete = cryptd_aead_decrypt;
> +		break;
> +	default:
> +		complete = NULL;
> +	}
> +
> +	if (complete == NULL)
> +		return;
> +
> +	local_bh_disable();
> +	cpu = get_cpu();
> +	cpu_queue = this_cpu_ptr(cryptd_queue->cpu_queue);
> +	queue = &cpu_queue->queue;
> +	len = queue->qlen;
> +
> +	if (!len)
> +		goto out;
> +
> +	list_for_each_entry_safe(req, tmp, &queue->list, list) {
> +		if (req->complete == complete) {
> +			queue->qlen--;
> +
> +			if (queue->backlog != &queue->list) {
> +				backlog = container_of(queue->backlog,
> +					struct crypto_async_request, list);
> +				queue->backlog = queue->backlog->next;
> +			} else
> +				backlog = NULL;
> +
> +			list_del(&req->list);
> +
> +			if (backlog)
> +				backlog->complete(backlog, -EINPROGRESS);
> +			req->complete(req, 0);

Same here, the complete function can not run in atomic context.

Also, this can not ensure that the request is finalized.
Subsequent algorithms may run asynchronously too, so this
does not fix the reordering problem completely.

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Herbert Xu]

On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
>
> Can't we just use cryptd unconditionally to fix this reordering problem?

I think the idea is that most of the time cryptd isn't required
so we want to stick with direct processing to lower latency.

I think the simplest fix would be to punt to cryptd as long as
there are cryptd requests queued.

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: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 04:51:48PM +0800, Herbert Xu wrote:
> On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
> >
> > Can't we just use cryptd unconditionally to fix this reordering problem?
> 
> I think the idea is that most of the time cryptd isn't required
> so we want to stick with direct processing to lower latency.

Yes, I thought that. But is it really the case that doing it
asynchronous increases the latency? I tried this some time
ago and as far as I remember there was not too much difference
between the direct and the asynchronous case. Might depend
on the usecase of course.

> 
> I think the simplest fix would be to punt to cryptd as long as
> there are cryptd requests queued.

This would be the second option. We may need to adjust the
maximum cryptd queue lenght then, as the networking receive
softirq might enqueue a lot of packets before cryptd can
process them.

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Ming Liu]

On 11/12/2014 04:51 PM, Herbert Xu wrote:
> On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
>> Can't we just use cryptd unconditionally to fix this reordering problem?
> I think the idea is that most of the time cryptd isn't required
> so we want to stick with direct processing to lower latency.
>
> I think the simplest fix would be to punt to cryptd as long as
> there are cryptd requests queued.
I've tried that method when I started to think about the fix, but it 
will cause 2 other issues per test while resolving the reordering one, 
as follows:
1 The work queue can not handle so many packets when the traffic is very 
high(over 200M/S), and it would drop most of them when the queue length 
is beyond CRYPTD_MAX_CPU_QLEN.
2 Soft lockups are observed, per my analysis, the cause is: in some 
extreme cases, all packets would be sent to the cryptd once there are 
requests already in it, this will let the net poll functions return 
pretty fast, and lead too many hw interrupts triggered in a certain period.

//Ming Liu
>
> Cheers,

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 06:41:30PM +0800, Ming Liu wrote:
> On 11/12/2014 04:51 PM, Herbert Xu wrote:
> >On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
> >>Can't we just use cryptd unconditionally to fix this reordering problem?
> >I think the idea is that most of the time cryptd isn't required
> >so we want to stick with direct processing to lower latency.
> >
> >I think the simplest fix would be to punt to cryptd as long as
> >there are cryptd requests queued.
> I've tried that method when I started to think about the fix, but it
> will cause 2 other issues per test while resolving the reordering
> one, as follows:
> 1 The work queue can not handle so many packets when the traffic is
> very high(over 200M/S), and it would drop most of them when the
> queue length is beyond CRYPTD_MAX_CPU_QLEN.

That's why I've proposed to adjust CRYPTD_MAX_CPU_QLEN in my other mail.
But anyway, it still does not fix the reorder problem completely.
We still have a problem if subsequent algorithms run asynchronously
or if we get interrupted while we are processing the last request
from the queue.

I think we have only two options, either processing all calls
directly or use cryptd unconditionally. Mixing direct and
asynchronous calls will lead to problems.

If we don't want to use cryptd unconditionally, we could use
direct calls for all requests. If the fpu is not usable, we
maybe could fallback to an algorithm that does not need the
fpu, such as aes-generic.

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Ming Liu]

On 11/12/2014 07:43 PM, Steffen Klassert wrote:
> On Wed, Nov 12, 2014 at 06:41:30PM +0800, Ming Liu wrote:
>> On 11/12/2014 04:51 PM, Herbert Xu wrote:
>>> On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
>>>> Can't we just use cryptd unconditionally to fix this reordering problem?
>>> I think the idea is that most of the time cryptd isn't required
>>> so we want to stick with direct processing to lower latency.
>>>
>>> I think the simplest fix would be to punt to cryptd as long as
>>> there are cryptd requests queued.
>> I've tried that method when I started to think about the fix, but it
>> will cause 2 other issues per test while resolving the reordering
>> one, as follows:
>> 1 The work queue can not handle so many packets when the traffic is
>> very high(over 200M/S), and it would drop most of them when the
>> queue length is beyond CRYPTD_MAX_CPU_QLEN.
> That's why I've proposed to adjust CRYPTD_MAX_CPU_QLEN in my other mail.
> But anyway, it still does not fix the reorder problem completely.
> We still have a problem if subsequent algorithms run asynchronously
> or if we get interrupted while we are processing the last request
> from the queue.
>
> I think we have only two options, either processing all calls
> directly or use cryptd unconditionally. Mixing direct and
> asynchronous calls will lead to problems.
>
> If we don't want to use cryptd unconditionally, we could use
> direct calls for all requests. If the fpu is not usable, we
> maybe could fallback to an algorithm that does not need the
> fpu, such as aes-generic.
Yes, this is a good idea, I will try to work on it based on your 
suggestion. Thanks!

the best,
thank you
>
>
>

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Ming Liu]

On 11/12/2014 04:41 PM, Steffen Klassert wrote:
> On Wed, Nov 12, 2014 at 01:49:31PM +0800, Ming Liu wrote:
>> So far, the encryption/decryption are asynchronously processed in
>> softirq and cryptd which would result in a implicit order of data,
>> therefore leads IPSec stack also out of order while encapsulating
>> or decapsulating packets.
>>
>> Consider the following scenario:
>>
>>               DECRYPTION INBOUND
>>                        |
>>                        |
>>               +-----Packet A
>>               |        |
>>               |        |
>>               |     Packet B
>>               |        |
>>      (cryptd) |        | (software interrupts)
>>               |      ......
>>               |        |
>>               |        |
>>               |     Packet B(decrypted)
>>               |        |
>>               |        |
>>               +---> Packet A(decrypted)
>>                        |
>>                        |
>>               DECRYPTION OUTBOUND
>>
>> Add cryptd_flush_queue function fixing it by being called from softirq
>> to flush all previous queued elements before processing a new one. To
>> prevent cryptd_flush_queue() being accessed from software interrupts,
>> local_bh_disable/enable needs to be relocated in several places.
>>
>> Signed-off-by: Ming Liu <ming.liu@windriver.com>
>> ---
>> I was told that I need resend this patch with git, so here it is:
>>
>> I've figured out a new patch for this issue reported by me previously,
>> the basic idea is adding a cryptd_flush_queue function fixing it by
>> being called from softirq to flush all previous queued elements
>> before processing a new one, and it works very well so far per my test.
> I doubt that this approach can work. The cryptd encrypt/decrypt functions
> assume to be called from a workqueue worker, they can't be called from
> atomic context.
Yes, you are correct, I am on board, I need rework the patch.

>
> Can't we just use cryptd unconditionally to fix this reordering problem?
>>   crypto/ablk_helper.c    |  10 ++++-
>>   crypto/cryptd.c         | 107 ++++++++++++++++++++++++++++++++++++++++--------
>>   include/crypto/cryptd.h |  13 ++++++
>>   3 files changed, 111 insertions(+), 19 deletions(-)
>>
>> diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
>> index ffe7278..600a70f 100644
>> --- a/crypto/ablk_helper.c
>> +++ b/crypto/ablk_helper.c
>> @@ -70,16 +70,19 @@ int ablk_encrypt(struct ablkcipher_request *req)
>>   {
>>   	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
>>   	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
>> +	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
>> +		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
>>   
>>   	if (!may_use_simd()) {
>>   		struct ablkcipher_request *cryptd_req =
>>   			ablkcipher_request_ctx(req);
>>   
>>   		*cryptd_req = *req;
>> -		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
>> +		cryptd_req->base.tfm = req_tfm;
>>   
>>   		return crypto_ablkcipher_encrypt(cryptd_req);
>>   	} else {
>> +		cryptd_flush_queue(req_tfm, CRYPTD_ENCRYPT);
> Where is CRYPTD_ENCRYPT defined?
> This does not even compile when applied to the cryptodev tree.
Sorry, it really should be CRYPTD_BLKCIPHER_ENCRYPT, I made the original 
patch against 3.4 kernel, there must be something wrong when I adjusted 
it to mainline kernel, sorry for that.
>
>>   		return __ablk_encrypt(req);
>>   	}
>>   }
>> @@ -89,13 +92,15 @@ int ablk_decrypt(struct ablkcipher_request *req)
>>   {
>>   	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
>>   	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
>> +	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
>> +		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
>>   
>>   	if (!may_use_simd()) {
>>   		struct ablkcipher_request *cryptd_req =
>>   			ablkcipher_request_ctx(req);
>>   
>>   		*cryptd_req = *req;
>> -		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
>> +		cryptd_req->base.tfm = req_tfm;
>>   
>>   		return crypto_ablkcipher_decrypt(cryptd_req);
>>   	} else {
>> @@ -105,6 +110,7 @@ int ablk_decrypt(struct ablkcipher_request *req)
>>   		desc.info = req->info;
>>   		desc.flags = 0;
>>   
>> +		cryptd_flush_queue(req_tfm, CRYPTD_DECRYPT);
> Same here.
Should be CRYPTD_BLKCIPHER_DECRYPT.

>
>>   		return crypto_blkcipher_crt(desc.tfm)->decrypt(
>>   			&desc, req->dst, req->src, req->nbytes);
>>   	}
>> diff --git a/crypto/cryptd.c b/crypto/cryptd.c
>> index e592c90..0b387a1 100644
>> --- a/crypto/cryptd.c
>> +++ b/crypto/cryptd.c
>> @@ -119,11 +119,13 @@ static int cryptd_enqueue_request(struct cryptd_queue *queue,
>>   	int cpu, err;
>>   	struct cryptd_cpu_queue *cpu_queue;
>>   
>> +	local_bh_disable();
>>   	cpu = get_cpu();
>>   	cpu_queue = this_cpu_ptr(queue->cpu_queue);
>>   	err = crypto_enqueue_request(&cpu_queue->queue, request);
>>   	queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);
>>   	put_cpu();
>> +	local_bh_enable();
>>   
>>   	return err;
>>   }
>> @@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct *work)
>>   	preempt_disable();
>>   	backlog = crypto_get_backlog(&cpu_queue->queue);
>>   	req = crypto_dequeue_request(&cpu_queue->queue);
>> -	preempt_enable();
>> -	local_bh_enable();
> Everything below the local_bh_enable() should not run in atomic context
> as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.
If I turn off all the CRYPTO_TFM_REQ_MAY_SLEEP in cryptd.c, is that 
going to work?

>
>>   
>>   	if (!req)
>> -		return;
>> +		goto out;
>>   
>>   	if (backlog)
>>   		backlog->complete(backlog, -EINPROGRESS);
>> @@ -159,6 +159,9 @@ static void cryptd_queue_worker(struct work_struct *work)
>>   
>>   	if (cpu_queue->queue.qlen)
>>   		queue_work(kcrypto_wq, &cpu_queue->work);
>> +out:
>> +	preempt_enable();
>> +	local_bh_enable();
>>   }
> ...
>
>>   
>> +void cryptd_flush_queue(struct crypto_tfm *tfm, cryptd_type_t type)
>> +{
>> +	struct crypto_instance *inst = crypto_tfm_alg_instance(tfm);
>> +	struct cryptd_instance_ctx *ictx = crypto_instance_ctx(inst);
>> +	struct cryptd_queue *cryptd_queue = ictx->queue;
>> +	struct cryptd_cpu_queue *cpu_queue;
>> +	struct crypto_queue *queue;
>> +	struct crypto_async_request *req, *tmp, *backlog;
>> +	crypto_completion_t complete;
>> +	int cpu;
>> +	unsigned int len;
>> +
>> +	switch (type) {
>> +	case CRYPTD_BLKCIPHER_ENCRYPT:
>> +		complete = cryptd_blkcipher_encrypt;
>> +		break;
>> +	case CRYPTD_BLKCIPHER_DECRYPT:
>> +		complete = cryptd_blkcipher_decrypt;
>> +		break;
>> +	case CRYPTD_HASH_INIT:
>> +		complete = cryptd_hash_init;
>> +		break;
>> +	case CRYPTD_HASH_UPDATE:
>> +		complete = cryptd_hash_update;
>> +		break;
>> +	case CRYPTD_HASH_FINAL:
>> +		complete = cryptd_hash_final;
>> +		break;
>> +	case CRYPTD_HASH_FINUP:
>> +		complete = cryptd_hash_finup;
>> +		break;
>> +	case CRYPTD_HASH_DIGEST:
>> +		complete = cryptd_hash_digest;
>> +		break;
>> +	case CRYPTD_AEAD_ENCRYPT:
>> +		complete = cryptd_aead_encrypt;
>> +		break;
>> +	case CRYPTD_AEAD_DECRYPT:
>> +		complete = cryptd_aead_decrypt;
>> +		break;
>> +	default:
>> +		complete = NULL;
>> +	}
>> +
>> +	if (complete == NULL)
>> +		return;
>> +
>> +	local_bh_disable();
>> +	cpu = get_cpu();
>> +	cpu_queue = this_cpu_ptr(cryptd_queue->cpu_queue);
>> +	queue = &cpu_queue->queue;
>> +	len = queue->qlen;
>> +
>> +	if (!len)
>> +		goto out;
>> +
>> +	list_for_each_entry_safe(req, tmp, &queue->list, list) {
>> +		if (req->complete == complete) {
>> +			queue->qlen--;
>> +
>> +			if (queue->backlog != &queue->list) {
>> +				backlog = container_of(queue->backlog,
>> +					struct crypto_async_request, list);
>> +				queue->backlog = queue->backlog->next;
>> +			} else
>> +				backlog = NULL;
>> +
>> +			list_del(&req->list);
>> +
>> +			if (backlog)
>> +				backlog->complete(backlog, -EINPROGRESS);
>> +			req->complete(req, 0);
> Same here, the complete function can not run in atomic context.
> Also, this can not ensure that the request is finalized.
> Subsequent algorithms may run asynchronously too, so this
> does not fix the reordering problem completely.
I do not know that before. Then I need rework the patch.

//Ming Liu
>
>
>

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 06:41:28PM +0800, Ming Liu wrote:
> On 11/12/2014 04:41 PM, Steffen Klassert wrote:
> >On Wed, Nov 12, 2014 at 01:49:31PM +0800, Ming Liu wrote:
> >>  }
> >>@@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct *work)
> >>  	preempt_disable();
> >>  	backlog = crypto_get_backlog(&cpu_queue->queue);
> >>  	req = crypto_dequeue_request(&cpu_queue->queue);
> >>-	preempt_enable();
> >>-	local_bh_enable();
> >Everything below the local_bh_enable() should not run in atomic context
> >as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.
> If I turn off all the CRYPTO_TFM_REQ_MAY_SLEEP in cryptd.c, is that
> going to work?

Well, this might make the cryptd function accessible in atomic context,
but it does not solve the other problems with this approach. Also,
cryptd can be used to move requests out of atomic context and I think
it should stay as it is.

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Ming Liu]

On 11/12/2014 07:48 PM, Steffen Klassert wrote:
> On Wed, Nov 12, 2014 at 06:41:28PM +0800, Ming Liu wrote:
>> On 11/12/2014 04:41 PM, Steffen Klassert wrote:
>>> On Wed, Nov 12, 2014 at 01:49:31PM +0800, Ming Liu wrote:
>>>>   }
>>>> @@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct *work)
>>>>   	preempt_disable();
>>>>   	backlog = crypto_get_backlog(&cpu_queue->queue);
>>>>   	req = crypto_dequeue_request(&cpu_queue->queue);
>>>> -	preempt_enable();
>>>> -	local_bh_enable();
>>> Everything below the local_bh_enable() should not run in atomic context
>>> as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.
>> If I turn off all the CRYPTO_TFM_REQ_MAY_SLEEP in cryptd.c, is that
>> going to work?
> Well, this might make the cryptd function accessible in atomic context,
> but it does not solve the other problems with this approach. Also,
> cryptd can be used to move requests out of atomic context and I think
> it should stay as it is.

OK, got it. Thanks for the information.

the best,
thank you
>
>

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Herbert Xu]

On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
>
> Everything below the local_bh_enable() should not run in atomic context
> as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.

Actually I'm thinking of doing exactly that (disabling softirq in
cryptd) to fix the reordering problem.

Most threads do not use the FPU/SIMD so cryptd is only ever needed
when we have a user-space app that touches the FPU/SIMD which then
gets an interrupt to perform crypto in softirq.  So forcing cryptd
on everyone just because some apps touch the FPU/SIMD is a non-
starter.

The most straightforward solution is to always defer to cryptd once
it gets started.  This is bad because if a rarely used app that
touches FPU/SIMD runs then we'll end up stuck in cryptd long after
the app goes away.

So what I'm thinking of is to have the softirq path forcibly regain
control from cryptd where possible.  This is tricky because cryptd
might be in the middle of processing a request.  So that's why I'd
like to disable softirqs while we're processing a request.

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: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Sat, Nov 15, 2014 at 11:15:50AM +0800, Herbert Xu wrote:
> On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
> >
> > Everything below the local_bh_enable() should not run in atomic context
> > as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.
> 
> Actually I'm thinking of doing exactly that (disabling softirq in
> cryptd) to fix the reordering problem.
> 
> Most threads do not use the FPU/SIMD so cryptd is only ever needed
> when we have a user-space app that touches the FPU/SIMD which then
> gets an interrupt to perform crypto in softirq.  So forcing cryptd
> on everyone just because some apps touch the FPU/SIMD is a non-
> starter.
> 
> The most straightforward solution is to always defer to cryptd once
> it gets started.  This is bad because if a rarely used app that
> touches FPU/SIMD runs then we'll end up stuck in cryptd long after
> the app goes away.
> 
> So what I'm thinking of is to have the softirq path forcibly regain
> control from cryptd where possible.  This is tricky because cryptd
> might be in the middle of processing a request.  So that's why I'd
> like to disable softirqs while we're processing a request.
> 

What about to use a fallback algorithm that does not need to touch
FPU/SIMD in such cases? We would not need cryptd at all and it would
keep the requests in the right order because we don't defer them.

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Herbert Xu]

On Thu, Nov 20, 2014 at 08:26:51AM +0100, Steffen Klassert wrote:
>
> What about to use a fallback algorithm that does not need to touch
> FPU/SIMD in such cases? We would not need cryptd at all and it would
> keep the requests in the right order because we don't defer them.

This would be bad for throughput since the fallback is many orders
of magnitude slower than aesni.

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: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Thu, Nov 20, 2014 at 03:43:42PM +0800, Herbert Xu wrote:
> On Thu, Nov 20, 2014 at 08:26:51AM +0100, Steffen Klassert wrote:
> >
> > What about to use a fallback algorithm that does not need to touch
> > FPU/SIMD in such cases? We would not need cryptd at all and it would
> > keep the requests in the right order because we don't defer them.
> 
> This would be bad for throughput since the fallback is many orders
> of magnitude slower than aesni.

Sure, but could be an option if this is really a rare case.

Anyway, I don't mind too much about the solution as long as we
get it to work :)

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Herbert Xu]

On Thu, Nov 20, 2014 at 08:59:44AM +0100, Steffen Klassert wrote:
>
> Sure, but could be an option if this is really a rare case.

Well it's rare but when it does hit it'll probably be there all
the time for that system.  IOW you either have no apps using the
FPU, but when you do, it's probably going to be hogging it.

> Anyway, I don't mind too much about the solution as long as we
> get it to work :)

:)

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: aesni-intel - avoid IPsec re-ordering

[Raj Ammanur]

Hi

I just signed up on this list a few days ago and couldn't find a way to do a
reply on the original thread. Apologies in advance, if this ends up creating
a new thread, I just created a new mail with same subject and last message
on this thread as a reply.

First, I would like to report that we are also seeing problem where IPSec
packets are getting queued up to the workqueue for async processing because
of the FPU not being available. Since there are also a lot of input pkts, by the
time xfrm_input() is invoked again after the async operation is completed, the
IPsec pkts are either out of sequence or out of the replay window, since the
replay window has advanced. We are using IPSec tunnel between two
switches connected over a Long Fat Network and have sender and receiver
servers connected to the two ends of the tunnel. Because of  the TCP
receiver receiving  pkts either out of order or not receiving pkts because of
dropped pkts, this is causing significant drop in TCP throughtput on Long Fat
Networks, where  the network latency is high.

I have a few questions.

1. I see that patch was submitted and modifications suggested. Since then, I
don't see an update on the thread in the archives. Is there a latest
patch available
for the problem reported originally on this issue ? If so, I would
like to try it.

2. When the pkts are put on the async work queue, the queue size is initialized
as 100 in cryptd_init_queue() in crypto/cryptd.c. Does anyone know how/why
100 was picked ?

3. In cryptd_queue_worker(), the backlog->complete() callback is invoked with
-EINPROGRESS. In net/ipv4/esp4.c, there is no handling for EINPROGRESS
in esp_input_done()/esp_input_done2(). Am I reading the code correctly and
is there any expectation that ESP should support backlog and have backlog
handling ? Currently, the pkt just gets dropped as the EINPROGRESS is treated
as nexthdr and hence is invalid.

Thanks in advance,
--Raj



> Herbert Xu <herbert <at> gondor.apana.org.au> writes:
>
> >
> > On Thu, Nov 20, 2014 at 08:59:44AM +0100, Steffen Klassert wrote:
> > >
> > > Sure, but could be an option if this is really a rare case.
> >
> > Well it's rare but when it does hit it'll probably be there all
> > the time for that system.  IOW you either have no apps using the
> > FPU, but when you do, it's probably going to be hogging it.
> >
> > > Anyway, I don't mind too much about the solution as long as we
> > > get it to work :)
> >
> > :)
> >
> > Cheers,
>
>
> Hi Herbert
>
> Is there an official "blessed" patch for this re-ordering problem?
> I saw some issues raised in the thread with the patch that Ming Liu had
> provided?
>
> Thanks
> -sunderam
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Herbert Xu]

Raj Ammanur <rammanur@arista.com> wrote:
>
> First, I would like to report that we are also seeing problem where IPSec
> packets are getting queued up to the workqueue for async processing because
> of the FPU not being available. Since there are also a lot of input pkts, by the
> time xfrm_input() is invoked again after the async operation is completed, the
> IPsec pkts are either out of sequence or out of the replay window, since the
> replay window has advanced. We are using IPSec tunnel between two
> switches connected over a Long Fat Network and have sender and receiver
> servers connected to the two ends of the tunnel. Because of  the TCP
> receiver receiving  pkts either out of order or not receiving pkts because of
> dropped pkts, this is causing significant drop in TCP throughtput on Long Fat
> Networks, where  the network latency is high.

Thanks for the reminder.  I will try to post a patch for this soon.

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: aesni-intel - avoid IPsec re-ordering

[Raj Ammanur]

Thanks a bunch Herbert. I will try out the patch when you make it available.


Are there any thoughts for these other questions from you or anyone
else on the list, more
importantly regarding backlog handling in ESP ?


2. When the pkts are put on the async work queue, the queue size is initialized
as 100 in cryptd_init_queue() in crypto/cryptd.c. Does anyone know how/why
100 was picked ?

3. In cryptd_queue_worker(), the backlog->complete() callback is invoked with
-EINPROGRESS. In net/ipv4/esp4.c, there is no handling for EINPROGRESS
in esp_input_done()/esp_input_done2(). Am I reading the code correctly and
is there any expectation that ESP should support backlog and have backlog
handling ? Currently, the pkt just gets dropped as the EINPROGRESS is treated
as nexthdr and hence is invalid.



On Mon, Jan 18, 2016 at 11:43 PM, Herbert Xu
<herbert@gondor.apana.org.au> wrote:
> Raj Ammanur <rammanur@arista.com> wrote:
>>
>> First, I would like to report that we are also seeing problem where IPSec
>> packets are getting queued up to the workqueue for async processing because
>> of the FPU not being available. Since there are also a lot of input pkts, by the
>> time xfrm_input() is invoked again after the async operation is completed, the
>> IPsec pkts are either out of sequence or out of the replay window, since the
>> replay window has advanced. We are using IPSec tunnel between two
>> switches connected over a Long Fat Network and have sender and receiver
>> servers connected to the two ends of the tunnel. Because of  the TCP
>> receiver receiving  pkts either out of order or not receiving pkts because of
>> dropped pkts, this is causing significant drop in TCP throughtput on Long Fat
>> Networks, where  the network latency is high.
>
> Thanks for the reminder.  I will try to post a patch for this soon.
>
> 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
> --
> 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: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Herbert Xu]

On Tue, Jan 19, 2016 at 09:39:51AM -0800, Raj Ammanur wrote:
> Thanks a bunch Herbert. I will try out the patch when you make it available.
> 
> 
> Are there any thoughts for these other questions from you or anyone
> else on the list, more
> importantly regarding backlog handling in ESP ?
> 
> 
> 2. When the pkts are put on the async work queue, the queue size is initialized
> as 100 in cryptd_init_queue() in crypto/cryptd.c. Does anyone know how/why
> 100 was picked ?

This should probably be increased to match NAPI queue lengths.

> 3. In cryptd_queue_worker(), the backlog->complete() callback is invoked with
> -EINPROGRESS. In net/ipv4/esp4.c, there is no handling for EINPROGRESS
> in esp_input_done()/esp_input_done2(). Am I reading the code correctly and
> is there any expectation that ESP should support backlog and have backlog
> handling ? Currently, the pkt just gets dropped as the EINPROGRESS is treated
> as nexthdr and hence is invalid.

IPsec does not use crypto backlogging so complete should never be
called with EINPROGRESS.  If the queue length is exceeded the
packet will be dropped.

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: aesni-intel - avoid IPsec re-ordering

[Raj Ammanur]

Hi Herbert,

Thanks very much for your prompt replies. Please see inline..

On Tue, Jan 19, 2016 at 7:25 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jan 19, 2016 at 09:39:51AM -0800, Raj Ammanur wrote:
>> Thanks a bunch Herbert. I will try out the patch when you make it available.
>>
>>
>> Are there any thoughts for these other questions from you or anyone
>> else on the list, more
>> importantly regarding backlog handling in ESP ?
>>
>>
>> 2. When the pkts are put on the async work queue, the queue size is initialized
>> as 100 in cryptd_init_queue() in crypto/cryptd.c. Does anyone know how/why
>> 100 was picked ?
>
> This should probably be increased to match NAPI queue lengths.


RAJ: ok. would it also be useful to be able to set the length as a module
load time parameter?

>
>> 3. In cryptd_queue_worker(), the backlog->complete() callback is invoked with
>> -EINPROGRESS. In net/ipv4/esp4.c, there is no handling for EINPROGRESS
>> in esp_input_done()/esp_input_done2(). Am I reading the code correctly and
>> is there any expectation that ESP should support backlog and have backlog
>> handling ? Currently, the pkt just gets dropped as the EINPROGRESS is treated
>> as nexthdr and hence is invalid.
>
> IPsec does not use crypto backlogging so complete should never be
> called with EINPROGRESS.  If the queue length is exceeded the
> packet will be dropped.
>

RAJ: hmm, thats true, yes.

so, is the below description, that you wrote in one of your email replies,
the solution for which you are going to create a patch?

> So what I'm thinking of is to have the softirq path forcibly regain
> control from cryptd where possible.  This is tricky because cryptd
> might be in the middle of processing a request.  So that's why I'd
> like to disable softirqs while we're processing a request.



thanks
--Raj



> 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: aesni-intel - avoid IPsec re-ordering

[Herbert Xu]

On Wed, Jan 20, 2016 at 12:31:46AM -0800, Raj Ammanur wrote:
> 
> RAJ: ok. would it also be useful to be able to set the length as a module
> load time parameter?

That would be a bad interface.  If we wanted to have an adjustable
limit it needs to be set by the entity that is creating the tfm,
i.e., IPsec.
 
> RAJ: hmm, thats true, yes.
> 
> so, is the below description, that you wrote in one of your email replies,
> the solution for which you are going to create a patch?
> 
> > So what I'm thinking of is to have the softirq path forcibly regain
> > control from cryptd where possible.  This is tricky because cryptd
> > might be in the middle of processing a request.  So that's why I'd
> > like to disable softirqs while we're processing a request.

Yes.

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: aesni-intel - avoid IPsec re-ordering

[Raj Ammanur]

On Wed, Jan 20, 2016 at 12:38 AM, Herbert Xu
<herbert@gondor.apana.org.au> wrote:
> On Wed, Jan 20, 2016 at 12:31:46AM -0800, Raj Ammanur wrote:
>>
>> RAJ: ok. would it also be useful to be able to set the length as a module
>> load time parameter?
>
> That would be a bad interface.  If we wanted to have an adjustable
> limit it needs to be set by the entity that is creating the tfm,
> i.e., IPsec.
>


RAJ: Agreed, but it also has been useful for me for testing/debugging to be
able to specify the size to a KLM, since the other stuff is built into
the kernel.
We atleast got rid of the dup acks, although the out of order pkts still remain.

>> RAJ: hmm, thats true, yes.
>>
>> so, is the below description, that you wrote in one of your email replies,
>> the solution for which you are going to create a patch?
>>
>> > So what I'm thinking of is to have the softirq path forcibly regain
>> > control from cryptd where possible.  This is tricky because cryptd
>> > might be in the middle of processing a request.  So that's why I'd
>> > like to disable softirqs while we're processing a request.
>
> Yes.
>

RAJ: Will wait for the patch to take a look and try it out and test
it.  Sincerely appreciate it.

thanks
--Raj

> 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
> --
> 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