[dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Anoob Joseph]

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

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 examples/ipsec-secgw/ipsec-secgw.c | 39 +++++++++++++++++++-------------------
 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,
 	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 @@ 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 "
-				"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);
+	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,
 			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,
 			if (j->sym.xform_type != RTE_CRYPTO_SYM_XFORM_AUTH)
 				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

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Ananyev, Konstantin]

> 
> 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.
The question is what for?
All these extra queues woulnd't be used by current poll mode data-path anyway.
Plus in some cases  hash_lookup() would fail to find existing mapping.
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).
Konstantin  

> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
>  examples/ipsec-secgw/ipsec-secgw.c | 39 +++++++++++++++++++-------------------
>  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,
>  	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 @@ 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 "
> -				"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);
> +	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,
>  			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,
>  			if (j->sym.xform_type != RTE_CRYPTO_SYM_XFORM_AUTH)
>  				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

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Anoob Joseph]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, January 30, 2020 3:43 AM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Nicolau, Radu <radu.nicolau@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Lukas Bartosik
> <lbartosik@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH] examples/ipsec-secgw: increase number of qps to
> lcore_params
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> 
> >
> > 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.

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. And, we cannot just randomly submit to multiple qps from the same core as then the ordering would be messed up. So the best possible usage model would be to map one eth queue to one crypto qp. That way, the core wouldn't be unnecessarily pipeline the crypto processing.
 
> The question is what for?
> All these extra queues woulnd't be used by current poll mode data-path anyway.
> 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.
 
> 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.
 
> Konstantin
> 
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > ---
> >  examples/ipsec-secgw/ipsec-secgw.c | 39 +++++++++++++++++++--------------
> -----
> >  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,
> >  	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 @@
> > 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
> "
> > -				"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);
> > +	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,
> >  			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,
> >  			if (j->sym.xform_type !=
> RTE_CRYPTO_SYM_XFORM_AUTH)
> >  				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

[dpdk-dev] [PATCH v2] examples/ipsec-secgw: increase number of qps to lcore_params

[Anoob Joseph]

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

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
---
 examples/ipsec-secgw/ipsec-secgw.c | 32 ++++++++++++++++++++++----------
 examples/ipsec-secgw/ipsec.c       | 13 +++++++------
 examples/ipsec-secgw/ipsec.h       |  9 ++++++++-
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
index 3b5aaf6..b49dfef 100644
--- a/examples/ipsec-secgw/ipsec-secgw.c
+++ b/examples/ipsec-secgw/ipsec-secgw.c
@@ -72,7 +72,7 @@
 
 #define MAX_LCORE_PARAMS 1024
 
-#define UNPROTECTED_PORT(port) (unprotected_port_mask & (1 << portid))
+#define UNPROTECTED_PORT(port) (unprotected_port_mask & (1 << port))
 
 /*
  * Configurable number of RX/TX ring descriptors
@@ -968,20 +968,25 @@ route6_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf *pkts[], uint8_t nb_pkts)
 }
 
 static inline void
-process_pkts(struct lcore_conf *qconf, struct rte_mbuf **pkts,
-		uint8_t nb_pkts, uint16_t portid)
+process_pkts(struct lcore_conf *qconf, struct lcore_rx_queue *rx_queue,
+		struct rte_mbuf **pkts, uint8_t nb_pkts)
 {
 	struct ipsec_traffic traffic;
 
 	prepare_traffic(pkts, &traffic, nb_pkts);
 
+	qconf->inbound.port_id = rx_queue->port_id;
+	qconf->inbound.queue_id = rx_queue->queue_id;
+	qconf->outbound.port_id = rx_queue->port_id;
+	qconf->outbound.queue_id = rx_queue->queue_id;
+
 	if (unlikely(single_sa)) {
-		if (UNPROTECTED_PORT(portid))
+		if (UNPROTECTED_PORT(rx_queue->port_id))
 			process_pkts_inbound_nosp(&qconf->inbound, &traffic);
 		else
 			process_pkts_outbound_nosp(&qconf->outbound, &traffic);
 	} else {
-		if (UNPROTECTED_PORT(portid))
+		if (UNPROTECTED_PORT(rx_queue->port_id))
 			process_pkts_inbound(&qconf->inbound, &traffic);
 		else
 			process_pkts_outbound(&qconf->outbound, &traffic);
@@ -1169,7 +1174,7 @@ main_loop(__attribute__((unused)) void *dummy)
 					pkts, MAX_PKT_BURST);
 
 			if (nb_rx > 0)
-				process_pkts(qconf, pkts, nb_rx, portid);
+				process_pkts(qconf, &rxql[i], pkts, nb_rx);
 
 			/* dequeue and process completed crypto-ops */
 			if (UNPROTECTED_PORT(portid))
