[dpdk-dev] [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

[dpdk-dev] [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

[Vamsi Attunuru]

"argv2[]" positive test case fails with RTE_MAX_NUMA_NODES=1 config
because of "--socket-mem=0,0,0,0" option, which passes memory sizes
for multiple sockets. This patch fixes the issue by passing memory
size for node 0 alone.

Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
---
 app/test/test_eal_flags.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index 9112c96..90df7b3 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -1161,7 +1161,7 @@ test_memory_flags(void)
 
 	/* valid (zero) --socket-mem flag */
 	const char *argv2[] = {prgname, "-c", "10", "-n", "2",
-			"--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
+			"--file-prefix=" memtest, "--socket-mem=0"};
 
 	/* invalid (incomplete) --socket-mem flag */
 	const char *argv3[] = {prgname, "-c", "10", "-n", "2",
-- 
2.8.4

[dpdk-dev] [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

[Vamsi Attunuru]

"argv2[]" positive test case fails with RTE_MAX_NUMA_NODES=1 config
because of "--socket-mem=0,0,0,0" option, which passes memory sizes
for multiple sockets. This patch fixes the issue by passing memory
size for node 0 alone.

Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
---
 app/test/test_eal_flags.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index 9112c96..90df7b3 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -1161,7 +1161,7 @@ test_memory_flags(void)
 
 	/* valid (zero) --socket-mem flag */
 	const char *argv2[] = {prgname, "-c", "10", "-n", "2",
-			"--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
+			"--file-prefix=" memtest, "--socket-mem=0"};
 
 	/* invalid (incomplete) --socket-mem flag */
 	const char *argv3[] = {prgname, "-c", "10", "-n", "2",
-- 
2.8.4

Re: [dpdk-dev] [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

[David Marchand]

On Wed, Apr 10, 2019 at 9:26 AM Vamsi Attunuru <vattunuru@marvell.com>
wrote:

> "argv2[]" positive test case fails with RTE_MAX_NUMA_NODES=1 config
> because of "--socket-mem=0,0,0,0" option, which passes memory sizes
> for multiple sockets. This patch fixes the issue by passing memory
> size for node 0 alone.
>

How about modifying the test so that it also validates the format is
consistent with the RTE_MAX_NUMA_NODES value ?


-- 
David Marchand

Re: [dpdk-dev] [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

[David Marchand]

On Wed, Apr 10, 2019 at 9:26 AM Vamsi Attunuru <vattunuru@marvell.com>
wrote:

> "argv2[]" positive test case fails with RTE_MAX_NUMA_NODES=1 config
> because of "--socket-mem=0,0,0,0" option, which passes memory sizes
> for multiple sockets. This patch fixes the issue by passing memory
> size for node 0 alone.
>

How about modifying the test so that it also validates the format is
consistent with the RTE_MAX_NUMA_NODES value ?


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

[Vamsi Krishna Attunuru]

________________________________
From: David Marchand <david.marchand@redhat.com>
Sent: Wednesday, April 10, 2019 1:56 PM
To: Vamsi Krishna Attunuru
Cc: dev; Michael Santana; Aaron Conole
Subject: [EXT] Re: [dpdk-dev] [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

External Email
________________________________
On Wed, Apr 10, 2019 at 9:26 AM Vamsi Attunuru <vattunuru@marvell.com<mailto:vattunuru@marvell.com>> wrote:
"argv2[]" positive test case fails with RTE_MAX_NUMA_NODES=1 config
because of "--socket-mem=0,0,0,0" option, which passes memory sizes
for multiple sockets. This patch fixes the issue by passing memory
size for node 0 alone.

How about modifying the test so that it also validates the format is consistent with the RTE_MAX_NUMA_NODES value ?

Vamsi > Yes, started working on it, it should also fix other negative tests(false negative) beneath argv2[] test.


--
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

[Vamsi Krishna Attunuru]

________________________________
From: David Marchand <david.marchand@redhat.com>
Sent: Wednesday, April 10, 2019 1:56 PM
To: Vamsi Krishna Attunuru
Cc: dev; Michael Santana; Aaron Conole
Subject: [EXT] Re: [dpdk-dev] [PATCH v1 1/1] app/test: fix --socket-mem option in eal flag autotest

External Email
________________________________
On Wed, Apr 10, 2019 at 9:26 AM Vamsi Attunuru <vattunuru@marvell.com<mailto:vattunuru@marvell.com>> wrote:
"argv2[]" positive test case fails with RTE_MAX_NUMA_NODES=1 config
because of "--socket-mem=0,0,0,0" option, which passes memory sizes
for multiple sockets. This patch fixes the issue by passing memory
size for node 0 alone.

How about modifying the test so that it also validates the format is consistent with the RTE_MAX_NUMA_NODES value ?

Vamsi > Yes, started working on it, it should also fix other negative tests(false negative) beneath argv2[] test.


--
David Marchand

[dpdk-dev] [PATCH v2 1/1] app/test: fix --socket-mem option in eal flag autotest

[vattunuru]

From: Vamsi Attunuru <vattunuru@marvell.com>

eal flag autotest fails when multiple mem size flags are passed
to --socket-mem option irrespective of RTE_MAX_NUMA_NODES.

Patch fixes --socket-mem option by setting enough numbers
of numa node mem flags based on RTE_MAX_NUMA_NODES config.

Fixes: 45f1b6e8680a ("app: add new tests on eal flags")

Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
---
V2 changes:
* Add routine to populate --socket-mem option and use it to form
valid and invalid test commands.

 app/test/test_eal_flags.c | 118 ++++++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 50 deletions(-)

diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index 672ca0a..c8e5c46 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -1250,12 +1250,45 @@ test_file_prefix(void)
 	return 0;
 }
 
+static void
+populate_socket_mem_param(int num_sockets, const char *mem_size, char *buf)
+{
+	char tmp[SOCKET_MEM_STRLEN];
+	int i;
+
+	snprintf(buf, SOCKET_MEM_STRLEN, "--socket-mem=");
+
+	for (i = 0; i < num_sockets ; i++) {
+		snprintf(tmp, sizeof(tmp), "%s%s", buf, mem_size);
+		strlcpy(buf, tmp, sizeof(tmp));
+
+		if (num_sockets + 1 - i > 1) {
+			snprintf(tmp, sizeof(tmp), "%s,", buf);
+			strlcpy(buf, tmp, sizeof(tmp));
+		}
+	}
+}
+
 /*
  * Tests for correct handling of -m and --socket-mem flags
  */
 static int
 test_memory_flags(void)
 {
+	char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto itself */
+
+#ifdef RTE_EXEC_ENV_FREEBSD
+	int num_sockets = 1;
+#else
+	int num_sockets = RTE_MIN(get_number_of_sockets(),
+			RTE_MAX_NUMA_NODES);
+#endif
+
+	if (num_sockets <= 0) {
+		printf("Error - cannot get number of sockets!\n");
+		return -1;
+	}
+
 #ifdef RTE_EXEC_ENV_FREEBSD
 	/* BSD target doesn't support prefixes at this point */
 	const char * prefix = "";
@@ -1277,85 +1310,70 @@ test_memory_flags(void)
 			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE};
 
 	/* valid (zero) --socket-mem flag */
+	char arg2_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "0", buf);
+	snprintf(arg2_socket_mem, sizeof(arg2_socket_mem), "%s%s", buf, "0");
 	const char *argv2[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
+			"--file-prefix=" memtest, arg2_socket_mem};
 
 	/* invalid (incomplete) --socket-mem flag */
+	char arg3_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf);
+	snprintf(arg3_socket_mem, sizeof(arg3_socket_mem), "%s%s", buf, "2,");
 	const char *argv3[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,"};
+			"--file-prefix=" memtest, arg3_socket_mem};
 
 	/* invalid (mixed with invalid data) --socket-mem flag */
+	char arg4_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf);
+	snprintf(arg4_socket_mem, sizeof(arg4_socket_mem), "%s%s", buf, "Fred");
 	const char *argv4[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,Fred"};
+			"--file-prefix=" memtest, arg4_socket_mem};
 
 	/* invalid (with numeric value as last character) --socket-mem flag */
+	char arg5_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf);
+	snprintf(arg5_socket_mem, sizeof(arg5_socket_mem),
+		"%s%s", buf, "Fred0");
 	const char *argv5[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,Fred0"};
+			"--file-prefix=" memtest, arg5_socket_mem};
 
 	/* invalid (with empty socket) --socket-mem flag */
+	char arg6_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf);
+	snprintf(arg6_socket_mem, sizeof(arg6_socket_mem), "%s%s", buf, ",,");
 	const char *argv6[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,,2"};
+			"--file-prefix=" memtest, arg6_socket_mem};
 
 	/* invalid (null) --socket-mem flag */
 	const char *argv7[] = {prgname,
 			"--file-prefix=" memtest, "--socket-mem="};
 
 	/* valid --socket-mem specified together with -m flag */
+	char arg8_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf);
+	snprintf(arg8_socket_mem, sizeof(arg8_socket_mem), "%s%s", buf, "2");
 	const char *argv8[] = {prgname,
-			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "--socket-mem=2,2"};
+			"--file-prefix=" memtest, "-m",
+			DEFAULT_MEM_SIZE, arg8_socket_mem};
 
 	/* construct an invalid socket mask with 2 megs on each socket plus
 	 * extra 2 megs on socket that doesn't exist on current system */
 	char invalid_socket_mem[SOCKET_MEM_STRLEN];
