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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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