[v3,1/1] app/test: fix --socket-mem option in eal flag autotest
Checks
Commit Message
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(-)
Comments
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
@@ -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};