-	char buf[SOCKET_MEM_STRLEN];	/* to avoid copying string onto itself */
-
-#ifdef RTE_EXEC_ENV_FREEBSD
-	int i, num_sockets = 1;
-#else
-	int i, num_sockets = RTE_MIN(get_number_of_sockets(),
-			RTE_MAX_NUMA_NODES);
-#endif
-
-	if (num_sockets <= 0) {
-		printf("Error - cannot get number of sockets!\n");
-		return -1;
-	}
-
-	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-mem=");
-
-	/* add one extra socket */
-	for (i = 0; i < num_sockets + 1; i++) {
-		snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
-		strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
-
-		if (num_sockets + 1 - i > 1) {
-			snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
-			strlcpy(invalid_socket_mem, buf,
-				sizeof(invalid_socket_mem));
-		}
-	}
-
-	/* construct a valid socket mask with 2 megs on each existing socket */
-	char valid_socket_mem[SOCKET_MEM_STRLEN];
-
-	snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-mem=");
-
-	/* add one extra socket */
-	for (i = 0; i < num_sockets; i++) {
-		snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
-		strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
-
-		if (num_sockets - i > 1) {
-			snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
-			strlcpy(valid_socket_mem, buf,
-				sizeof(valid_socket_mem));
-		}
-	}
+	populate_socket_mem_param(num_sockets, DEFAULT_MEM_SIZE, buf);
+	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem),
+		"%s%s", buf, DEFAULT_MEM_SIZE);
 
 	/* invalid --socket-mem flag (with extra socket) */
 	const char *argv9[] = {prgname,
 			"--file-prefix=" memtest, invalid_socket_mem};
 
+	/* construct a valid socket mask with 2 megs on each existing socket */
+	char valid_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, DEFAULT_MEM_SIZE, buf);
+	snprintf(valid_socket_mem, sizeof(valid_socket_mem),
+		"%s%s", buf, DEFAULT_MEM_SIZE);
+
 	/* valid --socket-mem flag */
 	const char *argv10[] = {prgname,
 			"--file-prefix=" memtest, valid_socket_mem};
-- 
2.8.4

Re: [dpdk-dev] [PATCH v2 1/1] app/test: fix --socket-mem option in eal flag autotest

[Aaron Conole]

<vattunuru@marvell.com> writes:

> From: Vamsi Attunuru <vattunuru@marvell.com>
>
> eal flag autotest fails when multiple mem size flags are passed
> to --socket-mem option irrespective of RTE_MAX_NUMA_NODES.
>
> Patch fixes --socket-mem option by setting enough numbers
> of numa node mem flags based on RTE_MAX_NUMA_NODES config.
>
> Fixes: 45f1b6e8680a ("app: add new tests on eal flags")
>
> Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> ---
> V2 changes:
> * Add routine to populate --socket-mem option and use it to form
> valid and invalid test commands.
>
>  app/test/test_eal_flags.c | 118 ++++++++++++++++++++++++++--------------------
>  1 file changed, 68 insertions(+), 50 deletions(-)
>
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 672ca0a..c8e5c46 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -1250,12 +1250,45 @@ test_file_prefix(void)
>  	return 0;
>  }
>  
> +static void
> +populate_socket_mem_param(int num_sockets, const char *mem_size, char *buf)
> +{
> +	char tmp[SOCKET_MEM_STRLEN];
> +	int i;
> +
> +	snprintf(buf, SOCKET_MEM_STRLEN, "--socket-mem=");
> +
> +	for (i = 0; i < num_sockets ; i++) {
> +		snprintf(tmp, sizeof(tmp), "%s%s", buf, mem_size);
> +		strlcpy(buf, tmp, sizeof(tmp));

strlcpy requires the 3rd parameter be the size of the destination not
the size of the source.

You'll need to pass sizeof(buf) into this function.

Same goes for everywhere strlcpy is used.

> +		if (num_sockets + 1 - i > 1) {
> +			snprintf(tmp, sizeof(tmp), "%s,", buf);
> +			strlcpy(buf, tmp, sizeof(tmp));
> +		}
> +	}
> +}
> +
>  /*
>   * Tests for correct handling of -m and --socket-mem flags
>   */
>  static int
>  test_memory_flags(void)
>  {
> +	char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto itself */
> +
> +#ifdef RTE_EXEC_ENV_FREEBSD
> +	int num_sockets = 1;
> +#else
> +	int num_sockets = RTE_MIN(get_number_of_sockets(),
> +			RTE_MAX_NUMA_NODES);
> +#endif
> +
> +	if (num_sockets <= 0) {
> +		printf("Error - cannot get number of sockets!\n");
> +		return -1;
> +	}
> +
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  	/* BSD target doesn't support prefixes at this point */
>  	const char * prefix = "";
> @@ -1277,85 +1310,70 @@ test_memory_flags(void)
>  			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE};
>  
>  	/* valid (zero) --socket-mem flag */
> +	char arg2_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "0", buf);
> +	snprintf(arg2_socket_mem, sizeof(arg2_socket_mem), "%s%s", buf, "0");
>  	const char *argv2[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
> +			"--file-prefix=" memtest, arg2_socket_mem};
>  
>  	/* invalid (incomplete) --socket-mem flag */
> +	char arg3_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf);
> +	snprintf(arg3_socket_mem, sizeof(arg3_socket_mem), "%s%s", buf, "2,");
>  	const char *argv3[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,2,"};
> +			"--file-prefix=" memtest, arg3_socket_mem};
>  
>  	/* invalid (mixed with invalid data) --socket-mem flag */
> +	char arg4_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf);
> +	snprintf(arg4_socket_mem, sizeof(arg4_socket_mem), "%s%s", buf, "Fred");
>  	const char *argv4[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,2,Fred"};
> +			"--file-prefix=" memtest, arg4_socket_mem};
>  
>  	/* invalid (with numeric value as last character) --socket-mem flag */
> +	char arg5_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf);
> +	snprintf(arg5_socket_mem, sizeof(arg5_socket_mem),
> +		"%s%s", buf, "Fred0");
>  	const char *argv5[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,2,Fred0"};
> +			"--file-prefix=" memtest, arg5_socket_mem};
>  
>  	/* invalid (with empty socket) --socket-mem flag */
> +	char arg6_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf);
> +	snprintf(arg6_socket_mem, sizeof(arg6_socket_mem), "%s%s", buf, ",,");
>  	const char *argv6[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,,2"};
> +			"--file-prefix=" memtest, arg6_socket_mem};
>  
>  	/* invalid (null) --socket-mem flag */
>  	const char *argv7[] = {prgname,
>  			"--file-prefix=" memtest, "--socket-mem="};
>  
>  	/* valid --socket-mem specified together with -m flag */
> +	char arg8_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf);
> +	snprintf(arg8_socket_mem, sizeof(arg8_socket_mem), "%s%s", buf, "2");
>  	const char *argv8[] = {prgname,
> -			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "--socket-mem=2,2"};
> +			"--file-prefix=" memtest, "-m",
> +			DEFAULT_MEM_SIZE, arg8_socket_mem};
>  
>  	/* construct an invalid socket mask with 2 megs on each socket plus
>  	 * extra 2 megs on socket that doesn't exist on current system */
>  	char invalid_socket_mem[SOCKET_MEM_STRLEN];
> -	char buf[SOCKET_MEM_STRLEN];	/* to avoid copying string onto itself */
> -
> -#ifdef RTE_EXEC_ENV_FREEBSD
> -	int i, num_sockets = 1;
> -#else
> -	int i, num_sockets = RTE_MIN(get_number_of_sockets(),
> -			RTE_MAX_NUMA_NODES);
> -#endif
> -
> -	if (num_sockets <= 0) {
> -		printf("Error - cannot get number of sockets!\n");
> -		return -1;
> -	}
> -
> -	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-mem=");
> -
> -	/* add one extra socket */
> -	for (i = 0; i < num_sockets + 1; i++) {
> -		snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
> -		strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
> -
> -		if (num_sockets + 1 - i > 1) {
> -			snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
> -			strlcpy(invalid_socket_mem, buf,
> -				sizeof(invalid_socket_mem));
> -		}
> -	}
> -
> -	/* construct a valid socket mask with 2 megs on each existing socket */
> -	char valid_socket_mem[SOCKET_MEM_STRLEN];
> -
> -	snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-mem=");
> -
> -	/* add one extra socket */
> -	for (i = 0; i < num_sockets; i++) {
> -		snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
> -		strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
> -
> -		if (num_sockets - i > 1) {
> -			snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
> -			strlcpy(valid_socket_mem, buf,
> -				sizeof(valid_socket_mem));
> -		}
> -	}
> +	populate_socket_mem_param(num_sockets, DEFAULT_MEM_SIZE, buf);
> +	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem),
> +		"%s%s", buf, DEFAULT_MEM_SIZE);
>  
>  	/* invalid --socket-mem flag (with extra socket) */
>  	const char *argv9[] = {prgname,
>  			"--file-prefix=" memtest, invalid_socket_mem};
>  
> +	/* construct a valid socket mask with 2 megs on each existing socket */
> +	char valid_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, DEFAULT_MEM_SIZE, buf);
> +	snprintf(valid_socket_mem, sizeof(valid_socket_mem),
> +		"%s%s", buf, DEFAULT_MEM_SIZE);
> +
>  	/* valid --socket-mem flag */
>  	const char *argv10[] = {prgname,
>  			"--file-prefix=" memtest, valid_socket_mem};