@@ -1709,6 +1714,8 @@ add_mapping(struct rte_hash *map, const char *str, uint16_t cdev_id,
 	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;
@@ -1722,7 +1729,9 @@ add_mapping(struct rte_hash *map, const char *str, uint16_t cdev_id,
 		return 0;
 
 	for (i = 0; i < ipsec_ctx->nb_qps; i++)
-		if (ipsec_ctx->tbl[i].id == cdev_id)
+		if (ipsec_ctx->tbl[i].id == cdev_id &&
+		    ipsec_ctx->tbl[i].port_id == key.port_id &&
+		    ipsec_ctx->tbl[i].port_queue_id == key.queue_id)
 			break;
 
 	if (i == ipsec_ctx->nb_qps) {
@@ -1733,9 +1742,12 @@ add_mapping(struct rte_hash *map, const char *str, uint16_t cdev_id,
 		}
 		ipsec_ctx->tbl[i].id = cdev_id;
 		ipsec_ctx->tbl[i].qp = qp;
+		ipsec_ctx->tbl[i].port_id = key.port_id;
+		ipsec_ctx->tbl[i].port_queue_id = key.queue_id;
 		ipsec_ctx->nb_qps++;
-		printf("%s cdev mapping: lcore %u using cdev %u qp %u "
-				"(cdev_id_qp %lu)\n", str, key.lcore_id,
+		printf("%s cdev mapping: lcore %u, portid %u, queueid %u "
+				"using cdev %u qp %u (cdev_id_qp %lu)\n",
+				str, key.lcore_id, key.port_id, key.queue_id,
 				cdev_id, qp, i);
 	}
 
@@ -1849,7 +1861,7 @@ cryptodevs_init(void)
 		rte_panic("Failed to create cdev_map hash table, errno = %d\n",
 				rte_errno);
 
-	printf("lcore/cryptodev/qp mappings:\n");
+	printf("lcore/portid/queueid/cryptodev/qp mappings:\n");
 
 	idx = 0;
 	for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
index d4b5712..68059d8 100644
--- a/examples/ipsec-secgw/ipsec.c
+++ b/examples/ipsec-secgw/ipsec.c
@@ -63,6 +63,8 @@ create_lookaside_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa,
 	struct cdev_key key = { 0 };
 
 	key.lcore_id = (uint8_t)rte_lcore_id();
+	key.port_id = ipsec_ctx->port_id;
+	key.queue_id = ipsec_ctx->queue_id;
 
 	key.cipher_algo = (uint8_t)sa->cipher_algo;
 	key.auth_algo = (uint8_t)sa->auth_algo;
@@ -72,12 +74,11 @@ create_lookaside_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa,
 			(void **)&cdev_id_qp);
 	if (ret < 0) {
 		RTE_LOG(ERR, IPSEC,
-				"No cryptodev: core %u, cipher_algo %u, "
-				"auth_algo %u, aead_algo %u\n",
-				key.lcore_id,
-				key.cipher_algo,
-				key.auth_algo,
-				key.aead_algo);
+				"No cryptodev: core %u, port_id %u, "
+				"queue_id %u, cipher_algo %u, auth_algo %u, "
+				"aead_algo %u\n",
+				key.lcore_id, key.port_id, key.queue_id,
+				key.cipher_algo, key.auth_algo, key.aead_algo);
 		return -1;
 	}
 
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index 8e07521..b76cb50 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -180,6 +180,8 @@ struct cdev_qp {
 	uint16_t qp;
 	uint16_t in_flight;
 	uint16_t len;
+	uint16_t port_id;
+	uint16_t port_queue_id;
 	struct rte_crypto_op *buf[MAX_PKT_BURST] __rte_aligned(sizeof(void *));
 };
 
@@ -197,10 +199,15 @@ struct ipsec_ctx {
 	uint16_t ol_pkts_cnt;
 	uint64_t ipv4_offloads;
 	uint64_t ipv6_offloads;
+	/* port_id and queue_id are used to select crypto qp */
+	uint16_t port_id;
+	uint16_t queue_id;
 };
 
 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

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Ananyev, Konstantin]

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

