diff mbox series

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

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

Checks

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

Commit Message

Vamsi Krishna Attunuru July 12, 2019, 6:43 a.m. UTC 
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

Vamsi Krishna Attunuru July 23, 2019, 6:13 a.m. UTC | #1 
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
David Marchand July 26, 2019, 1:39 p.m. UTC | #2 
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
Vamsi Krishna Attunuru July 28, 2019, 4:19 a.m. UTC | #3 
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 mbox series

Patch

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};