Re: [dpdk-dev] [PATCH v2 12/12] examples/ipsec-secgw: add cmd line option for bufs

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Wednesday, January 29, 2020 8:11 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Nicolau, Radu <radu.nicolau@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Lukas Bartosik <lbartosik@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Archana
> Muniganti <marchana@marvell.com>; Tejasree Kondoj
> <ktejasree@marvell.com>; Vamsi Krishna Attunuru <vattunuru@marvell.com>;
> dev@dpdk.org
> Subject: [EXT] RE: [PATCH v2 12/12] examples/ipsec-secgw: add cmd line option
> for bufs
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> >
> > From: Lukasz Bartosik <lbartosik@marvell.com>
> >
> > Add command line option -s which can be used to configure number of
> > buffers in a pool. Default number of buffers is 8192.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
> > ---
> >  examples/ipsec-secgw/ipsec-secgw.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/ipsec-secgw.c
> > b/examples/ipsec-secgw/ipsec-secgw.c
> > index 7d844bb..a67ea0a 100644
> > --- a/examples/ipsec-secgw/ipsec-secgw.c
> > +++ b/examples/ipsec-secgw/ipsec-secgw.c
> > @@ -59,8 +59,6 @@ volatile bool force_quit;
> >
> >  #define MEMPOOL_CACHE_SIZE 256
> >
> > -#define NB_MBUF	(32000)
> > -
> >  #define CDEV_QUEUE_DESC 2048
> >  #define CDEV_MAP_ENTRIES 16384
> >  #define CDEV_MP_NB_OBJS 1024
> > @@ -162,6 +160,7 @@ static int32_t numa_on = 1; /**< NUMA is enabled
> > by default. */  static uint32_t nb_lcores;  static uint32_t single_sa;
> > static uint32_t schedule_type;
> > +static uint32_t nb_bufs_in_pool = 8192;
> 
> I still think it is not a good idea to change default number of mbufs.
> 8K is not that much: 1 core with 4 ports, or 1 port over 4 cores, and user might
> start to see unexpected failures.
> Now you added an option to allow user define number of mbufs in the app,
> which is a good thing, but default one I think should remain the same (to avoid
> any unexpected failures).
> Konstantin

[Anoob] No disagreement. I had submitted this patch as is since I had some other ideas which could solve this better. I had mentioned this in the cover-letter.
 
Deferred to v3:
* The final patch updates the hardcoded number of buffers in a pool.
   Also, there was a discussion on the update of number of qp. Both the
   above can be handled properly, if we can remove the logic which limits
   one core to only use one crypto qp. If we can allow one qp per
   lcore_param, every eth queue can have it's own crypto qp and that would
   solve the requirements with OCTEON TX2 inline ipsec support as well.

http://patches.dpdk.org/patch/64408/

The above patch requires a minor rework and I would be submitting a v2 soon. But the idea would be same. Please take a look at it and share your thoughts. Please do wait for v2 before running on h/w, though 😊.

> 
> 
> >
> >  /*
> >   * RX/TX HW offload capabilities to enable/use on ethernet ports.
> > @@ -1264,6 +1263,7 @@ print_usage(const char *prgname)
> >  		" [-w REPLAY_WINDOW_SIZE]"
> >  		" [-e]"
> >  		" [-a]"
> > +		" [-s NUMBER_OF_MBUFS_IN_PKT_POOL]"
> >  		" -f CONFIG_FILE"
> >  		" --config (port,queue,lcore)[,(port,queue,lcore)]"
> >  		" [--single-sa SAIDX]"
> > @@ -1285,6 +1285,7 @@ print_usage(const char *prgname)
> >  		"     size for each SA\n"
> >  		"  -e enables ESN\n"
> >  		"  -a enables SA SQN atomic behaviour\n"
> > +		"  -s number of mbufs in packet pool (default 8192)\n"
> >  		"  -f CONFIG_FILE: Configuration file\n"
> >  		"  --config (port,queue,lcore): Rx queue configuration\n"
> >  		"  --single-sa SAIDX: In poll mode use single SA index for\n"
> > @@ -1484,7 +1485,7 @@ parse_args(int32_t argc, char **argv, struct
> > eh_conf *eh_conf)
> >
> >  	argvopt = argv;
> >
> > -	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:",
> > +	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:s:",
> >  				lgopts, &option_index)) != EOF) {
> >
> >  		switch (opt) {
> > @@ -1518,6 +1519,19 @@ parse_args(int32_t argc, char **argv, struct
> eh_conf *eh_conf)
> >  			cfgfile = optarg;
> >  			f_present = 1;
> >  			break;
> > +
> > +		case 's':
> > +			ret = parse_decimal(optarg);
> > +			if (ret < 0) {
> > +				printf("Invalid number of buffers in a pool: "
> > +					"%s\n", optarg);
> > +				print_usage(prgname);
> > +				return -1;
> > +			}
> > +
> > +			nb_bufs_in_pool = ret;
> > +			break;
> > +
> >  		case 'j':
> >  			ret = parse_decimal(optarg);
> >  			if (ret < RTE_MBUF_DEFAULT_BUF_SIZE || @@ -
> 2753,11 +2767,12 @@
> > main(int32_t argc, char **argv)
> >  		if (socket_ctx[socket_id].mbuf_pool)
> >  			continue;
> >
> > -		pool_init(&socket_ctx[socket_id], socket_id, NB_MBUF);
> > +		pool_init(&socket_ctx[socket_id], socket_id, nb_bufs_in_pool);
> >  		session_pool_init(&socket_ctx[socket_id], socket_id, sess_sz);
> >  		session_priv_pool_init(&socket_ctx[socket_id], socket_id,
> >  			sess_sz);
> >  	}
> > +	printf("Number of mbufs in packet pool %d\n", nb_bufs_in_pool);
> >
> >  	RTE_ETH_FOREACH_DEV(portid) {
> >  		if ((enabled_port_mask & (1 << portid)) == 0)
> > --
> > 2.7.4