> 
> > The question is what for?
> > All these extra queues woulnd't be used by current poll mode data-path anyway.
> > 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.
> 
> > 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>+....
?
 


> 
> > Konstantin
> >
> > >
> > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > ---
> > >  examples/ipsec-secgw/ipsec-secgw.c | 39 +++++++++++++++++++--------------
> > -----
> > >  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,
> > >  	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 @@
> > > 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
> > "
> > > -				"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);
> > > +	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,
> > >  			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,
> > >  			if (j->sym.xform_type !=
> > RTE_CRYPTO_SYM_XFORM_AUTH)
> > >  				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

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Anoob Joseph]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Friday, January 31, 2020 10:02 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Nicolau, Radu <radu.nicolau@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Lukas Bartosik
> <lbartosik@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH] examples/ipsec-secgw: increase number of qps to
> lcore_params
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> 
> > >
> > > --------------------------------------------------------------------
> > > --
> > >
> > >
> > > >
> > > > 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.
> >
> > 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.
> > 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
> > 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. 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? 

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
 
> 
> >
> > > The question is what for?
> > > All these extra queues woulnd't be used by current poll mode data-path
> anyway.
> > > 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.
> >
> > > 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? 

> ?
> 
> 
> 
> >
> > > Konstantin
> > >
> > > >
> > > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > > ---
> > > >  examples/ipsec-secgw/ipsec-secgw.c | 39
> > > > +++++++++++++++++++--------------
> > > -----
> > > >  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,
> > > >  	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 @@
> > > > 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
> > > "
> > > > -				"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);
> > > > +	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,
> > > >  			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,
> > > >  			if (j->sym.xform_type !=
> > > RTE_CRYPTO_SYM_XFORM_AUTH)
> > > >  				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

Re: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: increase number of qps to lcore_params

[Ananyev, Konstantin]

