diff mbox

[dpdk-dev,07/10] eal: add core list input format

Message ID 1416692622-28886-8-git-send-email-thomas.monjalon@6wind.com (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Thomas Monjalon Nov. 22, 2014, 9:43 p.m. UTC
From: Didier Pallard <didier.pallard@6wind.com>

In current version, used cores can only be specified using a bitmask.
It will now be possible to specify cores in 2 different ways:
- Using a bitmask (-c [0x]nnn): bitmask must be in hex format
- Using a list in following format: -l <c1>[-c2][,c3[-c4],...]

The letter -l can stand for lcore or list.

-l 0-7,16-23,31 being equivalent to -c 0x80FF00FF

Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 app/test/test_eal_flags.c                  | 28 ++++++++++-
 lib/librte_eal/common/eal_common_options.c | 81 ++++++++++++++++++++++++++++--
 2 files changed, 103 insertions(+), 6 deletions(-)

Comments

Neil Horman Nov. 23, 2014, 1:35 a.m. UTC | #1
On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
> From: Didier Pallard <didier.pallard@6wind.com>
> 
> In current version, used cores can only be specified using a bitmask.
> It will now be possible to specify cores in 2 different ways:
> - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
> - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
> 
> The letter -l can stand for lcore or list.
> 
> -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
> 
Do you want to burn an option letter on that?  It seems like it might be better
to search the string for 0x and base the selection of bitmap of list parsing
based on its presence or absence.


> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
>  app/test/test_eal_flags.c                  | 28 ++++++++++-
>  lib/librte_eal/common/eal_common_options.c | 81 ++++++++++++++++++++++++++++--
>  2 files changed, 103 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 1f95d7f..5ad89c5 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -484,7 +484,7 @@ test_invalid_r_flag(void)
>  }
>  
>  /*
> - * Test that the app doesn't run without the coremask flag. In all cases
> + * Test that the app doesn't run without the coremask/corelist flags. In all cases
>   * should give an error and fail to run
>   */
>  static int
> @@ -504,12 +504,22 @@ test_missing_c_flag(void)
>  
>  	/* -c flag but no coremask value */
>  	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "3", "-c"};
> -	/* No -c flag at all */
> +	/* No -c or -l flag at all */
>  	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "3"};
>  	/* bad coremask value */
>  	const char *argv3[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "error" };
>  	/* sanity check of tests - valid coremask value */
>  	const char *argv4[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "1" };
> +	/* -l flag but no corelist value */
> +	const char *argv5[] = { prgname, prefix, mp_flag, "-n", "3", "-l"};
> +	const char *argv6[] = { prgname, prefix, mp_flag, "-n", "3", "-l", " " };
> +	/* bad corelist values */
> +	const char *argv7[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "error" };
> +	const char *argv8[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-" };
> +	const char *argv9[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1," };
> +	const char *argv10[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1#2" };
> +	/* sanity check test - valid corelist value */
> +	const char *argv11[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-2,3" };
>  
>  	if (launch_proc(argv1) == 0
>  			|| launch_proc(argv2) == 0
> @@ -521,6 +531,20 @@ test_missing_c_flag(void)
>  		printf("Error - process did not run ok with valid coremask value\n");
>  		return -1;
>  	}
> +
> +	if (launch_proc(argv5) == 0
> +			|| launch_proc(argv6) == 0
> +			|| launch_proc(argv7) == 0
> +			|| launch_proc(argv8) == 0
> +			|| launch_proc(argv9) == 0
> +			|| launch_proc(argv10) == 0) {
> +		printf("Error - process ran without error with invalid -l flag\n");
> +		return -1;
> +	}
> +	if (launch_proc(argv11) != 0) {
> +		printf("Error - process did not run ok with valid corelist value\n");
> +		return -1;
> +	}
>  	return 0;
>  }
>  
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 63710b0..18d03e3 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -32,6 +32,7 @@
>   */
>  
>  #include <stdlib.h>
> +#include <unistd.h>
>  #include <string.h>
>  #include <syslog.h>
>  #include <ctype.h>
> @@ -55,8 +56,9 @@ const char
>  eal_short_options[] =
>  	"b:" /* pci-blacklist */
>  	"w:" /* pci-whitelist */
> -	"c:"
> +	"c:" /* coremask */
>  	"d:"
> +	"l:" /* corelist */
>  	"m:"
>  	"n:"
>  	"r:"
> @@ -205,6 +207,67 @@ eal_parse_coremask(const char *coremask)
>  }
>  
>  static int
> +eal_parse_corelist(const char *corelist)
> +{
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +	int i, idx = 0;
> +	unsigned count = 0;
> +	char *end = NULL;
> +	int min, max;
> +
> +	if (corelist == NULL)
> +		return -1;
> +
> +	/* Remove all blank characters ahead and after */
> +	while (isblank(*corelist))
> +		corelist++;
> +	i = strnlen(corelist, sysconf(_SC_ARG_MAX));
> +	while ((i > 0) && isblank(corelist[i - 1]))
> +		i--;
> +
> +	/* Reset core roles */
> +	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
> +		cfg->lcore_role[idx] = ROLE_OFF;
> +
> +	/* Get list of cores */
> +	min = RTE_MAX_LCORE;
> +	do {
> +		while (isblank(*corelist))
> +			corelist++;
> +		if (*corelist == '\0')
> +			return -1;
> +		errno = 0;
> +		idx = strtoul(corelist, &end, 10);
> +		if (errno || end == NULL)
> +			return -1;
> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-') {
> +			min = idx;
> +		} else if ((*end == ',') || (*end == '\0')) {
> +			max = idx;
> +			if (min == RTE_MAX_LCORE)
> +				min = idx;
> +			for (idx = min; idx <= max; idx++) {
> +				cfg->lcore_role[idx] = ROLE_RTE;
> +				if (count == 0)
> +					cfg->master_lcore = idx;
> +				count++;
> +			}
> +			min = RTE_MAX_LCORE;
> +		} else
> +			return -1;
> +		corelist = end + 1;
> +	} while (*end != '\0');
> +
> +	if (count == 0)
> +		return -1;
> +
> +	lcores_parsed = 1;
> +	return 0;
> +}
> +
> +static int
>  eal_parse_syslog(const char *facility, struct internal_config *conf)
>  {
>  	int i;
> @@ -304,6 +367,13 @@ eal_parse_common_option(int opt, const char *optarg,
>  			return -1;
>  		}
>  		break;
> +	/* corelist */
> +	case 'l':
> +		if (eal_parse_corelist(optarg) < 0) {
> +			RTE_LOG(ERR, EAL, "invalid core list\n");
> +			return -1;
> +		}
> +		break;
>  	/* size of memory */
>  	case 'm':
>  		conf->memory = atoi(optarg);
> @@ -421,8 +491,8 @@ eal_check_common_options(struct internal_config *internal_cfg)
>  	struct rte_config *cfg = rte_eal_get_configuration();
>  
>  	if (!lcores_parsed) {
> -		RTE_LOG(ERR, EAL, "CPU cores must be enabled with option "
> -			"-c\n");
> +		RTE_LOG(ERR, EAL, "CPU cores must be enabled with options "
> +			"-c or -l\n");
>  		return -1;
>  	}
>  
> @@ -470,6 +540,9 @@ eal_common_usage(void)
>  	       "[--proc-type primary|secondary|auto]\n\n"
>  	       "EAL common options:\n"
>  	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
> +	       "  -l CORELIST  : List of cores to run on\n"
> +	       "                 The argument format is <c1>[-c2][,c3[-c4],...]\n"
> +	       "                 where c1, c2, etc are core indexes between 0 and %d\n"
>  	       "  -n NUM       : Number of memory channels\n"
>  	       "  -v           : Display version information on startup\n"
>  	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
> @@ -494,5 +567,5 @@ eal_common_usage(void)
>  	       "  --"OPT_NO_PCI"   : disable pci\n"
>  	       "  --"OPT_NO_HPET"  : disable hpet\n"
>  	       "  --"OPT_NO_SHCONF": no shared config (mmap'd files)\n"
> -	       "\n");
> +	       "\n", RTE_MAX_LCORE);
>  }
> -- 
> 2.1.3
> 
>
Bruce Richardson Nov. 24, 2014, 11:28 a.m. UTC | #2
On Sat, Nov 22, 2014 at 08:35:17PM -0500, Neil Horman wrote:
> On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
> > From: Didier Pallard <didier.pallard@6wind.com>
> > 
> > In current version, used cores can only be specified using a bitmask.
> > It will now be possible to specify cores in 2 different ways:
> > - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
> > - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
> > 
> > The letter -l can stand for lcore or list.
> > 
> > -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
> > 
> Do you want to burn an option letter on that?  It seems like it might be better
> to search the string for 0x and base the selection of bitmap of list parsing
> based on its presence or absence.
> 
>

