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

[Anoob Joseph <anoobj@marvell.com>]

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