[dpdk-dev] [PATCH v3 1/1] app/test: fix --socket-mem option in eal flag autotest

[vattunuru]

From: Vamsi Attunuru <vattunuru@marvell.com>

eal flag autotest fails when multiple mem size flags are passed
to --socket-mem option irrespective of RTE_MAX_NUMA_NODES.

Patch fixes --socket-mem option by setting enough numbers
of numa node mem flags based on RTE_MAX_NUMA_NODES config.

Fixes: 45f1b6e8680a ("app: add new tests on eal flags")

Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
---
V3 changes:
* Pass buffer size info to populate socket mem routine which
strlcpy requires.

V2 changes:
* Add routine to populate --socket-mem option and use it to form
valid and invalid test commands.

 app/test/test_eal_flags.c | 121 +++++++++++++++++++++++++++-------------------
 1 file changed, 71 insertions(+), 50 deletions(-)

diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index 672ca0a..053d694 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -1250,12 +1250,46 @@ test_file_prefix(void)
 	return 0;
 }
 
+static void
+populate_socket_mem_param(int num_sockets, const char *mem,
+			 char *buf, size_t buf_size)
+{
+	char tmp[SOCKET_MEM_STRLEN];
+	int i;
+
+	snprintf(buf, SOCKET_MEM_STRLEN, "--socket-mem=");
+
+	for (i = 0; i < num_sockets ; i++) {
+		snprintf(tmp, sizeof(tmp), "%s%s", buf, mem);
+		strlcpy(buf, tmp, buf_size);
+
+		if (num_sockets + 1 - i > 1) {
+			snprintf(tmp, sizeof(tmp), "%s,", buf);
+			strlcpy(buf, tmp, buf_size);
+		}
+	}
+}
+
 /*
  * Tests for correct handling of -m and --socket-mem flags
  */
 static int
 test_memory_flags(void)
 {
+	char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto itself */
+
+#ifdef RTE_EXEC_ENV_FREEBSD
+	int num_sockets = 1;
+#else
+	int num_sockets = RTE_MIN(get_number_of_sockets(),
+			RTE_MAX_NUMA_NODES);
+#endif
+
+	if (num_sockets <= 0) {
+		printf("Error - cannot get number of sockets!\n");
+		return -1;
+	}
+
 #ifdef RTE_EXEC_ENV_FREEBSD
 	/* BSD target doesn't support prefixes at this point */
 	const char * prefix = "";
@@ -1277,85 +1311,72 @@ test_memory_flags(void)
 			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE};
 
 	/* valid (zero) --socket-mem flag */
+	char arg2_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "0", buf, sizeof(buf));
+	snprintf(arg2_socket_mem, sizeof(arg2_socket_mem), "%s%s", buf, "0");
 	const char *argv2[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
+			"--file-prefix=" memtest, arg2_socket_mem};
 
 	/* invalid (incomplete) --socket-mem flag */
+	char arg3_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
+	snprintf(arg3_socket_mem, sizeof(arg3_socket_mem), "%s%s", buf, "2,");
 	const char *argv3[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,"};
+			"--file-prefix=" memtest, arg3_socket_mem};
 
 	/* invalid (mixed with invalid data) --socket-mem flag */
+	char arg4_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
+	snprintf(arg4_socket_mem, sizeof(arg4_socket_mem), "%s%s", buf, "Fred");
 	const char *argv4[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,Fred"};
+			"--file-prefix=" memtest, arg4_socket_mem};
 
 	/* invalid (with numeric value as last character) --socket-mem flag */
+	char arg5_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
+	snprintf(arg5_socket_mem, sizeof(arg5_socket_mem),
+		"%s%s", buf, "Fred0");
 	const char *argv5[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,Fred0"};
+			"--file-prefix=" memtest, arg5_socket_mem};
 
 	/* invalid (with empty socket) --socket-mem flag */
+	char arg6_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
+	snprintf(arg6_socket_mem, sizeof(arg6_socket_mem), "%s%s", buf, ",,");
 	const char *argv6[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,,2"};
+			"--file-prefix=" memtest, arg6_socket_mem};
 
 	/* invalid (null) --socket-mem flag */
 	const char *argv7[] = {prgname,
 			"--file-prefix=" memtest, "--socket-mem="};
 
 	/* valid --socket-mem specified together with -m flag */
+	char arg8_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
+	snprintf(arg8_socket_mem, sizeof(arg8_socket_mem), "%s%s", buf, "2");
 	const char *argv8[] = {prgname,
-			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "--socket-mem=2,2"};
+			"--file-prefix=" memtest, "-m",
+			DEFAULT_MEM_SIZE, arg8_socket_mem};
 
 	/* construct an invalid socket mask with 2 megs on each socket plus
 	 * extra 2 megs on socket that doesn't exist on current system */
 	char invalid_socket_mem[SOCKET_MEM_STRLEN];
-	char buf[SOCKET_MEM_STRLEN];	/* to avoid copying string onto itself */
-
-#ifdef RTE_EXEC_ENV_FREEBSD
-	int i, num_sockets = 1;
-#else
-	int i, num_sockets = RTE_MIN(get_number_of_sockets(),
-			RTE_MAX_NUMA_NODES);
-#endif
-
-	if (num_sockets <= 0) {
-		printf("Error - cannot get number of sockets!\n");
-		return -1;
-	}
-
-	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-mem=");
-
-	/* add one extra socket */
-	for (i = 0; i < num_sockets + 1; i++) {
-		snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
-		strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
-
-		if (num_sockets + 1 - i > 1) {
-			snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
-			strlcpy(invalid_socket_mem, buf,
-				sizeof(invalid_socket_mem));
-		}
-	}
-
-	/* construct a valid socket mask with 2 megs on each existing socket */
-	char valid_socket_mem[SOCKET_MEM_STRLEN];
-
-	snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-mem=");
-
-	/* add one extra socket */
-	for (i = 0; i < num_sockets; i++) {
-		snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
-		strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
-
-		if (num_sockets - i > 1) {
-			snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
-			strlcpy(valid_socket_mem, buf,
-				sizeof(valid_socket_mem));
-		}
-	}
+	populate_socket_mem_param(num_sockets, DEFAULT_MEM_SIZE,
+				 buf, sizeof(buf));
+	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem),
+		"%s%s", buf, DEFAULT_MEM_SIZE);
 
 	/* invalid --socket-mem flag (with extra socket) */
 	const char *argv9[] = {prgname,
 			"--file-prefix=" memtest, invalid_socket_mem};
 