The existing coremask parsing always assumes a hex coremask, so just looking
for a 0x will not work. I prefer this scheme of using a new flag for this method
of specifying the cores to use. 

If you don't want to use up a single-letter option, two alternatives:
1) use a long option instead.
2) if the -c parameter includes a "-" or a ",", treat it as a new-style option,
otherwise treat as old. The only abiguity here would be for specifying a single
core value 1-9 e.g. is "-c 6" a mask with two bits, or a single-core to run on.
[0 is obviously a named core as it's an invalid mask, and A-F are obviously
masks.] If we did want this scheme, I would suggest that we allow trailing
commas in the list specifier, so we can force users to clear ambiguity by
either writing "0x6" or "6," i.e. disallow ambiguous values to avoid problems.
However, this is probably more work that it's worth to avoid using up a letter
option.

I'd prefer any of these options to breaking backward compatibility in this case.

/Bruce


> > Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > ---
> >  app/test/test_eal_flags.c                  | 28 ++++++++++-
> >  lib/librte_eal/common/eal_common_options.c | 81 ++++++++++++++++++++++++++++--
> >  2 files changed, 103 insertions(+), 6 deletions(-)
> > 
> > diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> > index 1f95d7f..5ad89c5 100644
> > --- a/app/test/test_eal_flags.c
> > +++ b/app/test/test_eal_flags.c
> > @@ -484,7 +484,7 @@ test_invalid_r_flag(void)
> >  }
> >  
> >  /*
> > - * Test that the app doesn't run without the coremask flag. In all cases
> > + * Test that the app doesn't run without the coremask/corelist flags. In all cases
> >   * should give an error and fail to run
> >   */
> >  static int
> > @@ -504,12 +504,22 @@ test_missing_c_flag(void)
> >  
> >  	/* -c flag but no coremask value */
> >  	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "3", "-c"};
> > -	/* No -c flag at all */
> > +	/* No -c or -l flag at all */
> >  	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "3"};
> >  	/* bad coremask value */
> >  	const char *argv3[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "error" };
> >  	/* sanity check of tests - valid coremask value */
> >  	const char *argv4[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "1" };
> > +	/* -l flag but no corelist value */
> > +	const char *argv5[] = { prgname, prefix, mp_flag, "-n", "3", "-l"};
> > +	const char *argv6[] = { prgname, prefix, mp_flag, "-n", "3", "-l", " " };
> > +	/* bad corelist values */
> > +	const char *argv7[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "error" };
> > +	const char *argv8[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-" };
> > +	const char *argv9[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1," };
> > +	const char *argv10[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1#2" };
> > +	/* sanity check test - valid corelist value */
> > +	const char *argv11[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-2,3" };
> >  
> >  	if (launch_proc(argv1) == 0
> >  			|| launch_proc(argv2) == 0
> > @@ -521,6 +531,20 @@ test_missing_c_flag(void)
> >  		printf("Error - process did not run ok with valid coremask value\n");
> >  		return -1;
> >  	}
> > +
> > +	if (launch_proc(argv5) == 0
> > +			|| launch_proc(argv6) == 0
> > +			|| launch_proc(argv7) == 0
> > +			|| launch_proc(argv8) == 0
> > +			|| launch_proc(argv9) == 0
> > +			|| launch_proc(argv10) == 0) {
> > +		printf("Error - process ran without error with invalid -l flag\n");
> > +		return -1;
> > +	}
> > +	if (launch_proc(argv11) != 0) {
> > +		printf("Error - process did not run ok with valid corelist value\n");
> > +		return -1;
> > +	}
> >  	return 0;
> >  }
> >  
> > diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> > index 63710b0..18d03e3 100644
> > --- a/lib/librte_eal/common/eal_common_options.c
> > +++ b/lib/librte_eal/common/eal_common_options.c
> > @@ -32,6 +32,7 @@
> >   */
> >  
> >  #include <stdlib.h>
> > +#include <unistd.h>
> >  #include <string.h>
> >  #include <syslog.h>
> >  #include <ctype.h>
> > @@ -55,8 +56,9 @@ const char
> >  eal_short_options[] =
> >  	"b:" /* pci-blacklist */
> >  	"w:" /* pci-whitelist */
> > -	"c:"
> > +	"c:" /* coremask */
> >  	"d:"
> > +	"l:" /* corelist */
> >  	"m:"
> >  	"n:"
> >  	"r:"
> > @@ -205,6 +207,67 @@ eal_parse_coremask(const char *coremask)
> >  }
> >  
> >  static int
> > +eal_parse_corelist(const char *corelist)
> > +{
> > +	struct rte_config *cfg = rte_eal_get_configuration();
> > +	int i, idx = 0;
> > +	unsigned count = 0;
> > +	char *end = NULL;
> > +	int min, max;
> > +
> > +	if (corelist == NULL)
> > +		return -1;
> > +
> > +	/* Remove all blank characters ahead and after */
> > +	while (isblank(*corelist))
> > +		corelist++;
> > +	i = strnlen(corelist, sysconf(_SC_ARG_MAX));
> > +	while ((i > 0) && isblank(corelist[i - 1]))
> > +		i--;
> > +
> > +	/* Reset core roles */
> > +	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
> > +		cfg->lcore_role[idx] = ROLE_OFF;
> > +
> > +	/* Get list of cores */
> > +	min = RTE_MAX_LCORE;
> > +	do {
> > +		while (isblank(*corelist))
> > +			corelist++;
> > +		if (*corelist == '\0')
> > +			return -1;
> > +		errno = 0;
> > +		idx = strtoul(corelist, &end, 10);
> > +		if (errno || end == NULL)
> > +			return -1;
> > +		while (isblank(*end))
> > +			end++;
> > +		if (*end == '-') {
> > +			min = idx;
> > +		} else if ((*end == ',') || (*end == '\0')) {
> > +			max = idx;
> > +			if (min == RTE_MAX_LCORE)
> > +				min = idx;
> > +			for (idx = min; idx <= max; idx++) {
> > +				cfg->lcore_role[idx] = ROLE_RTE;
> > +				if (count == 0)
> > +					cfg->master_lcore = idx;
> > +				count++;
> > +			}
> > +			min = RTE_MAX_LCORE;
> > +		} else
> > +			return -1;
> > +		corelist = end + 1;
> > +	} while (*end != '\0');
> > +
> > +	if (count == 0)
> > +		return -1;
> > +
> > +	lcores_parsed = 1;
> > +	return 0;
> > +}
> > +
> > +static int
> >  eal_parse_syslog(const char *facility, struct internal_config *conf)
> >  {
> >  	int i;
> > @@ -304,6 +367,13 @@ eal_parse_common_option(int opt, const char *optarg,
> >  			return -1;
> >  		}
> >  		break;
> > +	/* corelist */
> > +	case 'l':
> > +		if (eal_parse_corelist(optarg) < 0) {
> > +			RTE_LOG(ERR, EAL, "invalid core list\n");
> > +			return -1;
> > +		}
> > +		break;
> >  	/* size of memory */
> >  	case 'm':
> >  		conf->memory = atoi(optarg);
> > @@ -421,8 +491,8 @@ eal_check_common_options(struct internal_config *internal_cfg)
> >  	struct rte_config *cfg = rte_eal_get_configuration();
> >  
> >  	if (!lcores_parsed) {
> > -		RTE_LOG(ERR, EAL, "CPU cores must be enabled with option "
> > -			"-c\n");
> > +		RTE_LOG(ERR, EAL, "CPU cores must be enabled with options "
> > +			"-c or -l\n");
> >  		return -1;
> >  	}
> >  
> > @@ -470,6 +540,9 @@ eal_common_usage(void)
> >  	       "[--proc-type primary|secondary|auto]\n\n"
> >  	       "EAL common options:\n"
> >  	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
> > +	       "  -l CORELIST  : List of cores to run on\n"
> > +	       "                 The argument format is <c1>[-c2][,c3[-c4],...]\n"
> > +	       "                 where c1, c2, etc are core indexes between 0 and %d\n"
> >  	       "  -n NUM       : Number of memory channels\n"
> >  	       "  -v           : Display version information on startup\n"
> >  	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
> > @@ -494,5 +567,5 @@ eal_common_usage(void)
> >  	       "  --"OPT_NO_PCI"   : disable pci\n"
> >  	       "  --"OPT_NO_HPET"  : disable hpet\n"
> >  	       "  --"OPT_NO_SHCONF": no shared config (mmap'd files)\n"
> > -	       "\n");
> > +	       "\n", RTE_MAX_LCORE);
> >  }
> > -- 
> > 2.1.3
> > 
> >
Thomas Monjalon Nov. 24, 2014, 1:19 p.m. UTC | #3
Hi Bruce and Neil,