> Currently only one qp will be used for one core. The number of qps can
> be increased to match the number of lcore params.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
> ---
>  examples/ipsec-secgw/ipsec-secgw.c | 32 ++++++++++++++++++++++----------
>  examples/ipsec-secgw/ipsec.c       | 13 +++++++------
>  examples/ipsec-secgw/ipsec.h       |  9 ++++++++-
>  3 files changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
> index 3b5aaf6..b49dfef 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -72,7 +72,7 @@
> 
>  #define MAX_LCORE_PARAMS 1024
> 
> -#define UNPROTECTED_PORT(port) (unprotected_port_mask & (1 << portid))
> +#define UNPROTECTED_PORT(port) (unprotected_port_mask & (1 << port))
> 
>  /*
>   * Configurable number of RX/TX ring descriptors
> @@ -968,20 +968,25 @@ route6_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf *pkts[], uint8_t nb_pkts)
>  }
> 
>  static inline void
> -process_pkts(struct lcore_conf *qconf, struct rte_mbuf **pkts,
> -		uint8_t nb_pkts, uint16_t portid)
> +process_pkts(struct lcore_conf *qconf, struct lcore_rx_queue *rx_queue,
> +		struct rte_mbuf **pkts, uint8_t nb_pkts)
>  {
>  	struct ipsec_traffic traffic;
> 
>  	prepare_traffic(pkts, &traffic, nb_pkts);
> 
> +	qconf->inbound.port_id = rx_queue->port_id;
> +	qconf->inbound.queue_id = rx_queue->queue_id;
> +	qconf->outbound.port_id = rx_queue->port_id;
> +	qconf->outbound.queue_id = rx_queue->queue_id;
> +
>  	if (unlikely(single_sa)) {
> -		if (UNPROTECTED_PORT(portid))
> +		if (UNPROTECTED_PORT(rx_queue->port_id))
>  			process_pkts_inbound_nosp(&qconf->inbound, &traffic);
>  		else
>  			process_pkts_outbound_nosp(&qconf->outbound, &traffic);
>  	} else {
> -		if (UNPROTECTED_PORT(portid))
> +		if (UNPROTECTED_PORT(rx_queue->port_id))
>  			process_pkts_inbound(&qconf->inbound, &traffic);
>  		else
>  			process_pkts_outbound(&qconf->outbound, &traffic);
> @@ -1169,7 +1174,7 @@ main_loop(__attribute__((unused)) void *dummy)
>  					pkts, MAX_PKT_BURST);
> 
>  			if (nb_rx > 0)
> -				process_pkts(qconf, pkts, nb_rx, portid);
> +				process_pkts(qconf, &rxql[i], pkts, nb_rx);
> 
>  			/* dequeue and process completed crypto-ops */
>  			if (UNPROTECTED_PORT(portid))
> @@ -1709,6 +1714,8 @@ add_mapping(struct rte_hash *map, const char *str, uint16_t cdev_id,
>  	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;
> @@ -1722,7 +1729,9 @@ add_mapping(struct rte_hash *map, const char *str, uint16_t cdev_id,
>  		return 0;
> 
>  	for (i = 0; i < ipsec_ctx->nb_qps; i++)
> -		if (ipsec_ctx->tbl[i].id == cdev_id)
> +		if (ipsec_ctx->tbl[i].id == cdev_id &&
> +		    ipsec_ctx->tbl[i].port_id == key.port_id &&
> +		    ipsec_ctx->tbl[i].port_queue_id == key.queue_id)

I didn't test the patch, but at first glance this approach seems
reasonable to me with one main objection -
I don't think this new mapping method should become default and only possible one.
Probably need an extra option to allow user to select,
while keeping current mapping method as a default one.
Then this option could be translated into some global mask that will be applied
to cdev_key before add/lookup or so.  

>  			break;
> 
>  	if (i == ipsec_ctx->nb_qps) {
> @@ -1733,9 +1742,12 @@ add_mapping(struct rte_hash *map, const char *str, uint16_t cdev_id,
>  		}
>  		ipsec_ctx->tbl[i].id = cdev_id;
>  		ipsec_ctx->tbl[i].qp = qp;
> +		ipsec_ctx->tbl[i].port_id = key.port_id;
> +		ipsec_ctx->tbl[i].port_queue_id = key.queue_id;
>  		ipsec_ctx->nb_qps++;
> -		printf("%s cdev mapping: lcore %u using cdev %u qp %u "
> -				"(cdev_id_qp %lu)\n", str, key.lcore_id,
> +		printf("%s cdev mapping: lcore %u, portid %u, queueid %u "
> +				"using cdev %u qp %u (cdev_id_qp %lu)\n",
> +				str, key.lcore_id, key.port_id, key.queue_id,
>  				cdev_id, qp, i);
>  	}
> 
> @@ -1849,7 +1861,7 @@ cryptodevs_init(void)
>  		rte_panic("Failed to create cdev_map hash table, errno = %d\n",
>  				rte_errno);
> 
> -	printf("lcore/cryptodev/qp mappings:\n");
> +	printf("lcore/portid/queueid/cryptodev/qp mappings:\n");
> 
>  	idx = 0;
>  	for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> index d4b5712..68059d8 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -63,6 +63,8 @@ create_lookaside_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa,
>  	struct cdev_key key = { 0 };
> 
>  	key.lcore_id = (uint8_t)rte_lcore_id();
> +	key.port_id = ipsec_ctx->port_id;
> +	key.queue_id = ipsec_ctx->queue_id;
> 
>  	key.cipher_algo = (uint8_t)sa->cipher_algo;
>  	key.auth_algo = (uint8_t)sa->auth_algo;
> @@ -72,12 +74,11 @@ create_lookaside_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa,
>  			(void **)&cdev_id_qp);
>  	if (ret < 0) {
>  		RTE_LOG(ERR, IPSEC,
> -				"No cryptodev: core %u, cipher_algo %u, "
> -				"auth_algo %u, aead_algo %u\n",
> -				key.lcore_id,
> -				key.cipher_algo,
> -				key.auth_algo,
> -				key.aead_algo);
> +				"No cryptodev: core %u, port_id %u, "
> +				"queue_id %u, cipher_algo %u, auth_algo %u, "
> +				"aead_algo %u\n",
> +				key.lcore_id, key.port_id, key.queue_id,
> +				key.cipher_algo, key.auth_algo, key.aead_algo);
>  		return -1;
>  	}
> 
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index 8e07521..b76cb50 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -180,6 +180,8 @@ struct cdev_qp {
>  	uint16_t qp;
>  	uint16_t in_flight;
>  	uint16_t len;
> +	uint16_t port_id;
> +	uint16_t port_queue_id;
>  	struct rte_crypto_op *buf[MAX_PKT_BURST] __rte_aligned(sizeof(void *));
>  };
> 
> @@ -197,10 +199,15 @@ struct ipsec_ctx {
>  	uint16_t ol_pkts_cnt;
>  	uint64_t ipv4_offloads;
>  	uint64_t ipv6_offloads;
> +	/* port_id and queue_id are used to select crypto qp */
> +	uint16_t port_id;
> +	uint16_t queue_id;
>  };
> 
>  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

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Ananyev, Konstantin]

