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.

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?

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

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

+1723,17 @@

quoted

add_mapping(struct rte_hash *map, const char *str, uint16_t cdev_id,
 	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

-				"a core, increase MAX_QP_PER_LCORE

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 "
-				"(cdev_id_qp %lu)\n", str, key.lcore_id,
-				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, "
+		       "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) {
-			ret |= add_mapping(map, str, cdev_id, qp, params,
+			ret = add_mapping(map, str, cdev_id, qp, params,
 					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,
+			ret = add_mapping(map, str, cdev_id, qp, params,
 						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