2014-11-24 11:28, Bruce Richardson:
> On Sat, Nov 22, 2014 at 08:35:17PM -0500, Neil Horman wrote:
> > On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
> > > From: Didier Pallard <didier.pallard@6wind.com>
> > > 
> > > In current version, used cores can only be specified using a bitmask.
> > > It will now be possible to specify cores in 2 different ways:
> > > - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
> > > - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
> > > 
> > > The letter -l can stand for lcore or list.
> > > 
> > > -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
> > 
> > Do you want to burn an option letter on that?  It seems like it might be better
> > to search the string for 0x and base the selection of bitmap of list parsing
> > based on its presence or absence.

It was the initial proposal (in April):
	http://dpdk.org/ml/archives/dev/2014-April/002173.html
And I liked keeping only 1 option;
	http://dpdk.org/ml/archives/dev/2014-May/002722.html
But Anatoly raised the compatibility problem:
	http://dpdk.org/ml/archives/dev/2014-May/002723.html
Then there was no other comment so Didier and I reworked a separate option. 

> The existing coremask parsing always assumes a hex coremask, so just looking
> for a 0x will not work. I prefer this scheme of using a new flag for this method
> of specifying the cores to use. 
> 
> If you don't want to use up a single-letter option, two alternatives:
> 1) use a long option instead.
> 2) if the -c parameter includes a "-" or a ",", treat it as a new-style option,
> otherwise treat as old. The only abiguity here would be for specifying a single
> core value 1-9 e.g. is "-c 6" a mask with two bits, or a single-core to run on.
> [0 is obviously a named core as it's an invalid mask, and A-F are obviously
> masks.] If we did want this scheme, I would suggest that we allow trailing
> commas in the list specifier, so we can force users to clear ambiguity by
> either writing "0x6" or "6," i.e. disallow ambiguous values to avoid problems.
> However, this is probably more work that it's worth to avoid using up a letter
> option.
> 
> I'd prefer any of these options to breaking backward compatibility in this case.

We need a consensus here.
Who is supporting a "burn" of an one-letter option with clear usage?
Who is supporting a "re-merge" of the 2 syntaxes with more complicated rules
(list syntax is triggered by presence of "-" or ",")? 

