Hi Konstantin, Akhil,

Please see inline.

Thanks,
Anoob

-----Original Message-----
From: Ananyev, Konstantin <redacted>
Sent: Saturday, February 1, 2020 12:20 AM
To: Anoob Joseph <redacted>; Akhil Goyal
[off-list ref]; Nicolau, Radu [off-list ref]
Cc: Jerin Jacob Kollanukkaran <redacted>; Lukas Bartosik
[off-list ref]; Narayana Prasad Raju Athreya
[off-list ref]; dev@dpdk.org
Subject: [EXT] RE: [PATCH] examples/ipsec-secgw: increase number of qps to
lcore_params

External Email

----------------------------------------------------------------------

quoted

quoted

quoted

quoted

quoted

Currently only one qp will be used for one core. The number of
qps can be increased to match the number of lcore params.

I don't really understand the purpose of that patch....
As I understand, all it does - unconditionally increases number
of crypto-queues mapped to the same lcore.

[Anoob] With the current code, we will have only crypto qp mapped
to one lcore. So even if you have large number of crypto qps, you
would be only

using as much as the number of lcores.

quoted

This is an inefficient model as a fat flow(say with large packet
sizes) on one eth queue hitting one core can starve another flow
which

happens to hit the same core, because both flows would get queued to
the same qp.

quoted

And, we cannot just randomly submit to multiple qps from the same
core as then the ordering would be messed up.

No-one suggesting that one I believe.

So the best possible usage model would be to map one eth queue to
one

quoted

crypto qp. That way, the core wouldn't be unnecessarily pipeline
the crypto

processing.

I doubt it is really the 'best possible usage model'.
There could be cases when forcing lcore to use/manage more
crypto-queues will lead to negative effects: perf drop, not enough
crypto queues for all eth queues, etc.
If your HW/SW requires to match each eth queue with a separate
crypto-queue, then I think it should be an optional feature, while
keeping default behavior intact.

[Anoob] I think the question here is more to do with s/w crypto PMDs
vs h/w crypto PMDs. For s/w PMDs, having more queues doesn't really

make sense and for h/w PMDs it's better.

Not always.
If these queues belongs to the same device, sometimes it is faster to use just
one queue for device per core.
HW descriptor status polling, etc. is not free.

quoted

I'll see how we can make this an optional feature. Would you be okay
with allowing such behavior if the underlying PMD can support as many

queues as lcore_params?

quoted

As in, if the PMD doesn't support enough queues, we do 1 qp per core.

Would that work for you?

I am not fond of idea to change default mapping method silently.
My preference would be a new command-line option (--cdev-maping or so).
Another thought, make it more fine-grained and user-controlled by
extending eth-port-queue-lcore mapping option.
from current: <port>,<queue>,<lcore>
to <port>,<queue>,<lcore>,<cdev-queue>.

So let say with 2 cores , 2 eth ports/2 queues per port for current mapping
user would do:
# use cdev queue 0 on all cdevs for lcore 6 # use cdev queue 1 on all cdevs for
lcore 7 --lcores="6,7" ... -- --config="(0,0,6,0),(1,0,6,0),(0,1,7,1),(1,1,7,1)"

for the mapping you'd like to have:
--lcores="6,7" ... -- --config="(0,0,6,0),(1,0,6,1),(0,1,7,2),(1,1,7,3)"

[Anoob] I like this idea. This would work for inline case as well.

@Akhil, do you have any comments?

Also, I think we should make it <port>,<queue>,<lcore>,<cdev_id>,<cdev-queue>
 

quoted

Also, if we want to stick to 1 crypto qp per lcore, I don't understand
why we should have the following macro defined such,

#define MAX_QP_PER_LCORE 256

quoted

quoted

quoted

The question is what for?
All these extra queues woulnd't be used by current poll mode
data-path

anyway.

quoted

quoted

Plus in some cases  hash_lookup() would fail to find existing mapping.

[Anoob] It was an issue with v1 that we have addressed with v2.
I'll send v2

shortly. Please do check that see if you have more comments.

quoted

quoted

I understand that for your eventdev implementation you need one
crypto queue for each eth device.
But I think it could be done in less intrusive way (see my
previous mail as one possible option).

[Anoob] If this suggestion is agreeable, then we can solve both qp
number requirement (for inline) and default nb_mbuf calculation in
a much more sensible way. If this doesn't fly, I'll probably go
back to your

suggestion, but then there would be more code just to handle these
cases. And the nb_mbuf calculation could turn slightly complicated.

Don't see how it affects number of required mbufs...
Wouldn't it just be:
<num_of_eth_queues> * <eth_rxd_num + eth+txd_num> +
<num_of_crypto_queues> *<crq_desc_num> +
<lcore_num>*<lcore_cache+reassemble_table_size>+....

[Anoob] What would be num_of_crypto_queues?

quoted

?

quoted

quoted

Konstantin

quoted

Signed-off-by: Anoob Joseph <redacted>
---
 examples/ipsec-secgw/ipsec-secgw.c | 39
