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

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ananyev, Konstantin
> Sent: Monday, December 23, 2019 9:47 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: Re: [dpdk-dev] [PATCH 14/14] examples/ipsec-secgw: add cmd line
> option for bufs
> 
> 
> 
> > > 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 76719f2..f8e28d6 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
> > > @@ -167,6 +165,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 single_sa_idx;
> > > +static uint32_t nb_bufs_in_pool = 8192;
> >
> > Why to change the default number (behavior) here?
> > Why not to keep existing one as default?
> 
> Or, at least try to guess required number of mbufs (like l3fwd, etc., do)?

[Anoob] Existing code sets the default number of mbufs to 32k, which is leading to higher cache misses on our platform. Also, other example applications have 8192 as the minimum. Hence the change.

Do you see any perf issues with lowering the default value? Also, I'm fine with making the default one same as the ones in l2fwd & l3fwd.

From l3fwd:

/*
 * This expression is used to calculate the number of mbufs needed
 * depending on user input, taking  into account memory for rx and
 * tx hardware rings, cache per lcore and mtable per port per lcore.
 * RTE_MAX is used to ensure that NB_MBUF never goes below a minimum
 * value of 8192
 */
#define NB_MBUF(nports) RTE_MAX(	\
	(nports*nb_rx_queue*nb_rxd +		\
	nports*nb_lcores*MAX_PKT_BURST +	\
	nports*n_tx_queue*nb_txd +		\
	nb_lcores*MEMPOOL_CACHE_SIZE),		\
	(unsigned)8192)

I do understand that we will have to rework the above logic a bit more to handle the in-flight packets in cryptodev. What's your suggestion?
  
> 
> >
> > >
> > >  /*
> > >   * RX/TX HW offload capabilities to enable/use on ethernet ports.
> > > @@ -1261,6 +1260,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]"
> > > @@ -1284,6 +1284,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: Use single SA index for outbound
> traffic,\n"
> > > @@ -1534,7 +1535,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) {
> > > @@ -1568,6 +1569,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 || @@ -
> 2792,11 +2806,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