Please vote quickly.
Thanks
Bruce Richardson Nov. 24, 2014, 1:28 p.m. UTC | #4
On Mon, Nov 24, 2014 at 02:19:16PM +0100, Thomas Monjalon wrote:
> Hi Bruce and Neil,
> 
> 2014-11-24 11:28, Bruce Richardson:
> > On Sat, Nov 22, 2014 at 08:35:17PM -0500, Neil Horman wrote:
> > > On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
> > > > From: Didier Pallard <didier.pallard@6wind.com>
> > > > 
> > > > In current version, used cores can only be specified using a bitmask.
> > > > It will now be possible to specify cores in 2 different ways:
> > > > - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
> > > > - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
> > > > 
> > > > The letter -l can stand for lcore or list.
> > > > 
> > > > -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
> > > 
> > > Do you want to burn an option letter on that?  It seems like it might be better
> > > to search the string for 0x and base the selection of bitmap of list parsing
> > > based on its presence or absence.
> 
> It was the initial proposal (in April):
> 	http://dpdk.org/ml/archives/dev/2014-April/002173.html
> And I liked keeping only 1 option;
> 	http://dpdk.org/ml/archives/dev/2014-May/002722.html
> But Anatoly raised the compatibility problem:
> 	http://dpdk.org/ml/archives/dev/2014-May/002723.html
> Then there was no other comment so Didier and I reworked a separate option. 
> 
> > The existing coremask parsing always assumes a hex coremask, so just looking
> > for a 0x will not work. I prefer this scheme of using a new flag for this method
> > of specifying the cores to use. 
> > 
> > If you don't want to use up a single-letter option, two alternatives:
> > 1) use a long option instead.
> > 2) if the -c parameter includes a "-" or a ",", treat it as a new-style option,
> > otherwise treat as old. The only abiguity here would be for specifying a single
> > core value 1-9 e.g. is "-c 6" a mask with two bits, or a single-core to run on.
> > [0 is obviously a named core as it's an invalid mask, and A-F are obviously
> > masks.] If we did want this scheme, I would suggest that we allow trailing
> > commas in the list specifier, so we can force users to clear ambiguity by
> > either writing "0x6" or "6," i.e. disallow ambiguous values to avoid problems.
> > However, this is probably more work that it's worth to avoid using up a letter
> > option.
> > 
> > I'd prefer any of these options to breaking backward compatibility in this case.
> 
> We need a consensus here.
> Who is supporting a "burn" of an one-letter option with clear usage?
> Who is supporting a "re-merge" of the 2 syntaxes with more complicated rules
> (list syntax is triggered by presence of "-" or ",")? 
>

Burn!
Burakov, Anatoly Nov. 24, 2014, 1:37 p.m. UTC | #5
> > > > Do you want to burn an option letter on that?  It seems like it
> > > > might be better to search the string for 0x and base the selection
> > > > of bitmap of list parsing based on its presence or absence.
> >
> > It was the initial proposal (in April):
> > 	http://dpdk.org/ml/archives/dev/2014-April/002173.html
> > And I liked keeping only 1 option;
> > 	http://dpdk.org/ml/archives/dev/2014-May/002722.html
> > But Anatoly raised the compatibility problem:
> > 	http://dpdk.org/ml/archives/dev/2014-May/002723.html
> > Then there was no other comment so Didier and I reworked a separate
> option.
> >
> > > The existing coremask parsing always assumes a hex coremask, so just
> > > looking for a 0x will not work. I prefer this scheme of using a new
> > > flag for this method of specifying the cores to use.
> > >
> > > If you don't want to use up a single-letter option, two alternatives:
> > > 1) use a long option instead.
> > > 2) if the -c parameter includes a "-" or a ",", treat it as a
> > > new-style option, otherwise treat as old. The only abiguity here
> > > would be for specifying a single core value 1-9 e.g. is "-c 6" a mask with
> two bits, or a single-core to run on.
> > > [0 is obviously a named core as it's an invalid mask, and A-F are
> > > obviously masks.] If we did want this scheme, I would suggest that
> > > we allow trailing commas in the list specifier, so we can force
> > > users to clear ambiguity by either writing "0x6" or "6," i.e. disallow
> ambiguous values to avoid problems.
> > > However, this is probably more work that it's worth to avoid using
> > > up a letter option.
> > >
> > > I'd prefer any of these options to breaking backward compatibility in this
> case.
> >
> > We need a consensus here.
> > Who is supporting a "burn" of an one-letter option with clear usage?
> > Who is supporting a "re-merge" of the 2 syntaxes with more complicated
> > rules (list syntax is triggered by presence of "-" or ",")?
> >
> 
> Burn!

I would still prefer a long option (we already have a coremask parameter, so another one is kind-of non-essential and IMO shouldn't belong in a scarce resource of one-letter parameters), but if everyone else agrees, the "burn" option is much more preferable to me than complicating syntax of an already existing parameter.

Thanks,
Anatoly
Neil Horman Nov. 24, 2014, 2:01 p.m. UTC | #6
On Mon, Nov 24, 2014 at 01:37:03PM +0000, Burakov, Anatoly wrote:
> > > > > Do you want to burn an option letter on that?  It seems like it
> > > > > might be better to search the string for 0x and base the selection
> > > > > of bitmap of list parsing based on its presence or absence.
> > >
> > > It was the initial proposal (in April):
> > > 	http://dpdk.org/ml/archives/dev/2014-April/002173.html
> > > And I liked keeping only 1 option;
> > > 	http://dpdk.org/ml/archives/dev/2014-May/002722.html
> > > But Anatoly raised the compatibility problem:
> > > 	http://dpdk.org/ml/archives/dev/2014-May/002723.html
> > > Then there was no other comment so Didier and I reworked a separate
> > option.
> > >
> > > > The existing coremask parsing always assumes a hex coremask, so just
> > > > looking for a 0x will not work. I prefer this scheme of using a new
> > > > flag for this method of specifying the cores to use.
> > > >
> > > > If you don't want to use up a single-letter option, two alternatives:
> > > > 1) use a long option instead.
> > > > 2) if the -c parameter includes a "-" or a ",", treat it as a
> > > > new-style option, otherwise treat as old. The only abiguity here
> > > > would be for specifying a single core value 1-9 e.g. is "-c 6" a mask with
> > two bits, or a single-core to run on.
> > > > [0 is obviously a named core as it's an invalid mask, and A-F are
> > > > obviously masks.] If we did want this scheme, I would suggest that
> > > > we allow trailing commas in the list specifier, so we can force
> > > > users to clear ambiguity by either writing "0x6" or "6," i.e. disallow
> > ambiguous values to avoid problems.
> > > > However, this is probably more work that it's worth to avoid using
> > > > up a letter option.
> > > >
> > > > I'd prefer any of these options to breaking backward compatibility in this
> > case.
> > >
> > > We need a consensus here.
> > > Who is supporting a "burn" of an one-letter option with clear usage?
> > > Who is supporting a "re-merge" of the 2 syntaxes with more complicated
> > > rules (list syntax is triggered by presence of "-" or ",")?
> > >
> > 
> > Burn!
> 
> I would still prefer a long option (we already have a coremask parameter, so another one is kind-of non-essential and IMO shouldn't belong in a scarce resource of one-letter parameters), but if everyone else agrees, the "burn" option is much more preferable to me than complicating syntax of an already existing parameter.
> 
If we can't support two syntaxes with a single option (which still seems
reasonable to me, but I digress), then I would support the additional long
option.  Short options are not only scarse on their own, but they also have to
potentially comingle with options parsed by an application building on the DPDK.
At least with a long option the chances of a conflict are lessened.
Neil