+	/* construct a valid socket mask with 2 megs on each existing socket */
+	char valid_socket_mem[SOCKET_MEM_STRLEN];
+	populate_socket_mem_param(num_sockets - 1, DEFAULT_MEM_SIZE,
+				 buf, sizeof(buf));
+	snprintf(valid_socket_mem, sizeof(valid_socket_mem),
+		"%s%s", buf, DEFAULT_MEM_SIZE);
+
 	/* valid --socket-mem flag */
 	const char *argv10[] = {prgname,
 			"--file-prefix=" memtest, valid_socket_mem};
-- 
2.8.4

Re: [dpdk-dev] [PATCH v3 1/1] app/test: fix --socket-mem option in eal flag autotest

[Vamsi Krishna Attunuru]

Ping

V3 review request..

> -----Original Message-----
> From: vattunuru@marvell.com <vattunuru@marvell.com>
> Sent: Friday, July 12, 2019 12:13 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> david.marchand@redhat.com; aconole@redhat.com; Vamsi Krishna Attunuru
> <vattunuru@marvell.com>
> Subject: [PATCH v3 1/1] app/test: fix --socket-mem option in eal flag autotest
> 
> From: Vamsi Attunuru <vattunuru@marvell.com>
> 
> eal flag autotest fails when multiple mem size flags are passed to --socket-mem
> option irrespective of RTE_MAX_NUMA_NODES.
> 
> Patch fixes --socket-mem option by setting enough numbers of numa node mem
> flags based on RTE_MAX_NUMA_NODES config.
> 
> Fixes: 45f1b6e8680a ("app: add new tests on eal flags")
> 
> Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> ---
> V3 changes:
> * Pass buffer size info to populate socket mem routine which strlcpy requires.
> 
> V2 changes:
> * Add routine to populate --socket-mem option and use it to form valid and
> invalid test commands.
> 
>  app/test/test_eal_flags.c | 121 +++++++++++++++++++++++++++------------------
> -
>  1 file changed, 71 insertions(+), 50 deletions(-)
> 
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c index
> 672ca0a..053d694 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -1250,12 +1250,46 @@ test_file_prefix(void)
>  	return 0;
>  }
> 
> +static void
> +populate_socket_mem_param(int num_sockets, const char *mem,
> +			 char *buf, size_t buf_size)
> +{
> +	char tmp[SOCKET_MEM_STRLEN];
> +	int i;
> +
> +	snprintf(buf, SOCKET_MEM_STRLEN, "--socket-mem=");
> +
> +	for (i = 0; i < num_sockets ; i++) {
> +		snprintf(tmp, sizeof(tmp), "%s%s", buf, mem);
> +		strlcpy(buf, tmp, buf_size);
> +
> +		if (num_sockets + 1 - i > 1) {
> +			snprintf(tmp, sizeof(tmp), "%s,", buf);
> +			strlcpy(buf, tmp, buf_size);
> +		}
> +	}
> +}
> +
>  /*
>   * Tests for correct handling of -m and --socket-mem flags
>   */
>  static int
>  test_memory_flags(void)
>  {
> +	char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto itself
> */
> +
> +#ifdef RTE_EXEC_ENV_FREEBSD
> +	int num_sockets = 1;
> +#else
> +	int num_sockets = RTE_MIN(get_number_of_sockets(),
> +			RTE_MAX_NUMA_NODES);
> +#endif
> +
> +	if (num_sockets <= 0) {
> +		printf("Error - cannot get number of sockets!\n");
> +		return -1;
> +	}
> +
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  	/* BSD target doesn't support prefixes at this point */
>  	const char * prefix = "";
> @@ -1277,85 +1311,72 @@ test_memory_flags(void)
>  			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE};
> 
>  	/* valid (zero) --socket-mem flag */
> +	char arg2_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "0", buf, sizeof(buf));
> +	snprintf(arg2_socket_mem, sizeof(arg2_socket_mem), "%s%s", buf,
> "0");
>  	const char *argv2[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
> +			"--file-prefix=" memtest, arg2_socket_mem};
> 
>  	/* invalid (incomplete) --socket-mem flag */
> +	char arg3_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +	snprintf(arg3_socket_mem, sizeof(arg3_socket_mem), "%s%s", buf,
> "2,");
>  	const char *argv3[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,2,"};
> +			"--file-prefix=" memtest, arg3_socket_mem};
> 
>  	/* invalid (mixed with invalid data) --socket-mem flag */
> +	char arg4_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +	snprintf(arg4_socket_mem, sizeof(arg4_socket_mem), "%s%s", buf,
> +"Fred");
>  	const char *argv4[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,2,Fred"};
> +			"--file-prefix=" memtest, arg4_socket_mem};
> 
>  	/* invalid (with numeric value as last character) --socket-mem flag */
> +	char arg5_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +	snprintf(arg5_socket_mem, sizeof(arg5_socket_mem),
> +		"%s%s", buf, "Fred0");
>  	const char *argv5[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,2,Fred0"};
> +			"--file-prefix=" memtest, arg5_socket_mem};
> 
>  	/* invalid (with empty socket) --socket-mem flag */
> +	char arg6_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +	snprintf(arg6_socket_mem, sizeof(arg6_socket_mem), "%s%s", buf,
> ",,");
>  	const char *argv6[] = {prgname,
> -			"--file-prefix=" memtest, "--socket-mem=2,,2"};
> +			"--file-prefix=" memtest, arg6_socket_mem};
> 
>  	/* invalid (null) --socket-mem flag */
>  	const char *argv7[] = {prgname,
>  			"--file-prefix=" memtest, "--socket-mem="};
> 
>  	/* valid --socket-mem specified together with -m flag */
> +	char arg8_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +	snprintf(arg8_socket_mem, sizeof(arg8_socket_mem), "%s%s", buf,
> "2");
>  	const char *argv8[] = {prgname,
> -			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "-
> -socket-mem=2,2"};
> +			"--file-prefix=" memtest, "-m",
> +			DEFAULT_MEM_SIZE, arg8_socket_mem};
> 
>  	/* construct an invalid socket mask with 2 megs on each socket plus
>  	 * extra 2 megs on socket that doesn't exist on current system */
>  	char invalid_socket_mem[SOCKET_MEM_STRLEN];
> -	char buf[SOCKET_MEM_STRLEN];	/* to avoid copying string onto
> itself */
> -
> -#ifdef RTE_EXEC_ENV_FREEBSD
> -	int i, num_sockets = 1;
> -#else
> -	int i, num_sockets = RTE_MIN(get_number_of_sockets(),
> -			RTE_MAX_NUMA_NODES);
> -#endif
> -
> -	if (num_sockets <= 0) {
> -		printf("Error - cannot get number of sockets!\n");
> -		return -1;
> -	}
> -
> -	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-
> mem=");
> -
> -	/* add one extra socket */
> -	for (i = 0; i < num_sockets + 1; i++) {
> -		snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem,
> DEFAULT_MEM_SIZE);
> -		strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
> -
> -		if (num_sockets + 1 - i > 1) {
> -			snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
> -			strlcpy(invalid_socket_mem, buf,
> -				sizeof(invalid_socket_mem));
> -		}
> -	}
> -
> -	/* construct a valid socket mask with 2 megs on each existing socket */
> -	char valid_socket_mem[SOCKET_MEM_STRLEN];
> -
> -	snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-
> mem=");
> -
> -	/* add one extra socket */
> -	for (i = 0; i < num_sockets; i++) {
> -		snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem,
> DEFAULT_MEM_SIZE);
> -		strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
> -
> -		if (num_sockets - i > 1) {
> -			snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
> -			strlcpy(valid_socket_mem, buf,
> -				sizeof(valid_socket_mem));
> -		}
> -	}
> +	populate_socket_mem_param(num_sockets, DEFAULT_MEM_SIZE,
> +				 buf, sizeof(buf));
> +	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem),
> +		"%s%s", buf, DEFAULT_MEM_SIZE);
> 
>  	/* invalid --socket-mem flag (with extra socket) */
>  	const char *argv9[] = {prgname,
>  			"--file-prefix=" memtest, invalid_socket_mem};
> 
> +	/* construct a valid socket mask with 2 megs on each existing socket */
> +	char valid_socket_mem[SOCKET_MEM_STRLEN];
> +	populate_socket_mem_param(num_sockets - 1, DEFAULT_MEM_SIZE,
> +				 buf, sizeof(buf));
> +	snprintf(valid_socket_mem, sizeof(valid_socket_mem),
> +		"%s%s", buf, DEFAULT_MEM_SIZE);
> +
>  	/* valid --socket-mem flag */
>  	const char *argv10[] = {prgname,
>  			"--file-prefix=" memtest, valid_socket_mem};
> --
> 2.8.4