> > > > > 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.
> > >
> > > 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.
> > > 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
> > > 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
> 
> >
> > >
> > > > The question is what for?
> > > > All these extra queues woulnd't be used by current poll mode data-path
> > anyway.
> > > > 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.
> > >
> > > > 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?
> 
> > ?
> >
> >
> >
> > >
> > > > Konstantin
> > > >
> > > > >
> > > > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > > > ---
> > > > >  examples/ipsec-secgw/ipsec-secgw.c | 39
> > > > > +++++++++++++++++++--------------
> > > > -----
> > > > >  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,
> > > > >  	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 @@
> > > > > 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
> > > > "
> > > > > -				"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);
> > > > > +	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,
> > > > >  			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,
> > > > >  			if (j->sym.xform_type !=
> > > > RTE_CRYPTO_SYM_XFORM_AUTH)
> > > > >  				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

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Anoob Joseph]

Hi Konstantin, Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Saturday, February 1, 2020 12:20 AM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Lukas Bartosik
> <lbartosik@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH] examples/ipsec-secgw: increase number of qps to
> lcore_params
> 
> External Email
> 
> ----------------------------------------------------------------------
> > > > > > 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.
> > > >
> > > > 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.
> > > > 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
> > > > 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)"

[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>
 
> 
> > 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
> >
> > >
> > > >
> > > > > The question is what for?
> > > > > All these extra queues woulnd't be used by current poll mode
> > > > > data-path
> > > anyway.
> > > > > 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.
> > > >
> > > > > 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?
> >
> > > ?
> > >
> > >
> > >
> > > >
> > > > > Konstantin
> > > > >
> > > > > >
> > > > > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > > > > ---
> > > > > >  examples/ipsec-secgw/ipsec-secgw.c | 39
> > > > > > +++++++++++++++++++--------------
> > > > > -----
> > > > > >  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,
> > > > > >  	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 @@
> > > > > > 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
> > > > > "
> > > > > > -				"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);
> > > > > > +	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,
> > > > > >  			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,
> > > > > >  			if (j->sym.xform_type !=
> > > > > RTE_CRYPTO_SYM_XFORM_AUTH)
> > > > > >  				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

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Akhil Goyal]

> > > > > > > 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.
> > > > >
> > > > > 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.
> > > > > 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
> > > > > 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)"
> 
> [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>
> 
Looks good to me, but I believe this would need more changes and testing in event patches.
Also it does not have any changes for lookaside cases.
Can we move this to next release and add lookaside case as well in a single go.
We need to close the RC2 on 5th Feb, and I don't want to push this series for RC3 as it is a massive change in ipsec-secgw.

-Akhil

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Anoob Joseph]

Hi Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Monday, February 3, 2020 2:46 PM
> To: Anoob Joseph <anoobj@marvell.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Lukas Bartosik
> <lbartosik@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH] examples/ipsec-secgw: increase number of qps to
> lcore_params
> 
> External Email
> 
> ----------------------------------------------------------------------
> > > > > > > > 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.
> > > > > >
> > > > > > 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.
> > > > > > 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
> > > > > > 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)"
> >
> > [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>
> >
> Looks good to me, but I believe this would need more changes and testing in
> event patches.
> Also it does not have any changes for lookaside cases.
> Can we move this to next release and add lookaside case as well in a single
> go.

[Anoob] Not a problem. I'll defer this to next release then.
 
> We need to close the RC2 on 5th Feb, and I don't want to push this series for
> RC3 as it is a massive change in ipsec-secgw.
> 
> -Akhil

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Anoob Joseph]

Hi Akhil,

> > > @Akhil, do you have any comments?
> > >
> > > Also, I think we should make it
> > > <port>,<queue>,<lcore>,<cdev_id>,<cdev-queue>
> > >
> > Looks good to me, but I believe this would need more changes and
> > testing in event patches.
> > Also it does not have any changes for lookaside cases.
> > Can we move this to next release and add lookaside case as well in a
> > single go.
> 
> [Anoob] Not a problem. I'll defer this to next release then.