> Thanks,
> Anatoly
>
Venkatesan, Venky Nov. 24, 2014, 2:52 p.m. UTC | #7
On 11/24/2014 5:28 AM, Bruce Richardson wrote:
> On Mon, Nov 24, 2014 at 02:19:16PM +0100, Thomas Monjalon wrote:
>> Hi Bruce and Neil,
>>
>> 2014-11-24 11:28, Bruce Richardson:
>>> On Sat, Nov 22, 2014 at 08:35:17PM -0500, Neil Horman wrote:
>>>> On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
>>>>> From: Didier Pallard <didier.pallard@6wind.com>
>>>>>
>>>>> In current version, used cores can only be specified using a bitmask.
>>>>> It will now be possible to specify cores in 2 different ways:
>>>>> - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
>>>>> - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
>>>>>
>>>>> The letter -l can stand for lcore or list.
>>>>>
>>>>> -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
>>>> Do you want to burn an option letter on that?  It seems like it might be better
>>>> to search the string for 0x and base the selection of bitmap of list parsing
>>>> based on its presence or absence.
>> It was the initial proposal (in April):
>> 	http://dpdk.org/ml/archives/dev/2014-April/002173.html
>> And I liked keeping only 1 option;
>> 	http://dpdk.org/ml/archives/dev/2014-May/002722.html
>> But Anatoly raised the compatibility problem:
>> 	http://dpdk.org/ml/archives/dev/2014-May/002723.html
>> Then there was no other comment so Didier and I reworked a separate option.
>>
>>> The existing coremask parsing always assumes a hex coremask, so just looking
>>> for a 0x will not work. I prefer this scheme of using a new flag for this method
>>> of specifying the cores to use.
>>>
>>> If you don't want to use up a single-letter option, two alternatives:
>>> 1) use a long option instead.
>>> 2) if the -c parameter includes a "-" or a ",", treat it as a new-style option,
>>> otherwise treat as old. The only abiguity here would be for specifying a single
>>> core value 1-9 e.g. is "-c 6" a mask with two bits, or a single-core to run on.
>>> [0 is obviously a named core as it's an invalid mask, and A-F are obviously
>>> masks.] If we did want this scheme, I would suggest that we allow trailing
>>> commas in the list specifier, so we can force users to clear ambiguity by
>>> either writing "0x6" or "6," i.e. disallow ambiguous values to avoid problems.
>>> However, this is probably more work that it's worth to avoid using up a letter
>>> option.
>>>
>>> I'd prefer any of these options to breaking backward compatibility in this case.
>> We need a consensus here.
>> Who is supporting a "burn" of an one-letter option with clear usage?
>> Who is supporting a "re-merge" of the 2 syntaxes with more complicated rules
>> (list syntax is triggered by presence of "-" or ",")?
>>
> Burn!
Burn ^ 2 ;)
Keith Wiles Nov. 24, 2014, 4:12 p.m. UTC | #8
Burn, it is not like we are going to add a huge number of new options in the future and run out of letters.

> On Nov 24, 2014, at 8:52 AM, Venkatesan, Venky <venky.venkatesan@intel.com> wrote:
> 
> 
> On 11/24/2014 5:28 AM, Bruce Richardson wrote:
>> On Mon, Nov 24, 2014 at 02:19:16PM +0100, Thomas Monjalon wrote:
>>> Hi Bruce and Neil,
>>> 
>>> 2014-11-24 11:28, Bruce Richardson:
>>>> On Sat, Nov 22, 2014 at 08:35:17PM -0500, Neil Horman wrote:
>>>>> On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
>>>>>> From: Didier Pallard <didier.pallard@6wind.com>
>>>>>> 
>>>>>> In current version, used cores can only be specified using a bitmask.
>>>>>> It will now be possible to specify cores in 2 different ways:
>>>>>> - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
>>>>>> - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
>>>>>> 
>>>>>> The letter -l can stand for lcore or list.
>>>>>> 
>>>>>> -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
>>>>> Do you want to burn an option letter on that?  It seems like it might be better
>>>>> to search the string for 0x and base the selection of bitmap of list parsing
>>>>> based on its presence or absence.
>>> It was the initial proposal (in April):
>>> 	http://dpdk.org/ml/archives/dev/2014-April/002173.html
>>> And I liked keeping only 1 option;
>>> 	http://dpdk.org/ml/archives/dev/2014-May/002722.html
>>> But Anatoly raised the compatibility problem:
>>> 	http://dpdk.org/ml/archives/dev/2014-May/002723.html
>>> Then there was no other comment so Didier and I reworked a separate option.
>>> 
>>>> The existing coremask parsing always assumes a hex coremask, so just looking
>>>> for a 0x will not work. I prefer this scheme of using a new flag for this method
>>>> of specifying the cores to use.
>>>> 
>>>> If you don't want to use up a single-letter option, two alternatives:
>>>> 1) use a long option instead.
>>>> 2) if the -c parameter includes a "-" or a ",", treat it as a new-style option,
>>>> otherwise treat as old. The only abiguity here would be for specifying a single
>>>> core value 1-9 e.g. is "-c 6" a mask with two bits, or a single-core to run on.
>>>> [0 is obviously a named core as it's an invalid mask, and A-F are obviously
>>>> masks.] If we did want this scheme, I would suggest that we allow trailing
>>>> commas in the list specifier, so we can force users to clear ambiguity by
>>>> either writing "0x6" or "6," i.e. disallow ambiguous values to avoid problems.
>>>> However, this is probably more work that it's worth to avoid using up a letter
>>>> option.
>>>> 
>>>> I'd prefer any of these options to breaking backward compatibility in this case.
>>> We need a consensus here.
>>> Who is supporting a "burn" of an one-letter option with clear usage?
>>> Who is supporting a "re-merge" of the 2 syntaxes with more complicated rules
>>> (list syntax is triggered by presence of "-" or ",")?
>>> 
>> Burn!
> Burn ^ 2 ;)
Neil Horman Nov. 24, 2014, 5:04 p.m. UTC | #9
On Mon, Nov 24, 2014 at 10:12:33AM -0600, Roger Keith Wiles wrote:
> Burn, it is not like we are going to add a huge number of new options in the future and run out of letters.
> 
No, but what about the application authors that need to accomodate all of the
dpdk command line options as well?
Neil