Re: [dpdk-dev] [PATCH v3 1/1] app/test: fix --socket-mem option in eal flag autotest

[David Marchand]

Hello,

On Fri, Jul 12, 2019 at 8:43 AM <vattunuru@marvell.com> wrote:
>
> From: Vamsi Attunuru <vattunuru@marvell.com>
>
> eal flag autotest fails when multiple mem size flags are passed
> to --socket-mem option irrespective of RTE_MAX_NUMA_NODES.
>
> Patch fixes --socket-mem option by setting enough numbers
> of numa node mem flags based on RTE_MAX_NUMA_NODES config.
>
> Fixes: 45f1b6e8680a ("app: add new tests on eal flags")
>
> Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> ---
> V3 changes:
> * Pass buffer size info to populate socket mem routine which
> strlcpy requires.
>
> V2 changes:
> * Add routine to populate --socket-mem option and use it to form
> valid and invalid test commands.
>
>  app/test/test_eal_flags.c | 121 +++++++++++++++++++++++++++-------------------
>  1 file changed, 71 insertions(+), 50 deletions(-)
>
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 672ca0a..053d694 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -1250,12 +1250,46 @@ test_file_prefix(void)
>         return 0;
>  }
>
> +static void
> +populate_socket_mem_param(int num_sockets, const char *mem,
> +                        char *buf, size_t buf_size)
> +{
> +       char tmp[SOCKET_MEM_STRLEN];

You don't need this temp buffer.

> +       int i;
> +
> +       snprintf(buf, SOCKET_MEM_STRLEN, "--socket-mem=");

buf is buf_size large, not SOCKET_MEM_STRLEN.


> +
> +       for (i = 0; i < num_sockets ; i++) {
> +               snprintf(tmp, sizeof(tmp), "%s%s", buf, mem);
> +               strlcpy(buf, tmp, buf_size);
> +
> +               if (num_sockets + 1 - i > 1) {
> +                       snprintf(tmp, sizeof(tmp), "%s,", buf);
> +                       strlcpy(buf, tmp, buf_size);
> +               }
> +       }
> +}
> +


How about the following:

/* This function writes in passed buf pointer a valid --socket-mem= option
 * for num_sockets then concatenates the provided suffix string.
 *
 * Example for num_sockets 4, mem "2", suffix "plop"
 * --socket-mem=2,2,2,2plop
 */
static void
populate_socket_mem_param(int num_sockets, const char *mem,
                const char *suffix, char *buf, size_t buf_size)
{
        unsigned int offset = 0;
        int written;
        int i;

        written = snprintf(&buf[offset], buf_size - offset, "--socket-mem=");
        if (written < 0 || written + offset >= buf_size)
                return;
        offset += written;

        for (i = 0; i < num_sockets - 1; i++) {
                written = snprintf(&buf[offset], buf_size - offset,
                        "%s,", mem);
                if (written < 0 || written + offset >= buf_size)
                        return;
                offset += written;
        }

        written = snprintf(&buf[offset], buf_size - offset, "%s%s", mem,
                suffix);
        if (written < 0 || written + offset >= buf_size)
                return;
        offset += written;
}

See below for usage.



>  /*
>   * Tests for correct handling of -m and --socket-mem flags
>   */
>  static int
>  test_memory_flags(void)
>  {
> +       char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto itself */

You don't need this temp buffer.


> +
> +#ifdef RTE_EXEC_ENV_FREEBSD
> +       int num_sockets = 1;
> +#else
> +       int num_sockets = RTE_MIN(get_number_of_sockets(),
> +                       RTE_MAX_NUMA_NODES);
> +#endif
> +
> +       if (num_sockets <= 0) {
> +               printf("Error - cannot get number of sockets!\n");
> +               return -1;
> +       }
> +
>  #ifdef RTE_EXEC_ENV_FREEBSD
>         /* BSD target doesn't support prefixes at this point */
>         const char * prefix = "";
> @@ -1277,85 +1311,72 @@ test_memory_flags(void)
>                         "--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE};
>
>         /* valid (zero) --socket-mem flag */
> +       char arg2_socket_mem[SOCKET_MEM_STRLEN];
> +       populate_socket_mem_param(num_sockets - 1, "0", buf, sizeof(buf));
> +       snprintf(arg2_socket_mem, sizeof(arg2_socket_mem), "%s%s", buf, "0");

With the proposed populate_socket_mem_param, it becomes:

        populate_socket_mem_param(num_sockets, "0", "",
                        arg2_socket_mem, sizeof(arg2_socket_mem));

>         const char *argv2[] = {prgname,
> -                       "--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
> +                       "--file-prefix=" memtest, arg2_socket_mem};
>
>         /* invalid (incomplete) --socket-mem flag */
> +       char arg3_socket_mem[SOCKET_MEM_STRLEN];
> +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));

What happens when num_socket == 1?

With the proposed populate_socket_mem_param, it would become:

        populate_socket_mem_param(num_sockets - 1, "2", ",",
                        arg3_socket_mem, sizeof(arg3_socket_mem));

But we need to ensure that we have at least two sockets before
starting the test on argv3.


> +       snprintf(arg3_socket_mem, sizeof(arg3_socket_mem), "%s%s", buf, "2,");
>         const char *argv3[] = {prgname,
> -                       "--file-prefix=" memtest, "--socket-mem=2,2,"};
> +                       "--file-prefix=" memtest, arg3_socket_mem};
>
>         /* invalid (mixed with invalid data) --socket-mem flag */
> +       char arg4_socket_mem[SOCKET_MEM_STRLEN];
> +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +       snprintf(arg4_socket_mem, sizeof(arg4_socket_mem), "%s%s", buf, "Fred");
>         const char *argv4[] = {prgname,
> -                       "--file-prefix=" memtest, "--socket-mem=2,2,Fred"};
> +                       "--file-prefix=" memtest, arg4_socket_mem};

Idem argv3.

>
>         /* invalid (with numeric value as last character) --socket-mem flag */
> +       char arg5_socket_mem[SOCKET_MEM_STRLEN];
> +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +       snprintf(arg5_socket_mem, sizeof(arg5_socket_mem),
> +               "%s%s", buf, "Fred0");
>         const char *argv5[] = {prgname,
> -                       "--file-prefix=" memtest, "--socket-mem=2,2,Fred0"};
> +                       "--file-prefix=" memtest, arg5_socket_mem};

Idem argv3.

>
>         /* invalid (with empty socket) --socket-mem flag */
> +       char arg6_socket_mem[SOCKET_MEM_STRLEN];
> +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +       snprintf(arg6_socket_mem, sizeof(arg6_socket_mem), "%s%s", buf, ",,");
>         const char *argv6[] = {prgname,
> -                       "--file-prefix=" memtest, "--socket-mem=2,,2"};
> +                       "--file-prefix=" memtest, arg6_socket_mem};

Here, this test requires at least 3 sockets so that we can test at minimum 2,,2.


