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

[Bruce Richardson <bruce.richardson@intel.com>]

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