> > On Nov 24, 2014, at 8:52 AM, Venkatesan, Venky <venky.venkatesan@intel.com> wrote:
> > 
> > 
> > On 11/24/2014 5:28 AM, Bruce Richardson wrote:
> >> On Mon, Nov 24, 2014 at 02:19:16PM +0100, Thomas Monjalon wrote:
> >>> Hi Bruce and Neil,
> >>> 
> >>> 2014-11-24 11:28, Bruce Richardson:
> >>>> On Sat, Nov 22, 2014 at 08:35:17PM -0500, Neil Horman wrote:
> >>>>> On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
> >>>>>> From: Didier Pallard <didier.pallard@6wind.com>
> >>>>>> 
> >>>>>> In current version, used cores can only be specified using a bitmask.
> >>>>>> It will now be possible to specify cores in 2 different ways:
> >>>>>> - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
> >>>>>> - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
> >>>>>> 
> >>>>>> The letter -l can stand for lcore or list.
> >>>>>> 
> >>>>>> -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
> >>>>> Do you want to burn an option letter on that?  It seems like it might be better
> >>>>> to search the string for 0x and base the selection of bitmap of list parsing
> >>>>> based on its presence or absence.
> >>> It was the initial proposal (in April):
> >>> 	http://dpdk.org/ml/archives/dev/2014-April/002173.html
> >>> And I liked keeping only 1 option;
> >>> 	http://dpdk.org/ml/archives/dev/2014-May/002722.html
> >>> But Anatoly raised the compatibility problem:
> >>> 	http://dpdk.org/ml/archives/dev/2014-May/002723.html
> >>> Then there was no other comment so Didier and I reworked a separate option.
> >>> 
> >>>> The existing coremask parsing always assumes a hex coremask, so just looking
> >>>> for a 0x will not work. I prefer this scheme of using a new flag for this method
> >>>> of specifying the cores to use.
> >>>> 
> >>>> If you don't want to use up a single-letter option, two alternatives:
> >>>> 1) use a long option instead.
> >>>> 2) if the -c parameter includes a "-" or a ",", treat it as a new-style option,
> >>>> otherwise treat as old. The only abiguity here would be for specifying a single
> >>>> core value 1-9 e.g. is "-c 6" a mask with two bits, or a single-core to run on.
> >>>> [0 is obviously a named core as it's an invalid mask, and A-F are obviously
> >>>> masks.] If we did want this scheme, I would suggest that we allow trailing
> >>>> commas in the list specifier, so we can force users to clear ambiguity by
> >>>> either writing "0x6" or "6," i.e. disallow ambiguous values to avoid problems.
> >>>> However, this is probably more work that it's worth to avoid using up a letter
> >>>> option.
> >>>> 
> >>>> I'd prefer any of these options to breaking backward compatibility in this case.
> >>> We need a consensus here.
> >>> Who is supporting a "burn" of an one-letter option with clear usage?
> >>> Who is supporting a "re-merge" of the 2 syntaxes with more complicated rules
> >>> (list syntax is triggered by presence of "-" or ",")?
> >>> 
> >> Burn!
> > Burn ^ 2 ;)
> 
>
Keith Wiles Nov. 24, 2014, 5:09 p.m. UTC | #10
> On Nov 24, 2014, at 11:04 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> On Mon, Nov 24, 2014 at 10:12:33AM -0600, Roger Keith Wiles wrote:
>> Burn, it is not like we are going to add a huge number of new options in the future and run out of letters.
>> 
> No, but what about the application authors that need to accomodate all of the
> dpdk command line options as well?

The application authors are not effected. The application authors can use any options after the ‘--‘ as DPDK does not define these options correct except in the example applications.

> Neil
> 
>>> On Nov 24, 2014, at 8:52 AM, Venkatesan, Venky <venky.venkatesan@intel.com> wrote:
>>> 
>>> 
>>> On 11/24/2014 5:28 AM, Bruce Richardson wrote:
>>>> On Mon, Nov 24, 2014 at 02:19:16PM +0100, Thomas Monjalon wrote:
>>>>> Hi Bruce and Neil,
>>>>> 
>>>>> 2014-11-24 11:28, Bruce Richardson:
>>>>>> On Sat, Nov 22, 2014 at 08:35:17PM -0500, Neil Horman wrote:
>>>>>>> On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
>>>>>>>> From: Didier Pallard <didier.pallard@6wind.com>
>>>>>>>> 
>>>>>>>> In current version, used cores can only be specified using a bitmask.
>>>>>>>> It will now be possible to specify cores in 2 different ways:
>>>>>>>> - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
>>>>>>>> - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
>>>>>>>> 
>>>>>>>> The letter -l can stand for lcore or list.
>>>>>>>> 
>>>>>>>> -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
>>>>>>> Do you want to burn an option letter on that?  It seems like it might be better
>>>>>>> to search the string for 0x and base the selection of bitmap of list parsing
>>>>>>> based on its presence or absence.
>>>>> It was the initial proposal (in April):
>>>>> 	http://dpdk.org/ml/archives/dev/2014-April/002173.html
>>>>> And I liked keeping only 1 option;
>>>>> 	http://dpdk.org/ml/archives/dev/2014-May/002722.html
>>>>> But Anatoly raised the compatibility problem:
>>>>> 	http://dpdk.org/ml/archives/dev/2014-May/002723.html
>>>>> Then there was no other comment so Didier and I reworked a separate option.
>>>>> 
>>>>>> The existing coremask parsing always assumes a hex coremask, so just looking
>>>>>> for a 0x will not work. I prefer this scheme of using a new flag for this method
>>>>>> of specifying the cores to use.
>>>>>> 
>>>>>> If you don't want to use up a single-letter option, two alternatives:
>>>>>> 1) use a long option instead.
>>>>>> 2) if the -c parameter includes a "-" or a ",", treat it as a new-style option,
>>>>>> otherwise treat as old. The only abiguity here would be for specifying a single
>>>>>> core value 1-9 e.g. is "-c 6" a mask with two bits, or a single-core to run on.
>>>>>> [0 is obviously a named core as it's an invalid mask, and A-F are obviously
>>>>>> masks.] If we did want this scheme, I would suggest that we allow trailing
>>>>>> commas in the list specifier, so we can force users to clear ambiguity by
>>>>>> either writing "0x6" or "6," i.e. disallow ambiguous values to avoid problems.
>>>>>> However, this is probably more work that it's worth to avoid using up a letter
>>>>>> option.
>>>>>> 
>>>>>> I'd prefer any of these options to breaking backward compatibility in this case.
>>>>> We need a consensus here.
>>>>> Who is supporting a "burn" of an one-letter option with clear usage?
>>>>> Who is supporting a "re-merge" of the 2 syntaxes with more complicated rules
>>>>> (list syntax is triggered by presence of "-" or ",")?
>>>>> 
>>>> Burn!
>>> Burn ^ 2 ;)
>> 
>>
Burakov, Anatoly Nov. 24, 2014, 5:11 p.m. UTC | #11
> On Mon, Nov 24, 2014 at 10:12:33AM -0600, Roger Keith Wiles wrote:
> > Burn, it is not like we are going to add a huge number of new options in the
> future and run out of letters.
> >
> No, but what about the application authors that need to accomodate all of
> the dpdk command line options as well?
> Neil

Hi Neil

I don't think that's a problem as all those parameters are separated with a --. I.e. ./testpmd -c f -n 2 -- <testpmd parameters>. Is that not the case?

Thanks,
Anatoly
Neil Horman Nov. 24, 2014, 5:17 p.m. UTC | #12
On Mon, Nov 24, 2014 at 05:11:16PM +0000, Burakov, Anatoly wrote:
> > On Mon, Nov 24, 2014 at 10:12:33AM -0600, Roger Keith Wiles wrote:
> > > Burn, it is not like we are going to add a huge number of new options in the
> > future and run out of letters.
> > >
> > No, but what about the application authors that need to accomodate all of
> > the dpdk command line options as well?
> > Neil
> 
> Hi Neil
> 
> I don't think that's a problem as all those parameters are separated with a --. I.e. ./testpmd -c f -n 2 -- <testpmd parameters>. Is that not the case?
> 
I don't know, I've not tried, though I'm still not a fan of having a command
line that takes the form:

<application> -c 0xnnnn -- -c ...

That just looks confusing to me.
Neil

> Thanks,
> Anatoly
>
Bruce Richardson Nov. 25, 2014, 10:45 a.m. UTC | #13
On Sat, Nov 22, 2014 at 10:43:39PM +0100, Thomas Monjalon wrote:
> From: Didier Pallard <didier.pallard@6wind.com>
> 
> In current version, used cores can only be specified using a bitmask.
> It will now be possible to specify cores in 2 different ways:
> - Using a bitmask (-c [0x]nnn): bitmask must be in hex format
> - Using a list in following format: -l <c1>[-c2][,c3[-c4],...]
> 
> The letter -l can stand for lcore or list.
> 
> -l 0-7,16-23,31 being equivalent to -c 0x80FF00FF
> 
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Majority of people seem happy enough using the -l flag, so

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  app/test/test_eal_flags.c                  | 28 ++++++++++-
>  lib/librte_eal/common/eal_common_options.c | 81 ++++++++++++++++++++++++++++--
>  2 files changed, 103 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 1f95d7f..5ad89c5 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -484,7 +484,7 @@ test_invalid_r_flag(void)
>  }
>  
>  /*
> - * Test that the app doesn't run without the coremask flag. In all cases
> + * Test that the app doesn't run without the coremask/corelist flags. In all cases
>   * should give an error and fail to run
>   */
>  static int
> @@ -504,12 +504,22 @@ test_missing_c_flag(void)
>  
>  	/* -c flag but no coremask value */
>  	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "3", "-c"};
> -	/* No -c flag at all */
> +	/* No -c or -l flag at all */
>  	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "3"};
>  	/* bad coremask value */
>  	const char *argv3[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "error" };
>  	/* sanity check of tests - valid coremask value */
>  	const char *argv4[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "1" };
> +	/* -l flag but no corelist value */
> +	const char *argv5[] = { prgname, prefix, mp_flag, "-n", "3", "-l"};
> +	const char *argv6[] = { prgname, prefix, mp_flag, "-n", "3", "-l", " " };
> +	/* bad corelist values */
> +	const char *argv7[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "error" };
> +	const char *argv8[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-" };
> +	const char *argv9[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1," };
> +	const char *argv10[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1#2" };
> +	/* sanity check test - valid corelist value */
> +	const char *argv11[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-2,3" };
>  
>  	if (launch_proc(argv1) == 0
>  			|| launch_proc(argv2) == 0
> @@ -521,6 +531,20 @@ test_missing_c_flag(void)
>  		printf("Error - process did not run ok with valid coremask value\n");
>  		return -1;
>  	}
> +
> +	if (launch_proc(argv5) == 0
> +			|| launch_proc(argv6) == 0
> +			|| launch_proc(argv7) == 0
> +			|| launch_proc(argv8) == 0
> +			|| launch_proc(argv9) == 0
> +			|| launch_proc(argv10) == 0) {
> +		printf("Error - process ran without error with invalid -l flag\n");
> +		return -1;
> +	}
> +	if (launch_proc(argv11) != 0) {
> +		printf("Error - process did not run ok with valid corelist value\n");
> +		return -1;
> +	}
>  	return 0;
>  }
>  
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 63710b0..18d03e3 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -32,6 +32,7 @@
>   */
>  
>  #include <stdlib.h>
> +#include <unistd.h>
>  #include <string.h>
>  #include <syslog.h>
>  #include <ctype.h>
> @@ -55,8 +56,9 @@ const char
>  eal_short_options[] =
>  	"b:" /* pci-blacklist */
>  	"w:" /* pci-whitelist */
> -	"c:"
> +	"c:" /* coremask */
>  	"d:"
> +	"l:" /* corelist */
>  	"m:"
>  	"n:"
>  	"r:"
> @@ -205,6 +207,67 @@ eal_parse_coremask(const char *coremask)
>  }
>  
>  static int
> +eal_parse_corelist(const char *corelist)
> +{
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +	int i, idx = 0;
> +	unsigned count = 0;
> +	char *end = NULL;
> +	int min, max;
> +
> +	if (corelist == NULL)
> +		return -1;
> +
> +	/* Remove all blank characters ahead and after */
> +	while (isblank(*corelist))
> +		corelist++;
> +	i = strnlen(corelist, sysconf(_SC_ARG_MAX));
> +	while ((i > 0) && isblank(corelist[i - 1]))
> +		i--;
> +
> +	/* Reset core roles */
> +	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
> +		cfg->lcore_role[idx] = ROLE_OFF;
> +
> +	/* Get list of cores */
> +	min = RTE_MAX_LCORE;
> +	do {
> +		while (isblank(*corelist))
> +			corelist++;
> +		if (*corelist == '\0')
> +			return -1;
> +		errno = 0;
> +		idx = strtoul(corelist, &end, 10);
> +		if (errno || end == NULL)
> +			return -1;
> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-') {
> +			min = idx;
> +		} else if ((*end == ',') || (*end == '\0')) {
> +			max = idx;
> +			if (min == RTE_MAX_LCORE)
> +				min = idx;
> +			for (idx = min; idx <= max; idx++) {
> +				cfg->lcore_role[idx] = ROLE_RTE;
> +				if (count == 0)
> +					cfg->master_lcore = idx;
> +				count++;
> +			}
> +			min = RTE_MAX_LCORE;
> +		} else
> +			return -1;
> +		corelist = end + 1;
> +	} while (*end != '\0');
> +
> +	if (count == 0)
> +		return -1;
> +
> +	lcores_parsed = 1;
> +	return 0;
> +}
> +
> +static int
>  eal_parse_syslog(const char *facility, struct internal_config *conf)
>  {
>  	int i;
> @@ -304,6 +367,13 @@ eal_parse_common_option(int opt, const char *optarg,
>  			return -1;
>  		}
>  		break;
> +	/* corelist */
> +	case 'l':
> +		if (eal_parse_corelist(optarg) < 0) {
> +			RTE_LOG(ERR, EAL, "invalid core list\n");
> +			return -1;
> +		}
> +		break;
>  	/* size of memory */
>  	case 'm':
>  		conf->memory = atoi(optarg);
> @@ -421,8 +491,8 @@ eal_check_common_options(struct internal_config *internal_cfg)
>  	struct rte_config *cfg = rte_eal_get_configuration();
>  
>  	if (!lcores_parsed) {
> -		RTE_LOG(ERR, EAL, "CPU cores must be enabled with option "
> -			"-c\n");
> +		RTE_LOG(ERR, EAL, "CPU cores must be enabled with options "
> +			"-c or -l\n");
>  		return -1;
>  	}
>  
> @@ -470,6 +540,9 @@ eal_common_usage(void)
>  	       "[--proc-type primary|secondary|auto]\n\n"
>  	       "EAL common options:\n"
>  	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
> +	       "  -l CORELIST  : List of cores to run on\n"
> +	       "                 The argument format is <c1>[-c2][,c3[-c4],...]\n"
> +	       "                 where c1, c2, etc are core indexes between 0 and %d\n"
>  	       "  -n NUM       : Number of memory channels\n"
>  	       "  -v           : Display version information on startup\n"
>  	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
> @@ -494,5 +567,5 @@ eal_common_usage(void)
>  	       "  --"OPT_NO_PCI"   : disable pci\n"
>  	       "  --"OPT_NO_HPET"  : disable hpet\n"
>  	       "  --"OPT_NO_SHCONF": no shared config (mmap'd files)\n"
> -	       "\n");
> +	       "\n", RTE_MAX_LCORE);
>  }
> -- 
> 2.1.3
>
diff mbox

