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

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

[Chaoyong He]

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>
---
 examples/l3fwd/main.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 01b763e5ba..ccce16c6bb 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_seg_size = RTE_MBUF_DEFAULT_BUF_SIZE;
 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);
 }
@@ -698,6 +700,7 @@ static const char short_options[] =
 #define CMD_LINE_OPT_RULE_IPV4 "rule_ipv4"
 #define CMD_LINE_OPT_RULE_IPV6 "rule_ipv6"
 #define CMD_LINE_OPT_ALG "alg"
+#define CMD_LINE_OPT_MBUF_SIZE "mbuf-size"
 
 enum {
 	/* long options mapped to a short option */
@@ -726,7 +729,8 @@ enum {
 	CMD_LINE_OPT_LOOKUP_NUM,
 	CMD_LINE_OPT_ENABLE_VECTOR_NUM,
 	CMD_LINE_OPT_VECTOR_SIZE_NUM,
-	CMD_LINE_OPT_VECTOR_TMO_NS_NUM
+	CMD_LINE_OPT_VECTOR_TMO_NS_NUM,
+	CMD_LINE_OPT_MBUF_SIZE_NUM,
 };
 
 static const struct option lgopts[] = {
@@ -753,6 +757,7 @@ static const struct option lgopts[] = {
 	{CMD_LINE_OPT_RULE_IPV4,   1, 0, CMD_LINE_OPT_RULE_IPV4_NUM},
 	{CMD_LINE_OPT_RULE_IPV6,   1, 0, CMD_LINE_OPT_RULE_IPV6_NUM},
 	{CMD_LINE_OPT_ALG,   1, 0, CMD_LINE_OPT_ALG_NUM},
+	{CMD_LINE_OPT_MBUF_SIZE, 1, 0, CMD_LINE_OPT_MBUF_SIZE_NUM},
 	{NULL, 0, 0, 0}
 };
 
@@ -934,6 +939,12 @@ parse_args(int argc, char **argv)
 		case CMD_LINE_OPT_ALG_NUM:
 			l3fwd_set_alg(optarg);
 			break;
+		case CMD_LINE_OPT_MBUF_SIZE_NUM:
+			mbuf_seg_size = strtoul(optarg, NULL, 10) + RTE_PKTMBUF_HEADROOM;
+			if (mbuf_seg_size <= 0 || mbuf_seg_size > 0xFFFF)
+				rte_exit(EXIT_FAILURE,
+						"mbuf-size should be > 0 and < 65536\n");
+			break;
 		default:
 			print_usage(prgname);
 			return -1;
@@ -1034,7 +1045,7 @@ init_mem(uint16_t portid, unsigned int nb_mbuf)
 			pktmbuf_pool[portid][socketid] =
 				rte_pktmbuf_pool_create(s, nb_mbuf,
 					MEMPOOL_CACHE_SIZE, 0,
-					RTE_MBUF_DEFAULT_BUF_SIZE, socketid);
+					mbuf_seg_size, socketid);
 			if (pktmbuf_pool[portid][socketid] == NULL)
 				rte_exit(EXIT_FAILURE,
 					"Cannot init mbuf pool on socket %d\n",
-- 
2.39.1

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

[Morten Brørup]

> From: Chaoyong He [mailto:chaoyong.he@corigine.com]
> Sent: Wednesday, 16 October 2024 10.23
> 
> 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>
> ---
>  examples/l3fwd/main.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 01b763e5ba..ccce16c6bb 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_seg_size = RTE_MBUF_DEFAULT_BUF_SIZE;

Prefer variable using same offset as command line parameter:
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);
>  }
> @@ -698,6 +700,7 @@ static const char short_options[] =
>  #define CMD_LINE_OPT_RULE_IPV4 "rule_ipv4"
>  #define CMD_LINE_OPT_RULE_IPV6 "rule_ipv6"
>  #define CMD_LINE_OPT_ALG "alg"
> +#define CMD_LINE_OPT_MBUF_SIZE "mbuf-size"
> 
>  enum {
>  	/* long options mapped to a short option */
> @@ -726,7 +729,8 @@ enum {
>  	CMD_LINE_OPT_LOOKUP_NUM,
>  	CMD_LINE_OPT_ENABLE_VECTOR_NUM,
>  	CMD_LINE_OPT_VECTOR_SIZE_NUM,
> -	CMD_LINE_OPT_VECTOR_TMO_NS_NUM
> +	CMD_LINE_OPT_VECTOR_TMO_NS_NUM,
> +	CMD_LINE_OPT_MBUF_SIZE_NUM,
>  };
> 
>  static const struct option lgopts[] = {
> @@ -753,6 +757,7 @@ static const struct option lgopts[] = {
>  	{CMD_LINE_OPT_RULE_IPV4,   1, 0, CMD_LINE_OPT_RULE_IPV4_NUM},
>  	{CMD_LINE_OPT_RULE_IPV6,   1, 0, CMD_LINE_OPT_RULE_IPV6_NUM},
>  	{CMD_LINE_OPT_ALG,   1, 0, CMD_LINE_OPT_ALG_NUM},
> +	{CMD_LINE_OPT_MBUF_SIZE, 1, 0, CMD_LINE_OPT_MBUF_SIZE_NUM},
>  	{NULL, 0, 0, 0}
>  };
> 
> @@ -934,6 +939,12 @@ parse_args(int argc, char **argv)
>  		case CMD_LINE_OPT_ALG_NUM:
>  			l3fwd_set_alg(optarg);
>  			break;
> +		case CMD_LINE_OPT_MBUF_SIZE_NUM:
> +			mbuf_seg_size = strtoul(optarg, NULL, 10) +
> RTE_PKTMBUF_HEADROOM;
> +			if (mbuf_seg_size <= 0 || mbuf_seg_size > 0xFFFF)
> +				rte_exit(EXIT_FAILURE,
> +						"mbuf-size should be > 0 and <
> 65536\n");

mbuf_data_size = strtoul(optarg, NULL, 10);
if (mbuf_data_size == ULONG_MAX)
	rte_exit(EXIT_FAILURE, "mbuf-size should be a number\n");
if (mbuf_data_size < RTE_ETHER_MIN_LEN ||
		mbuf_data_size > 0xFFFF - RTE_PKTMBUF_HEADROOM)
	rte_exit(EXIT_FAILURE,
			"mbuf-size should be >= %u and <= %u\n",
			RTE_ETHER_MIN_LEN, 0xFFFF - RTE_PKTMBUF_HEADROOM);

> +			break;
>  		default:
>  			print_usage(prgname);
>  			return -1;
> @@ -1034,7 +1045,7 @@ init_mem(uint16_t portid, unsigned int nb_mbuf)
>  			pktmbuf_pool[portid][socketid] =
>  				rte_pktmbuf_pool_create(s, nb_mbuf,
>  					MEMPOOL_CACHE_SIZE, 0,
> -					RTE_MBUF_DEFAULT_BUF_SIZE, socketid);
> +					mbuf_seg_size, socketid);

mbuf_data_size + RTE_PKTMBUF_HEADROOM, socketid);

>  			if (pktmbuf_pool[portid][socketid] == NULL)
>  				rte_exit(EXIT_FAILURE,
>  					"Cannot init mbuf pool on socket %d\n",
> --
> 2.39.1

With above changes,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

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

[Stephen Hemminger]

On Wed, 16 Oct 2024 16:22:32 +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>

Why not automatically determine the mbuf size based on mtu?
Would be more natural.

L3fwd functions as a test program and often used as a starting
point for best practices for doing new applications.

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

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 4835 bytes --]

Sorry to interrupt the conversation - obviously the CI testing fail is a
false failure and should be ignored by the maintainer here.

For those curious, it failed like this, which I have seen previously in a
similar case:
tester: Pkt number not matched,2000 sent and 1999 received

I'm removing this legacy DTS testsuite from our CI testing as it cannot be
trusted and also the testsuite has been rewritten recently (and we are
switching to the new version).

Sorry to be a bother!

On Wed, Oct 16, 2024 at 4:22 AM 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>
> ---
>  examples/l3fwd/main.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 01b763e5ba..ccce16c6bb 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_seg_size = RTE_MBUF_DEFAULT_BUF_SIZE;
>  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);
>  }
> @@ -698,6 +700,7 @@ static const char short_options[] =
>  #define CMD_LINE_OPT_RULE_IPV4 "rule_ipv4"
>  #define CMD_LINE_OPT_RULE_IPV6 "rule_ipv6"
>  #define CMD_LINE_OPT_ALG "alg"
> +#define CMD_LINE_OPT_MBUF_SIZE "mbuf-size"
>
>  enum {
>         /* long options mapped to a short option */
> @@ -726,7 +729,8 @@ enum {
>         CMD_LINE_OPT_LOOKUP_NUM,
>         CMD_LINE_OPT_ENABLE_VECTOR_NUM,
>         CMD_LINE_OPT_VECTOR_SIZE_NUM,
> -       CMD_LINE_OPT_VECTOR_TMO_NS_NUM
> +       CMD_LINE_OPT_VECTOR_TMO_NS_NUM,
> +       CMD_LINE_OPT_MBUF_SIZE_NUM,
>  };
>
>  static const struct option lgopts[] = {
> @@ -753,6 +757,7 @@ static const struct option lgopts[] = {
>         {CMD_LINE_OPT_RULE_IPV4,   1, 0, CMD_LINE_OPT_RULE_IPV4_NUM},
>         {CMD_LINE_OPT_RULE_IPV6,   1, 0, CMD_LINE_OPT_RULE_IPV6_NUM},
>         {CMD_LINE_OPT_ALG,   1, 0, CMD_LINE_OPT_ALG_NUM},
> +       {CMD_LINE_OPT_MBUF_SIZE, 1, 0, CMD_LINE_OPT_MBUF_SIZE_NUM},
>         {NULL, 0, 0, 0}
>  };
>
> @@ -934,6 +939,12 @@ parse_args(int argc, char **argv)
>                 case CMD_LINE_OPT_ALG_NUM:
>                         l3fwd_set_alg(optarg);
>                         break;
> +               case CMD_LINE_OPT_MBUF_SIZE_NUM:
> +                       mbuf_seg_size = strtoul(optarg, NULL, 10) +
> RTE_PKTMBUF_HEADROOM;
> +                       if (mbuf_seg_size <= 0 || mbuf_seg_size > 0xFFFF)
> +                               rte_exit(EXIT_FAILURE,
> +                                               "mbuf-size should be > 0
> and < 65536\n");
> +                       break;
>                 default:
>                         print_usage(prgname);
>                         return -1;
> @@ -1034,7 +1045,7 @@ init_mem(uint16_t portid, unsigned int nb_mbuf)
>                         pktmbuf_pool[portid][socketid] =
>                                 rte_pktmbuf_pool_create(s, nb_mbuf,
>                                         MEMPOOL_CACHE_SIZE, 0,
> -                                       RTE_MBUF_DEFAULT_BUF_SIZE,
> socketid);
> +                                       mbuf_seg_size, socketid);
>                         if (pktmbuf_pool[portid][socketid] == NULL)
>                                 rte_exit(EXIT_FAILURE,
>                                         "Cannot init mbuf pool on socket
> %d\n",
> --
> 2.39.1
>
>

[-- Attachment #2: Type: text/html, Size: 6168 bytes --]

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

[Stephen Hemminger]

On Wed, 16 Oct 2024 16:22:32 +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>

Patch has build failures

*Build Failed #1:
OS: OpenAnolis8.9-64
Target: x86_64-native-linuxapp-gcc
FAILED: examples/dpdk-l3fwd.p/l3fwd_main.c.o 
gcc -Iexamples/dpdk-l3fwd.p -Iexamples -I../examples -Iexamples/l3fwd -I../examples/l3fwd -I../examples/common -I. -I.. -Iconfig -I../config -Ilib/eal/include -I../lib/eal/include -Ilib/eal/linux/include -I../lib/eal/linux/include -Ilib/eal/x86/include -I../lib/eal/x86/include -Ilib/eal/common -I../lib/eal/common -Ilib/eal -I../lib/eal -Ilib/kvargs -I../lib/kvargs -Ilib/log -I../lib/log -Ilib/metrics -I../lib/metrics -Ilib/telemetry -I../lib/telemetry -Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf -Ilib/ethdev -I../lib/ethdev -Ilib/meter -I../lib/meter -Ilib/cmdline -I../lib/cmdline -Ilib/acl -I../lib/acl -Ilib/hash -I../lib/hash -Ilib/rcu -I../lib/rcu -Ilib/lpm -I../lib/lpm -Ilib/fib -I../lib/fib -Ilib/rib -I../lib/rib -Ilib/eventdev -I../lib/eventdev -Ilib/timer -I../lib/timer -Ilib/cryptodev -I../lib/cryptodev -Ilib/dmadev -I../lib/dmadev -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -O3 -include rte_config.h -Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare -Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned -Wno-missing-field-initializers -D_GNU_SOURCE -march=native -mrtm -Wno-format-truncation -DALLOW_EXPERIMENTAL_API -MD -MQ examples/dpdk-l3fwd.p/l3fwd_main.c.o -MF examples/dpdk-l3fwd.p/l3fwd_main.c.o.d -o examples/dpdk-l3fwd.p/l3fwd_main.c.o -c ../examples/l3fwd/main.c
../examples/l3fwd/main.c: In function ‘parse_args’:
../examples/l3fwd/main.c:944:44: error: comparison is always false due to limited range of data type [-Werror=type-limits]
    if (mbuf_seg_size <= 0 || mbuf_seg_size > 0xFFFF)
                                            ^
cc1: all warnings being treated as errors

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

[Chaoyong He]

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)
+		rte_exit(EXIT_FAILURE,
+				"mbuf-size should be >= %u and <= %u\n",
+				RTE_ETHER_MIN_LEN, 0xFFFF - RTE_PKTMBUF_HEADROOM);
+}
+
 #define MAX_JUMBO_PKT_LEN  9600
 
 static const char short_options[] =
@@ -698,6 +716,7 @@ static const char short_options[] =
 #define CMD_LINE_OPT_RULE_IPV4 "rule_ipv4"
 #define CMD_LINE_OPT_RULE_IPV6 "rule_ipv6"
 #define CMD_LINE_OPT_ALG "alg"
+#define CMD_LINE_OPT_MBUF_SIZE "mbuf-size"
 
 enum {
 	/* long options mapped to a short option */
@@ -726,7 +745,8 @@ enum {
 	CMD_LINE_OPT_LOOKUP_NUM,
 	CMD_LINE_OPT_ENABLE_VECTOR_NUM,
 	CMD_LINE_OPT_VECTOR_SIZE_NUM,
-	CMD_LINE_OPT_VECTOR_TMO_NS_NUM
+	CMD_LINE_OPT_VECTOR_TMO_NS_NUM,
+	CMD_LINE_OPT_MBUF_SIZE_NUM,
 };
 
 static const struct option lgopts[] = {
@@ -753,6 +773,7 @@ static const struct option lgopts[] = {
 	{CMD_LINE_OPT_RULE_IPV4,   1, 0, CMD_LINE_OPT_RULE_IPV4_NUM},
 	{CMD_LINE_OPT_RULE_IPV6,   1, 0, CMD_LINE_OPT_RULE_IPV6_NUM},
 	{CMD_LINE_OPT_ALG,   1, 0, CMD_LINE_OPT_ALG_NUM},
+	{CMD_LINE_OPT_MBUF_SIZE, 1, 0, CMD_LINE_OPT_MBUF_SIZE_NUM},
 	{NULL, 0, 0, 0}
 };
 
@@ -934,6 +955,9 @@ parse_args(int argc, char **argv)
 		case CMD_LINE_OPT_ALG_NUM:
 			l3fwd_set_alg(optarg);
 			break;
+		case CMD_LINE_OPT_MBUF_SIZE_NUM:
+			parse_mbuf_data_size(optarg);
+			break;
 		default:
 			print_usage(prgname);
 			return -1;
@@ -1034,7 +1058,8 @@ init_mem(uint16_t portid, unsigned int nb_mbuf)
 			pktmbuf_pool[portid][socketid] =
 				rte_pktmbuf_pool_create(s, nb_mbuf,
 					MEMPOOL_CACHE_SIZE, 0,
-					RTE_MBUF_DEFAULT_BUF_SIZE, socketid);
+					mbuf_data_size + RTE_PKTMBUF_HEADROOM,
+					socketid);
 			if (pktmbuf_pool[portid][socketid] == NULL)
 				rte_exit(EXIT_FAILURE,
 					"Cannot init mbuf pool on socket %d\n",
-- 
2.39.1

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

[lihuisong (C)]

在 2024/10/18 10:42, Chaoyong He 写道:
> 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>
>

Acked-by: Huisong Li<lihuisong@huawei.com>

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

[Stephen Hemminger]

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?

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

[Chaoyong He]

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

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

[Stephen Hemminger]

On Fri, 18 Oct 2024 03:21:28 +0000
Chaoyong He <chaoyong.he@corigine.com> wrote:

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

Maybe query the driver, and use the max_rx_pkt_len as input to deciding the right mbuf size.
If max-pkt-len was 4000 and driver can only take 2K buffers, then use 2K mbuf size.
If max-pkt-len was 1500 then use mtu + headroom and round up

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

[Chaoyong He]

> On Fri, 18 Oct 2024 03:21:28 +0000
> Chaoyong He <chaoyong.he@corigine.com> wrote:
> 
> > > 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?
> 
> Maybe query the driver, and use the max_rx_pkt_len as input to deciding the
> right mbuf size.

Sorry, I am not quite understanding here.
I can't find 'max_rx_pkt_len' in l3fwd app, instead it's exist testpmd app.
Could you please explain a little more about the advice?

> If max-pkt-len was 4000 and driver can only take 2K buffers, then use 2K mbuf
> size.
> If max-pkt-len was 1500 then use mtu + headroom and round up

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

[Stephen Hemminger]

On Fri, 18 Oct 2024 05:50:20 +0000
Chaoyong He <chaoyong.he@corigine.com> wrote:

> > On Fri, 18 Oct 2024 03:21:28 +0000
> > Chaoyong He <chaoyong.he@corigine.com> wrote:
> >   
> > > > 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?  
> > 
> > Maybe query the driver, and use the max_rx_pkt_len as input to deciding the
> > right mbuf size.  
> 
> Sorry, I am not quite understanding here.
> I can't find 'max_rx_pkt_len' in l3fwd app, instead it's exist testpmd app.
> Could you please explain a little more about the advice?

In rte_eth_dev_info, I meant the field max_rx_bufsize and there is also max_rx_pktlen.

> 
> > If max-pkt-len was 4000 and driver can only take 2K buffers, then use 2K mbuf
> > size.
> > If max-pkt-len was 1500 then use mtu + headroom and round up  

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

[Chaoyong He]

> On Fri, 18 Oct 2024 05:50:20 +0000
> Chaoyong He <chaoyong.he@corigine.com> wrote:
> 
> > > On Fri, 18 Oct 2024 03:21:28 +0000
> > > Chaoyong He <chaoyong.he@corigine.com> wrote:
> > >
> > > > > 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?
> > >
> > > Maybe query the driver, and use the max_rx_pkt_len as input to
> > > deciding the right mbuf size.
> >
> > Sorry, I am not quite understanding here.
> > I can't find 'max_rx_pkt_len' in l3fwd app, instead it's exist testpmd app.
> > Could you please explain a little more about the advice?
> 
> In rte_eth_dev_info, I meant the field max_rx_bufsize and there is also
> max_rx_pktlen.
> 
> >
> > > If max-pkt-len was 4000 and driver can only take 2K buffers, then
> > > use 2K mbuf size.
> > > If max-pkt-len was 1500 then use mtu + headroom and round up

Oh, I understand what you mean now, thanks for the clarification.
But the solution you suppose is not flexible enough, thus can't satisfy our needs.

Follow your example and consider this situation:
If max-pkt-len was 4000 and driver can only take 2K buffers, then
use 2K mbuf size.

But we want to measure the performance when the mbuf size is 1024 and 512.

Then there is no way to do this in your solution, I suppose?

But with our '--mbuf-size' parameter, we can easily do that.
Thanks for your hint, we realized our solution also has a little problem, which not 
consider the 'max_rx_bufsize' of rte_eth_dev_info, and we will fix that in the next version patch.

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

[Stephen Hemminger]

On Mon, 21 Oct 2024 02:00:55 +0000
Chaoyong He <chaoyong.he@corigine.com> wrote:

> > On Fri, 18 Oct 2024 05:50:20 +0000
> > Chaoyong He <chaoyong.he@corigine.com> wrote:
> >   
> > > > On Fri, 18 Oct 2024 03:21:28 +0000
> > > > Chaoyong He <chaoyong.he@corigine.com> wrote:
> > > >  
> > > > > > 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?  
> > > >
> > > > Maybe query the driver, and use the max_rx_pkt_len as input to
> > > > deciding the right mbuf size.  
> > >
> > > Sorry, I am not quite understanding here.
> > > I can't find 'max_rx_pkt_len' in l3fwd app, instead it's exist testpmd app.
> > > Could you please explain a little more about the advice?  
> > 
> > In rte_eth_dev_info, I meant the field max_rx_bufsize and there is also
> > max_rx_pktlen.
> >   
> > >  
> > > > If max-pkt-len was 4000 and driver can only take 2K buffers, then
> > > > use 2K mbuf size.
> > > > If max-pkt-len was 1500 then use mtu + headroom and round up  
> 
> Oh, I understand what you mean now, thanks for the clarification.
> But the solution you suppose is not flexible enough, thus can't satisfy our needs.
> 
> Follow your example and consider this situation:
> If max-pkt-len was 4000 and driver can only take 2K buffers, then
> use 2K mbuf size.
> 
> But we want to measure the performance when the mbuf size is 1024 and 512.
> 
> Then there is no way to do this in your solution, I suppose?
> 
> But with our '--mbuf-size' parameter, we can easily do that.
> Thanks for your hint, we realized our solution also has a little problem, which not 
> consider the 'max_rx_bufsize' of rte_eth_dev_info, and we will fix that in the next version patch.

It would be best if the default was to choose mbuf size automatically,
but since this is a test program having an override is also useful.

In general l3fwd is target at usability (less options) and test-pmd is focused on testing (lots of options).
There is a tradeoff here. l3fwd should not get as complex as test-pmd or there is no point.