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

Hi,

-----Original Message-----
From: David Marchand <redacted>
Sent: Friday, July 26, 2019 7:10 PM
To: Vamsi Krishna Attunuru <vattunuru@marvell.com>
Cc: dev <redacted>; Thomas Monjalon <redacted>; Jerin
Jacob Kollanukkaran [off-list ref]; Aaron Conole
[off-list ref]
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 [off-list ref] wrote:

quoted

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.

quoted

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

buf is buf_size large, not SOCKET_MEM_STRLEN.

quoted

+
+       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.

quoted

 /*
  * 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.

quoted

+
+#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));

quoted

        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.

quoted

+       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.

quoted

        /* 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.

quoted

        /* 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.

quoted

        /* 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"};

quoted

+                       "--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 */

quoted

-
-#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=");

quoted

-
-       /* add one extra socket */
-       for (i = 0; i < num_sockets + 1; i++) {
-               snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem,

DEFAULT_MEM_SIZE);

quoted

-               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=");

quoted

-
-       /* add one extra socket */
-       for (i = 0; i < num_sockets; i++) {
-               snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem,

DEFAULT_MEM_SIZE);

quoted

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

quoted

        /* 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.

quoted

        /* 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