Patch

diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index 1f95d7f..5ad89c5 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -484,7 +484,7 @@  test_invalid_r_flag(void)
 }
 
 /*
- * Test that the app doesn't run without the coremask flag. In all cases
+ * Test that the app doesn't run without the coremask/corelist flags. In all cases
  * should give an error and fail to run
  */
 static int
@@ -504,12 +504,22 @@  test_missing_c_flag(void)
 
 	/* -c flag but no coremask value */
 	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "3", "-c"};
-	/* No -c flag at all */
+	/* No -c or -l flag at all */
 	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "3"};
 	/* bad coremask value */
 	const char *argv3[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "error" };
 	/* sanity check of tests - valid coremask value */
 	const char *argv4[] = { prgname, prefix, mp_flag, "-n", "3", "-c", "1" };
+	/* -l flag but no corelist value */
+	const char *argv5[] = { prgname, prefix, mp_flag, "-n", "3", "-l"};
+	const char *argv6[] = { prgname, prefix, mp_flag, "-n", "3", "-l", " " };
+	/* bad corelist values */
+	const char *argv7[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "error" };
+	const char *argv8[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-" };
+	const char *argv9[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1," };
+	const char *argv10[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1#2" };
+	/* sanity check test - valid corelist value */
+	const char *argv11[] = { prgname, prefix, mp_flag, "-n", "3", "-l", "1-2,3" };
 
 	if (launch_proc(argv1) == 0
 			|| launch_proc(argv2) == 0
@@ -521,6 +531,20 @@  test_missing_c_flag(void)
 		printf("Error - process did not run ok with valid coremask value\n");
 		return -1;
 	}
+
+	if (launch_proc(argv5) == 0
+			|| launch_proc(argv6) == 0
+			|| launch_proc(argv7) == 0
+			|| launch_proc(argv8) == 0
+			|| launch_proc(argv9) == 0
+			|| launch_proc(argv10) == 0) {
+		printf("Error - process ran without error with invalid -l flag\n");
+		return -1;
+	}
+	if (launch_proc(argv11) != 0) {
+		printf("Error - process did not run ok with valid corelist value\n");
+		return -1;
+	}
 	return 0;
 }
 
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 63710b0..18d03e3 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -32,6 +32,7 @@ 
  */
 
 #include <stdlib.h>
+#include <unistd.h>
 #include <string.h>
 #include <syslog.h>
 #include <ctype.h>
@@ -55,8 +56,9 @@  const char
 eal_short_options[] =
 	"b:" /* pci-blacklist */
 	"w:" /* pci-whitelist */
-	"c:"
+	"c:" /* coremask */
 	"d:"
+	"l:" /* corelist */
 	"m:"
 	"n:"
 	"r:"
@@ -205,6 +207,67 @@  eal_parse_coremask(const char *coremask)
 }
 
 static int
+eal_parse_corelist(const char *corelist)
+{
+	struct rte_config *cfg = rte_eal_get_configuration();
+	int i, idx = 0;
+	unsigned count = 0;
+	char *end = NULL;
+	int min, max;
+
+	if (corelist == NULL)
+		return -1;
+
+	/* Remove all blank characters ahead and after */
+	while (isblank(*corelist))
+		corelist++;
+	i = strnlen(corelist, sysconf(_SC_ARG_MAX));
+	while ((i > 0) && isblank(corelist[i - 1]))
+		i--;
+
+	/* Reset core roles */
+	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
+		cfg->lcore_role[idx] = ROLE_OFF;
+
+	/* Get list of cores */
+	min = RTE_MAX_LCORE;
+	do {
+		while (isblank(*corelist))
+			corelist++;
+		if (*corelist == '\0')
+			return -1;
+		errno = 0;
+		idx = strtoul(corelist, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = idx;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = idx;
+			if (min == RTE_MAX_LCORE)
+				min = idx;
+			for (idx = min; idx <= max; idx++) {
+				cfg->lcore_role[idx] = ROLE_RTE;
+				if (count == 0)
+					cfg->master_lcore = idx;
+				count++;
+			}
+			min = RTE_MAX_LCORE;
+		} else
+			return -1;
+		corelist = end + 1;
+	} while (*end != '\0');
+
+	if (count == 0)
+		return -1;
+
+	lcores_parsed = 1;
+	return 0;
+}
+
+static int
 eal_parse_syslog(const char *facility, struct internal_config *conf)
 {
 	int i;
@@ -304,6 +367,13 @@  eal_parse_common_option(int opt, const char *optarg,
 			return -1;
 		}
 		break;
+	/* corelist */
+	case 'l':
+		if (eal_parse_corelist(optarg) < 0) {
+			RTE_LOG(ERR, EAL, "invalid core list\n");
+			return -1;
+		}
+		break;
 	/* size of memory */
 	case 'm':
 		conf->memory = atoi(optarg);
@@ -421,8 +491,8 @@  eal_check_common_options(struct internal_config *internal_cfg)
 	struct rte_config *cfg = rte_eal_get_configuration();
 
 	if (!lcores_parsed) {
-		RTE_LOG(ERR, EAL, "CPU cores must be enabled with option "
-			"-c\n");
+		RTE_LOG(ERR, EAL, "CPU cores must be enabled with options "
+			"-c or -l\n");
 		return -1;
 	}
 
@@ -470,6 +540,9 @@  eal_common_usage(void)
 	       "[--proc-type primary|secondary|auto]\n\n"
 	       "EAL common options:\n"
 	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
+	       "  -l CORELIST  : List of cores to run on\n"
+	       "                 The argument format is <c1>[-c2][,c3[-c4],...]\n"
+	       "                 where c1, c2, etc are core indexes between 0 and %d\n"
 	       "  -n NUM       : Number of memory channels\n"
 	       "  -v           : Display version information on startup\n"
 	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
@@ -494,5 +567,5 @@  eal_common_usage(void)
 	       "  --"OPT_NO_PCI"   : disable pci\n"
 	       "  --"OPT_NO_HPET"  : disable hpet\n"
 	       "  --"OPT_NO_SHCONF": no shared config (mmap'd files)\n"
-	       "\n");
+	       "\n", RTE_MAX_LCORE);
 }