+++++++++++++++++++--------------

-----

quoted

 examples/ipsec-secgw/ipsec.h       |  4 +++-
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/examples/ipsec-secgw/ipsec-secgw.c

b/examples/ipsec-secgw/ipsec-secgw.c
index 3b5aaf6..d8c435e 100644

--- a/examples/ipsec-secgw/ipsec-secgw.c
+++ b/examples/ipsec-secgw/ipsec-secgw.c

@@ -1709,6 +1709,8 @@ add_mapping(struct rte_hash *map, const

char

*str, uint16_t cdev_id,

quoted

 	unsigned long i;
 	struct cdev_key key = { 0 };

+	key.port_id = params->port_id;
+	key.queue_id = params->queue_id;
 	key.lcore_id = params->lcore_id;
 	if (cipher)
 		key.cipher_algo = cipher->sym.cipher.algo; @@ -

1721,23

quoted

quoted

quoted

quoted

+1723,17 @@

quoted

add_mapping(struct rte_hash *map, const char *str, uint16_t

cdev_id,

quoted

quoted

quoted

quoted

quoted

 	if (ret != -ENOENT)
 		return 0;

-	for (i = 0; i < ipsec_ctx->nb_qps; i++)
-		if (ipsec_ctx->tbl[i].id == cdev_id)
-			break;
-
-	if (i == ipsec_ctx->nb_qps) {
-		if (ipsec_ctx->nb_qps == MAX_QP_PER_LCORE) {
-			printf("Maximum number of crypto devices

assigned to

quoted

quoted

quoted

quoted

"

quoted

-				"a core, increase

MAX_QP_PER_LCORE

quoted

quoted

quoted

quoted

value\n");

quoted

-			return 0;
-		}
-		ipsec_ctx->tbl[i].id = cdev_id;
-		ipsec_ctx->tbl[i].qp = qp;
-		ipsec_ctx->nb_qps++;
-		printf("%s cdev mapping: lcore %u using cdev %u qp

%u "

quoted

quoted

quoted

quoted

quoted

-				"(cdev_id_qp %lu)\n", str,

key.lcore_id,

quoted

quoted

quoted

quoted

quoted

-				cdev_id, qp, i);
+	i = ipsec_ctx->nb_qps;
+	if (ipsec_ctx->nb_qps == MAX_QP_PER_LCORE) {
+		printf("Maximum number of crypto devices assigned

to a core, "

quoted

quoted

quoted

quoted

quoted

+		       "increase MAX_QP_PER_LCORE value\n");
+		return 0;
 	}
+	ipsec_ctx->tbl[i].id = cdev_id;
+	ipsec_ctx->tbl[i].qp = qp;
+	ipsec_ctx->nb_qps++;
+	printf("%s cdev mapping: lcore %u using cdev %u qp %u "
+	       "(cdev_id_qp %lu)\n", str, key.lcore_id, cdev_id, qp,
+i);

 	ret = rte_hash_add_key_data(map, &key, (void *)i);
 	if (ret < 0) {

@@ -1785,8 +1781,10 @@ add_cdev_mapping(struct

rte_cryptodev_info

*dev_info, uint16_t cdev_id,

quoted

 			continue;

 		if (i->sym.xform_type ==

RTE_CRYPTO_SYM_XFORM_AEAD) {

quoted

quoted

quoted

quoted

quoted

-			ret |= add_mapping(map, str, cdev_id, qp,

params,

quoted

quoted

quoted

quoted

quoted

+			ret = add_mapping(map, str, cdev_id, qp,

params,

quoted

quoted

quoted

quoted

quoted

 					ipsec_ctx, NULL, NULL, i);
+			if (ret)
+				return ret;
 			continue;
 		}

@@ -1801,12 +1799,15 @@ add_cdev_mapping(struct

rte_cryptodev_info

*dev_info, uint16_t cdev_id,

quoted

 			if (j->sym.xform_type !=

RTE_CRYPTO_SYM_XFORM_AUTH)

quoted

 				continue;

-			ret |= add_mapping(map, str, cdev_id, qp,

params,

quoted

quoted

quoted

quoted

quoted

+			ret = add_mapping(map, str, cdev_id, qp,

params,

quoted

quoted

quoted

quoted

quoted

 						ipsec_ctx, i, j, NULL);
+			if (ret)
+				return ret;
+			continue;
 		}
 	}

-	return ret;
+	return 0;
 }

 /* Check if the device is enabled by cryptodev_mask */ diff
--git a/examples/ipsec-secgw/ipsec.h
b/examples/ipsec-secgw/ipsec.h index 8e07521..92fd5eb 100644

--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h

@@ -200,7 +200,9 @@ struct ipsec_ctx {  };

 struct cdev_key {
-	uint16_t lcore_id;
+	uint16_t port_id;
+	uint8_t queue_id;
+	uint8_t lcore_id;
 	uint8_t cipher_algo;
 	uint8_t auth_algo;
 	uint8_t aead_algo;
--

2.7.4