Message ID | 20190712064320.8106-1-vattunuru@marvell.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Thomas Monjalon |
Headers | show |
Series | [v3,1/1] app/test: fix --socket-mem option in eal flag autotest | expand |
Context | Check | Description |
---|---|---|
ci/checkpatch | success | coding style OK |
ci/mellanox-Performance-Testing | success | Performance Testing PASS |
ci/intel-Performance-Testing | success | Performance Testing PASS |
ci/Intel-compilation | success | Compilation OK |
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
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
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
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};