RE: [PATCH v2] examples/l3fwd: support setting the data size of mbuf

[Chaoyong He <chaoyong.he@corigine.com>]

> On Fri, 18 Oct 2024 10:42:53 +0800
> Chaoyong He <chaoyong.he@corigine.com> wrote:
> 
> > From: Long Wu <long.wu@corigine.com>
> >
> > The previous code used a macro as the data size for mbuf to create the
> > mempool and users cannot modify the size.
> >
> > Now modify the code to support setting the data size of mbuf by
> > '--mbuf-size' parameter. If user does not add the parameter in start
> > command line, the default size is still 'RTE_MBUF_DEFAULT_BUF_SIZE'.
> >
> > Examples:
> > dpdk-l3fwd -l 0-3 -- -p 0x03 --mbuf-size=4096
> >
> > Signed-off-by: Long Wu <long.wu@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >
> > ---
> > v2:
> > * Modify some logic following the advices of reviewer.
> > * Add the 'Acked-by' tag.
> > ---
> >  doc/guides/sample_app_ug/l3_forward.rst |  2 ++
> >  examples/l3fwd/main.c                   | 31 ++++++++++++++++++++++---
> >  2 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/guides/sample_app_ug/l3_forward.rst
> > b/doc/guides/sample_app_ug/l3_forward.rst
> > index 1cc2c1dd1d..5afbbb242b 100644
> > --- a/doc/guides/sample_app_ug/l3_forward.rst
> > +++ b/doc/guides/sample_app_ug/l3_forward.rst
> > @@ -143,6 +143,8 @@ Where,
> >  * ``--alg=<val>:`` optional, ACL classify method to use, one of:
> >    ``scalar|sse|avx2|neon|altivec|avx512x16|avx512x32``
> >
> > +* ``--mbuf-size=N:`` Optional, Set the data size of mbuf to N bytes.
> > +
> >  * ``-E:`` Optional, enable exact match,
> >    legacy flag, please use ``--lookup=em`` instead.
> >
> > diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c index
> > 01b763e5ba..ed5d0c2608 100644
> > --- a/examples/l3fwd/main.c
> > +++ b/examples/l3fwd/main.c
> > @@ -140,6 +140,7 @@ uint32_t max_pkt_len;  #ifdef RTE_LIB_EVENTDEV
> > static struct rte_mempool *vector_pool[RTE_MAX_ETHPORTS];  #endif
> > +static uint16_t mbuf_data_size = RTE_MBUF_DEFAULT_DATAROOM;
> >  static struct rte_mempool
> > *pktmbuf_pool[RTE_MAX_ETHPORTS][NB_SOCKETS];
> >  static uint8_t lkp_per_socket[NB_SOCKETS];
> >
> > @@ -448,7 +449,8 @@ print_usage(const char *prgname)
> >  		"                    One is ACL entry at while line leads with character
> '%c',\n"
> >  		"                    another is route entry at while line leads with
> character '%c'.\n"
> >  		"  --rule_ipv6=FILE: Specify the ipv6 rules entries file.\n"
> > -		"  --alg: ACL classify method to use, one of: %s.\n\n",
> > +		"  --alg: ACL classify method to use, one of: %s.\n"
> > +		"  --mbuf-size=N: Set the data size of mbuf to N bytes.\n\n",
> >  		prgname, RX_DESC_DEFAULT, TX_DESC_DEFAULT,
> >  		ACL_LEAD_CHAR, ROUTE_LEAD_CHAR, alg);  } @@ -667,6
> +669,22 @@
> > parse_lookup(const char *optarg)
> >  	return 0;
> >  }
> >
> > +static void
> > +parse_mbuf_data_size(const char *optarg) {
> > +	char *end = NULL;
> > +
> > +	mbuf_data_size = strtoul(optarg, &end, 10);
> > +	if ((optarg[0] == '\0') || (end == NULL) || (*end != '\0'))
> > +		rte_exit(EXIT_FAILURE, "Invalid mbuf data size: %s\n",
> optarg);
> > +
> > +	if (mbuf_data_size < RTE_ETHER_MIN_LEN ||
> > +			mbuf_data_size > 0xFFFF -
> RTE_PKTMBUF_HEADROOM)
> 
> For clarity replace 0xffff with UINT16_MAX (which is data_len)
> 
> > +		rte_exit(EXIT_FAILURE,
> > +				"mbuf-size should be >= %u and <= %u\n",
> > +				RTE_ETHER_MIN_LEN, 0xFFFF -
> RTE_PKTMBUF_HEADROOM); }
> > +
> 
> Not sure why this is needed? What is the problem with the original code?
> Are you trying to force packets to be segmented?

Actually, we are trying to force packets *not* segmented by making the mbuf size large enough to hold the packets.

In our user case, we start l3fwd app with parameter '--max-pkt-len 4000', and obviously the original logic with RTE_MBUF_DEFAULT_DATAROOM mbuf size will cause the packets to be segmented.
Which is not what we want, so we add this new '--mbuf-size=4096' parameter, the mbuf size will large enough to hold even the largest packet.

Do you think this make sense?