>
>         /* invalid (null) --socket-mem flag */
>         const char *argv7[] = {prgname,
>                         "--file-prefix=" memtest, "--socket-mem="};
>
>         /* valid --socket-mem specified together with -m flag */
> +       char arg8_socket_mem[SOCKET_MEM_STRLEN];
> +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> +       snprintf(arg8_socket_mem, sizeof(arg8_socket_mem), "%s%s", buf, "2");
>         const char *argv8[] = {prgname,
> -                       "--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "--socket-mem=2,2"};
> +                       "--file-prefix=" memtest, "-m",
> +                       DEFAULT_MEM_SIZE, arg8_socket_mem};
>
>         /* construct an invalid socket mask with 2 megs on each socket plus
>          * extra 2 megs on socket that doesn't exist on current system */
>         char invalid_socket_mem[SOCKET_MEM_STRLEN];
> -       char buf[SOCKET_MEM_STRLEN];    /* to avoid copying string onto itself */
> -
> -#ifdef RTE_EXEC_ENV_FREEBSD
> -       int i, num_sockets = 1;
> -#else
> -       int i, num_sockets = RTE_MIN(get_number_of_sockets(),
> -                       RTE_MAX_NUMA_NODES);
> -#endif
> -
> -       if (num_sockets <= 0) {
> -               printf("Error - cannot get number of sockets!\n");
> -               return -1;
> -       }
> -
> -       snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-mem=");
> -
> -       /* add one extra socket */
> -       for (i = 0; i < num_sockets + 1; i++) {
> -               snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
> -               strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
> -
> -               if (num_sockets + 1 - i > 1) {
> -                       snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
> -                       strlcpy(invalid_socket_mem, buf,
> -                               sizeof(invalid_socket_mem));
> -               }
> -       }
> -
> -       /* construct a valid socket mask with 2 megs on each existing socket */
> -       char valid_socket_mem[SOCKET_MEM_STRLEN];
> -
> -       snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-mem=");
> -
> -       /* add one extra socket */
> -       for (i = 0; i < num_sockets; i++) {
> -               snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
> -               strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
> -
> -               if (num_sockets - i > 1) {
> -                       snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
> -                       strlcpy(valid_socket_mem, buf,
> -                               sizeof(valid_socket_mem));
> -               }
> -       }
> +       populate_socket_mem_param(num_sockets, DEFAULT_MEM_SIZE,
> +                                buf, sizeof(buf));
> +       snprintf(invalid_socket_mem, sizeof(invalid_socket_mem),
> +               "%s%s", buf, DEFAULT_MEM_SIZE);

The comment says you want to generate a string with 2M per socket, not
DEFAULT_MEM_SIZE.

>
>         /* invalid --socket-mem flag (with extra socket) */
>         const char *argv9[] = {prgname,
>                         "--file-prefix=" memtest, invalid_socket_mem};
>
> +       /* construct a valid socket mask with 2 megs on each existing socket */
> +       char valid_socket_mem[SOCKET_MEM_STRLEN];
> +       populate_socket_mem_param(num_sockets - 1, DEFAULT_MEM_SIZE,
> +                                buf, sizeof(buf));
> +       snprintf(valid_socket_mem, sizeof(valid_socket_mem),
> +               "%s%s", buf, DEFAULT_MEM_SIZE);
> +

Idem 2M, not DEFAULT_MEM_SIZE.


>         /* valid --socket-mem flag */
>         const char *argv10[] = {prgname,
>                         "--file-prefix=" memtest, valid_socket_mem};
> --
> 2.8.4
>

I prepared a patch with my comments addressed, can you have a look ?
https://github.com/david-marchand/dpdk/commit/a78b7572a402579d775f1d0622d1454eb36e0ad4

Thanks.


--
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v3 1/1] app/test: fix --socket-mem option in eal flag autotest

[Vamsi Krishna Attunuru]