This patch is not part of the event additions to ipsec-segcw. This patch is required to handle few corner cases, but is equally applicable to lookaside crypto as well. I had agreed to only deferring this patch.

Lukasz is preparing v4 of event mode patches with doc update and addressing one minor comment from Konstantin. If the event ipsec-secgw patches are not getting merged for this release, can you confirm it will be merged immediately after this release? I'm assuming you have finished the reviews as the patch was submitted early in this release cycle.

Thanks,
Anoob

> -----Original Message-----
> From: Anoob Joseph
> Sent: Monday, February 3, 2020 2:52 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Lukas Bartosik
> <lbartosik@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; dev@dpdk.org
> Subject: RE: [PATCH] examples/ipsec-secgw: increase number of qps to
> lcore_params
> 
> Hi Akhil,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: Monday, February 3, 2020 2:46 PM
> > To: Anoob Joseph <anoobj@marvell.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>
> > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Lukas Bartosik
> > <lbartosik@marvell.com>; Narayana Prasad Raju Athreya
> > <pathreya@marvell.com>; dev@dpdk.org
> > Subject: [EXT] RE: [PATCH] examples/ipsec-secgw: increase number of
> > qps to lcore_params
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > > > > > > > > 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.
> > > > > > >
> > > > > > > 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.
> > > > > > > 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
> > > > > > > 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)"
> > >
> > > [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>
> > >
> > Looks good to me, but I believe this would need more changes and
> > testing in event patches.
> > Also it does not have any changes for lookaside cases.
> > Can we move this to next release and add lookaside case as well in a
> > single go.
> 
> [Anoob] Not a problem. I'll defer this to next release then.
> 
> > We need to close the RC2 on 5th Feb, and I don't want to push this
> > series for
> > RC3 as it is a massive change in ipsec-secgw.
> >
> > -Akhil

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Akhil Goyal]

> 
> Hi Akhil,
> 
> > > > @Akhil, do you have any comments?
> > > >
> > > > Also, I think we should make it
> > > > <port>,<queue>,<lcore>,<cdev_id>,<cdev-queue>
> > > >
> > > Looks good to me, but I believe this would need more changes and
> > > testing in event patches.
> > > Also it does not have any changes for lookaside cases.
> > > Can we move this to next release and add lookaside case as well in a
> > > single go.
> >
> > [Anoob] Not a problem. I'll defer this to next release then.
> 
> This patch is not part of the event additions to ipsec-segcw. This patch is
> required to handle few corner cases, but is equally applicable to lookaside
> crypto as well. I had agreed to only deferring this patch.
> 
> Lukasz is preparing v4 of event mode patches with doc update and addressing
> one minor comment from Konstantin. If the event ipsec-secgw patches are not
> getting merged for this release, can you confirm it will be merged immediately
> after this release? I'm assuming you have finished the reviews as the patch was
> submitted early in this release cycle.
> 
I would say Lukasz should wait for sending the next version till I complete the review.
I will notify as soon as possible.
I am not done with the review of the event patches yet. I need more time for that.
Yes, I believe these will be merged early in the next cycle.

-Akhil

Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: increase number of qps to lcore_params

[Thomas Monjalon]

06/02/2020 11:46, Anoob Joseph:
> Hi Akhil,
> 
> > > > @Akhil, do you have any comments?
> > > >
> > > > Also, I think we should make it
> > > > <port>,<queue>,<lcore>,<cdev_id>,<cdev-queue>
> > > >
> > > Looks good to me, but I believe this would need more changes and
> > > testing in event patches.
> > > Also it does not have any changes for lookaside cases.
> > > Can we move this to next release and add lookaside case as well in a
> > > single go.
> > 
> > [Anoob] Not a problem. I'll defer this to next release then.
> 
> This patch is not part of the event additions to ipsec-segcw. This patch is required to handle few corner cases, but is equally applicable to lookaside crypto as well. I had agreed to only deferring this patch.
> 
> Lukasz is preparing v4 of event mode patches with doc update and addressing one minor comment from Konstantin. If the event ipsec-secgw patches are not getting merged for this release, can you confirm it will be merged immediately after this release? I'm assuming you have finished the reviews as the patch was submitted early in this release cycle.

Akhil, as all other tree maintainers, is doing its best.
Akhil is doing a great job, and had a lot of work in this release cycle.
Please be patient.