Hi,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, July 26, 2019 7:10 PM
> To: Vamsi Krishna Attunuru <vattunuru@marvell.com>
> Cc: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Jerin
> Jacob Kollanukkaran <jerinj@marvell.com>; Aaron Conole
> <aconole@redhat.com>
> Subject: [EXT] Re: [PATCH v3 1/1] app/test: fix --socket-mem option in eal flag
> autotest
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hello,
> 
> On Fri, Jul 12, 2019 at 8:43 AM <vattunuru@marvell.com> wrote:
> >
> > From: Vamsi Attunuru <vattunuru@marvell.com>
> >
> > eal flag autotest fails when multiple mem size flags are passed to
> > --socket-mem option irrespective of RTE_MAX_NUMA_NODES.
> >
> > Patch fixes --socket-mem option by setting enough numbers of numa node
> > mem flags based on RTE_MAX_NUMA_NODES config.
> >
> > Fixes: 45f1b6e8680a ("app: add new tests on eal flags")
> >
> > Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> > ---
> > V3 changes:
> > * Pass buffer size info to populate socket mem routine which strlcpy
> > requires.
> >
> > V2 changes:
> > * Add routine to populate --socket-mem option and use it to form valid
> > and invalid test commands.
> >
> >  app/test/test_eal_flags.c | 121
> > +++++++++++++++++++++++++++-------------------
> >  1 file changed, 71 insertions(+), 50 deletions(-)
> >
> > diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> > index 672ca0a..053d694 100644
> > --- a/app/test/test_eal_flags.c
> > +++ b/app/test/test_eal_flags.c
> > @@ -1250,12 +1250,46 @@ test_file_prefix(void)
> >         return 0;
> >  }
> >
> > +static void
> > +populate_socket_mem_param(int num_sockets, const char *mem,
> > +                        char *buf, size_t buf_size) {
> > +       char tmp[SOCKET_MEM_STRLEN];
> 
> You don't need this temp buffer.
> 
> > +       int i;
> > +
> > +       snprintf(buf, SOCKET_MEM_STRLEN, "--socket-mem=");
> 
> buf is buf_size large, not SOCKET_MEM_STRLEN.
> 
> 
> > +
> > +       for (i = 0; i < num_sockets ; i++) {
> > +               snprintf(tmp, sizeof(tmp), "%s%s", buf, mem);
> > +               strlcpy(buf, tmp, buf_size);
> > +
> > +               if (num_sockets + 1 - i > 1) {
> > +                       snprintf(tmp, sizeof(tmp), "%s,", buf);
> > +                       strlcpy(buf, tmp, buf_size);
> > +               }
> > +       }
> > +}
> > +
> 
> 
> How about the following:
> 
> /* This function writes in passed buf pointer a valid --socket-mem= option
>  * for num_sockets then concatenates the provided suffix string.
>  *
>  * Example for num_sockets 4, mem "2", suffix "plop"
>  * --socket-mem=2,2,2,2plop
>  */
> static void
> populate_socket_mem_param(int num_sockets, const char *mem,
>                 const char *suffix, char *buf, size_t buf_size) {
>         unsigned int offset = 0;
>         int written;
>         int i;
> 
>         written = snprintf(&buf[offset], buf_size - offset, "--socket-mem=");
>         if (written < 0 || written + offset >= buf_size)
>                 return;
>         offset += written;
> 
>         for (i = 0; i < num_sockets - 1; i++) {
>                 written = snprintf(&buf[offset], buf_size - offset,
>                         "%s,", mem);
>                 if (written < 0 || written + offset >= buf_size)
>                         return;
>                 offset += written;
>         }
> 
>         written = snprintf(&buf[offset], buf_size - offset, "%s%s", mem,
>                 suffix);
>         if (written < 0 || written + offset >= buf_size)
>                 return;
>         offset += written;
> }
> 
> See below for usage.
> 
> 
> 
> >  /*
> >   * Tests for correct handling of -m and --socket-mem flags
> >   */
> >  static int
> >  test_memory_flags(void)
> >  {
> > +       char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto
> > + itself */
> 
> You don't need this temp buffer.
> 
> 
> > +
> > +#ifdef RTE_EXEC_ENV_FREEBSD
> > +       int num_sockets = 1;
> > +#else
> > +       int num_sockets = RTE_MIN(get_number_of_sockets(),
> > +                       RTE_MAX_NUMA_NODES); #endif
> > +
> > +       if (num_sockets <= 0) {
> > +               printf("Error - cannot get number of sockets!\n");
> > +               return -1;
> > +       }
> > +
> >  #ifdef RTE_EXEC_ENV_FREEBSD
> >         /* BSD target doesn't support prefixes at this point */
> >         const char * prefix = "";
> > @@ -1277,85 +1311,72 @@ test_memory_flags(void)
> >                         "--file-prefix=" memtest, "-m",
> > DEFAULT_MEM_SIZE};
> >
> >         /* valid (zero) --socket-mem flag */
> > +       char arg2_socket_mem[SOCKET_MEM_STRLEN];
> > +       populate_socket_mem_param(num_sockets - 1, "0", buf, sizeof(buf));
> > +       snprintf(arg2_socket_mem, sizeof(arg2_socket_mem), "%s%s",
> > + buf, "0");
> 
> With the proposed populate_socket_mem_param, it becomes:
> 
>         populate_socket_mem_param(num_sockets, "0", "",
>                         arg2_socket_mem, sizeof(arg2_socket_mem));
> 
> >         const char *argv2[] = {prgname,
> > -                       "--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
> > +                       "--file-prefix=" memtest, arg2_socket_mem};
> >
> >         /* invalid (incomplete) --socket-mem flag */
> > +       char arg3_socket_mem[SOCKET_MEM_STRLEN];
> > +       populate_socket_mem_param(num_sockets - 1, "2", buf,
> > + sizeof(buf));
> 
> What happens when num_socket == 1?
> 
> With the proposed populate_socket_mem_param, it would become:
> 
>         populate_socket_mem_param(num_sockets - 1, "2", ",",
>                         arg3_socket_mem, sizeof(arg3_socket_mem));
> 
> But we need to ensure that we have at least two sockets before starting the
> test on argv3.
> 
> 
> > +       snprintf(arg3_socket_mem, sizeof(arg3_socket_mem), "%s%s",
> > + buf, "2,");
> >         const char *argv3[] = {prgname,
> > -                       "--file-prefix=" memtest, "--socket-mem=2,2,"};
> > +                       "--file-prefix=" memtest, arg3_socket_mem};
> >
> >         /* invalid (mixed with invalid data) --socket-mem flag */
> > +       char arg4_socket_mem[SOCKET_MEM_STRLEN];
> > +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> > +       snprintf(arg4_socket_mem, sizeof(arg4_socket_mem), "%s%s",
> > + buf, "Fred");
> >         const char *argv4[] = {prgname,
> > -                       "--file-prefix=" memtest, "--socket-mem=2,2,Fred"};
> > +                       "--file-prefix=" memtest, arg4_socket_mem};
> 
> Idem argv3.
> 
> >
> >         /* invalid (with numeric value as last character) --socket-mem
> > flag */
> > +       char arg5_socket_mem[SOCKET_MEM_STRLEN];
> > +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> > +       snprintf(arg5_socket_mem, sizeof(arg5_socket_mem),
> > +               "%s%s", buf, "Fred0");
> >         const char *argv5[] = {prgname,
> > -                       "--file-prefix=" memtest, "--socket-mem=2,2,Fred0"};
> > +                       "--file-prefix=" memtest, arg5_socket_mem};
> 
> Idem argv3.
> 
> >
> >         /* invalid (with empty socket) --socket-mem flag */
> > +       char arg6_socket_mem[SOCKET_MEM_STRLEN];
> > +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> > +       snprintf(arg6_socket_mem, sizeof(arg6_socket_mem), "%s%s",
> > + buf, ",,");
> >         const char *argv6[] = {prgname,
> > -                       "--file-prefix=" memtest, "--socket-mem=2,,2"};
> > +                       "--file-prefix=" memtest, arg6_socket_mem};
> 
> Here, this test requires at least 3 sockets so that we can test at minimum 2,,2.
> 
> 
> >
> >         /* invalid (null) --socket-mem flag */
> >         const char *argv7[] = {prgname,
> >                         "--file-prefix=" memtest, "--socket-mem="};
> >
> >         /* valid --socket-mem specified together with -m flag */
> > +       char arg8_socket_mem[SOCKET_MEM_STRLEN];
> > +       populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf));
> > +       snprintf(arg8_socket_mem, sizeof(arg8_socket_mem), "%s%s",
> > + buf, "2");
> >         const char *argv8[] = {prgname,
> > -                       "--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "--
> socket-mem=2,2"};
> > +                       "--file-prefix=" memtest, "-m",
> > +                       DEFAULT_MEM_SIZE, arg8_socket_mem};
> >
> >         /* construct an invalid socket mask with 2 megs on each socket plus
> >          * extra 2 megs on socket that doesn't exist on current system */
> >         char invalid_socket_mem[SOCKET_MEM_STRLEN];
> > -       char buf[SOCKET_MEM_STRLEN];    /* to avoid copying string onto
> itself */
> > -
> > -#ifdef RTE_EXEC_ENV_FREEBSD
> > -       int i, num_sockets = 1;
> > -#else
> > -       int i, num_sockets = RTE_MIN(get_number_of_sockets(),
> > -                       RTE_MAX_NUMA_NODES);
> > -#endif
> > -
> > -       if (num_sockets <= 0) {
> > -               printf("Error - cannot get number of sockets!\n");
> > -               return -1;
> > -       }
> > -
> > -       snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-
> mem=");
> > -
> > -       /* add one extra socket */
> > -       for (i = 0; i < num_sockets + 1; i++) {
> > -               snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem,
> DEFAULT_MEM_SIZE);
> > -               strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
> > -
> > -               if (num_sockets + 1 - i > 1) {
> > -                       snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
> > -                       strlcpy(invalid_socket_mem, buf,
> > -                               sizeof(invalid_socket_mem));
> > -               }
> > -       }
> > -
> > -       /* construct a valid socket mask with 2 megs on each existing socket */
> > -       char valid_socket_mem[SOCKET_MEM_STRLEN];
> > -
> > -       snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-
> mem=");
> > -
> > -       /* add one extra socket */
> > -       for (i = 0; i < num_sockets; i++) {
> > -               snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem,
> DEFAULT_MEM_SIZE);
> > -               strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
> > -
> > -               if (num_sockets - i > 1) {
> > -                       snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
> > -                       strlcpy(valid_socket_mem, buf,
> > -                               sizeof(valid_socket_mem));
> > -               }
> > -       }
> > +       populate_socket_mem_param(num_sockets, DEFAULT_MEM_SIZE,
> > +                                buf, sizeof(buf));
> > +       snprintf(invalid_socket_mem, sizeof(invalid_socket_mem),
> > +               "%s%s", buf, DEFAULT_MEM_SIZE);
> 
> The comment says you want to generate a string with 2M per socket, not
> DEFAULT_MEM_SIZE.
> 
> >
> >         /* invalid --socket-mem flag (with extra socket) */
> >         const char *argv9[] = {prgname,
> >                         "--file-prefix=" memtest, invalid_socket_mem};
> >
> > +       /* construct a valid socket mask with 2 megs on each existing socket */
> > +       char valid_socket_mem[SOCKET_MEM_STRLEN];
> > +       populate_socket_mem_param(num_sockets - 1, DEFAULT_MEM_SIZE,
> > +                                buf, sizeof(buf));
> > +       snprintf(valid_socket_mem, sizeof(valid_socket_mem),
> > +               "%s%s", buf, DEFAULT_MEM_SIZE);
> > +
> 
> Idem 2M, not DEFAULT_MEM_SIZE.
> 
> 
> >         /* valid --socket-mem flag */
> >         const char *argv10[] = {prgname,
> >                         "--file-prefix=" memtest, valid_socket_mem};
> > --
> > 2.8.4
> >
> 
> I prepared a patch with my comments addressed, can you have a look ?
> https://github.com/david-
> marchand/dpdk/commit/a78b7572a402579d775f1d0622d1454eb36e0ad4

Thanks for the comments and new patch, reviewed and verified it.
Patch looks good to me.

Reviewed-by: Vamsi Attunuru <vattunuru@marvell.com>
Tested-by: Vamsi Attunuru <vattunuru@marvell.com>

> 
> Thanks.
> 
> 
> --
> David Marchand

[dpdk-dev] [PATCH v4] test: fix --socket-mem option in eal flag autotest

[David Marchand]

From: Vamsi Attunuru <vattunuru@marvell.com>

eal flag autotest fails when multiple mem size flags are passed to
--socket-mem option irrespective of RTE_MAX_NUMA_NODES and the number of
available sockets on the test system.

Fixes: 45f1b6e8680a ("app: add new tests on eal flags")
Cc: stable@dpdk.org

Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Reviewed-by: Vamsi Attunuru <vattunuru@marvell.com>
Tested-by: Vamsi Attunuru <vattunuru@marvell.com>
---
 app/test/test_eal_flags.c | 144 +++++++++++++++++++++++++++-------------------
 1 file changed, 86 insertions(+), 58 deletions(-)

diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index 672ca0a..827ea88 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -1250,6 +1250,40 @@ test_file_prefix(void)
 	return 0;
 }
 
+/* This function writes in passed buf pointer a valid --socket-mem= option
+ * for num_sockets then concatenates the provided suffix string.
+ *
+ * Example for num_sockets 4, mem "2", suffix "plop"
+ * --socket-mem=2,2,2,2plop
+ */
+static void
+populate_socket_mem_param(int num_sockets, const char *mem,
+		const char *suffix, char *buf, size_t buf_size)
+{
+	unsigned int offset = 0;
+	int written;
+	int i;
+
+	written = snprintf(&buf[offset], buf_size - offset, "--socket-mem=");
+	if (written < 0 || written + offset >= buf_size)
+		return;
+	offset += written;
+
+	for (i = 0; i < num_sockets - 1; i++) {
+		written = snprintf(&buf[offset], buf_size - offset,
+			"%s,", mem);
+		if (written < 0 || written + offset >= buf_size)
+			return;
+		offset += written;
+	}
+
+	written = snprintf(&buf[offset], buf_size - offset, "%s%s", mem,
+		suffix);
+	if (written < 0 || written + offset >= buf_size)
+		return;
+	offset += written;
+}
+
 /*
  * Tests for correct handling of -m and --socket-mem flags
  */
@@ -1277,42 +1311,44 @@ test_memory_flags(void)
 			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE};
 
 	/* valid (zero) --socket-mem flag */
+	char arg2_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv2[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=0,0,0,0"};
+			"--file-prefix=" memtest, arg2_socket_mem};
 
 	/* invalid (incomplete) --socket-mem flag */
+	char arg3_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv3[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,"};
+			"--file-prefix=" memtest, arg3_socket_mem};
 
 	/* invalid (mixed with invalid data) --socket-mem flag */
+	char arg4_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv4[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,Fred"};
+			"--file-prefix=" memtest, arg4_socket_mem};
 
 	/* invalid (with numeric value as last character) --socket-mem flag */
+	char arg5_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv5[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,2,Fred0"};
+			"--file-prefix=" memtest, arg5_socket_mem};
 
 	/* invalid (with empty socket) --socket-mem flag */
+	char arg6_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv6[] = {prgname,
-			"--file-prefix=" memtest, "--socket-mem=2,,2"};
+			"--file-prefix=" memtest, arg6_socket_mem};
 
 	/* invalid (null) --socket-mem flag */
 	const char *argv7[] = {prgname,
 			"--file-prefix=" memtest, "--socket-mem="};
 
 	/* valid --socket-mem specified together with -m flag */
+	char arg8_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv8[] = {prgname,
-			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "--socket-mem=2,2"};
-
-	/* construct an invalid socket mask with 2 megs on each socket plus
-	 * extra 2 megs on socket that doesn't exist on current system */
-	char invalid_socket_mem[SOCKET_MEM_STRLEN];
-	char buf[SOCKET_MEM_STRLEN];	/* to avoid copying string onto itself */
+			"--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE,
+			arg8_socket_mem};
 
 #ifdef RTE_EXEC_ENV_FREEBSD
-	int i, num_sockets = 1;
+	int num_sockets = 1;
 #else
-	int i, num_sockets = RTE_MIN(get_number_of_sockets(),
+	int num_sockets = RTE_MIN(get_number_of_sockets(),
 			RTE_MAX_NUMA_NODES);
 #endif
 
@@ -1321,42 +1357,13 @@ test_memory_flags(void)
 		return -1;
 	}
 
-	snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-mem=");
-
-	/* add one extra socket */
-	for (i = 0; i < num_sockets + 1; i++) {
-		snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE);
-		strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem));
-
-		if (num_sockets + 1 - i > 1) {
-			snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem);
-			strlcpy(invalid_socket_mem, buf,
-				sizeof(invalid_socket_mem));
-		}
-	}
-
-	/* construct a valid socket mask with 2 megs on each existing socket */
-	char valid_socket_mem[SOCKET_MEM_STRLEN];
-
-	snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-mem=");
-
-	/* add one extra socket */
-	for (i = 0; i < num_sockets; i++) {
-		snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE);
-		strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem));
-
-		if (num_sockets - i > 1) {
-			snprintf(buf, sizeof(buf), "%s,", valid_socket_mem);
-			strlcpy(valid_socket_mem, buf,
-				sizeof(valid_socket_mem));
-		}
-	}
-
 	/* invalid --socket-mem flag (with extra socket) */
+	char invalid_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv9[] = {prgname,
 			"--file-prefix=" memtest, invalid_socket_mem};
 
 	/* valid --socket-mem flag */
+	char valid_socket_mem[SOCKET_MEM_STRLEN];
 	const char *argv10[] = {prgname,
 			"--file-prefix=" memtest, valid_socket_mem};
 
@@ -1374,34 +1381,49 @@ test_memory_flags(void)
 		printf("Error - process failed with valid -m flag!\n");
 		return -1;
 	}
+
+	populate_socket_mem_param(num_sockets, "0", "",
+		arg2_socket_mem, sizeof(arg2_socket_mem));
 	if (launch_proc(argv2) != 0) {
 		printf("Error - process failed with valid (zero) --socket-mem!\n");
 		return -1;
 	}
 
-	if (launch_proc(argv3) == 0) {
-		printf("Error - process run ok with invalid "
+	if (num_sockets > 1) {
+		populate_socket_mem_param(num_sockets - 1, "2", ",",
+			arg3_socket_mem, sizeof(arg3_socket_mem));
+		if (launch_proc(argv3) == 0) {
+			printf("Error - process run ok with invalid "
 				"(incomplete) --socket-mem!\n");
-		return -1;
-	}
+			return -1;
+		}
 
-	if (launch_proc(argv4) == 0) {
-		printf("Error - process run ok with invalid "
+		populate_socket_mem_param(num_sockets - 1, "2", ",Fred",
+			arg4_socket_mem, sizeof(arg4_socket_mem));
+		if (launch_proc(argv4) == 0) {
+			printf("Error - process run ok with invalid "
 				"(mixed with invalid input) --socket-mem!\n");
-		return -1;
-	}
+			return -1;
+		}
 
-	if (launch_proc(argv5) == 0) {
-		printf("Error - process run ok with invalid "
+		populate_socket_mem_param(num_sockets - 1, "2", ",Fred0",
+			arg5_socket_mem, sizeof(arg5_socket_mem));
+		if (launch_proc(argv5) == 0) {
+			printf("Error - process run ok with invalid "
 				"(mixed with invalid input with a numeric value as "
 				"last character) --socket-mem!\n");
-		return -1;
+			return -1;
+		}
 	}
 
-	if (launch_proc(argv6) == 0) {
-		printf("Error - process run ok with invalid "
+	if (num_sockets > 2) {
+		populate_socket_mem_param(num_sockets - 2, "2", ",,2",
+			arg6_socket_mem, sizeof(arg6_socket_mem));
+		if (launch_proc(argv6) == 0) {
+			printf("Error - process run ok with invalid "
 				"(with empty socket) --socket-mem!\n");
-		return -1;
+			return -1;
+		}
 	}
 
 	if (launch_proc(argv7) == 0) {
@@ -1409,16 +1431,22 @@ test_memory_flags(void)
 		return -1;
 	}
 
+	populate_socket_mem_param(num_sockets, "2", "",
+		arg8_socket_mem, sizeof(arg8_socket_mem));
 	if (launch_proc(argv8) == 0) {
 		printf("Error - process run ok with --socket-mem and -m specified!\n");
 		return -1;
 	}
 
+	populate_socket_mem_param(num_sockets + 1, "2", "",
+		invalid_socket_mem, sizeof(invalid_socket_mem));
 	if (launch_proc(argv9) == 0) {
 		printf("Error - process run ok with extra socket in --socket-mem!\n");
 		return -1;
 	}
 
+	populate_socket_mem_param(num_sockets, "2", "",
+		valid_socket_mem, sizeof(valid_socket_mem));
 	if (launch_proc(argv10) != 0) {
 		printf("Error - process failed with valid --socket-mem!\n");
 		return -1;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v4] test: fix --socket-mem option in eal flag autotest

[Thomas Monjalon]

29/07/2019 10:08, David Marchand:
> From: Vamsi Attunuru <vattunuru@marvell.com>
> 
> eal flag autotest fails when multiple mem size flags are passed to
> --socket-mem option irrespective of RTE_MAX_NUMA_NODES and the number of
> available sockets on the test system.
> 
> Fixes: 45f1b6e8680a ("app: add new tests on eal flags")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Vamsi Attunuru <vattunuru@marvell.com>
> Tested-by: Vamsi Attunuru <vattunuru@marvell.com>

Applied, thanks