Re: [dpdk-dev] [PATCH v4 02/17] eal: new eal option '--lcores' for cpu assignment

["Liang, Cunming" <cunming.liang@intel.com>]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 09, 2015 4:00 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 02/17] eal: new eal option '--lcores' for cpu
> assignment
> 
> Hi,
> 
> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> > It supports one new eal long option '--lcores' for EAL thread cpuset assignment.
> >
> > The format pattern:
> > 	--lcores='lcores[@cpus]<,lcores[@cpus]>'
> > lcores, cpus could be a single digit/range or a group.
> > '(' and ')' are necessary if it's a group.
> > If not supply '@cpus', the value of cpus uses the same as lcores.
> >
> > e.g. '1,2@(5-7),(3-5)@(0,2),(0,6),7-8' means starting 9 EAL thread as below
> >   lcore 0 runs on cpuset 0x41 (cpu 0,6)
> >   lcore 1 runs on cpuset 0x2 (cpu 1)
> >   lcore 2 runs on cpuset 0xe0 (cpu 5,6,7)
> >   lcore 3,4,5 runs on cpuset 0x5 (cpu 0,2)
> >   lcore 6 runs on cpuset 0x41 (cpu 0,6)
> >   lcore 7 runs on cpuset 0x80 (cpu 7)
> >   lcore 8 runs on cpuset 0x100 (cpu 8)
> >
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > ---
> >  lib/librte_eal/common/eal_common_launch.c  |   1 -
> >  lib/librte_eal/common/eal_common_options.c | 300
> ++++++++++++++++++++++++++++-
> >  lib/librte_eal/common/eal_options.h        |   2 +
> >  lib/librte_eal/linuxapp/eal/Makefile       |   1 +
> >  4 files changed, 299 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_launch.c
> b/lib/librte_eal/common/eal_common_launch.c
> > index 599f83b..2d732b1 100644
> > --- a/lib/librte_eal/common/eal_common_launch.c
> > +++ b/lib/librte_eal/common/eal_common_launch.c
> > @@ -117,4 +117,3 @@ rte_eal_mp_wait_lcore(void)
> >  		rte_eal_wait_lcore(lcore_id);
> >  	}
> >  }
> > -
> 
> 
> This line should be removed from the patch.
[LCM] Accept.
> 
> 
> > diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> > index 67e02dc..29ebb6f 100644
> > --- a/lib/librte_eal/common/eal_common_options.c
> > +++ b/lib/librte_eal/common/eal_common_options.c
> > @@ -45,6 +45,7 @@
> >  #include <rte_lcore.h>
> >  #include <rte_version.h>
> >  #include <rte_devargs.h>
> > +#include <rte_memcpy.h>
> >
> >  #include "eal_internal_cfg.h"
> >  #include "eal_options.h"
> > @@ -85,6 +86,7 @@ eal_long_options[] = {
> >  	{OPT_XEN_DOM0, 0, 0, OPT_XEN_DOM0_NUM},
> >  	{OPT_CREATE_UIO_DEV, 1, NULL, OPT_CREATE_UIO_DEV_NUM},
> >  	{OPT_VFIO_INTR, 1, NULL, OPT_VFIO_INTR_NUM},
> > +	{OPT_LCORES, 1, 0, OPT_LCORES_NUM},
> >  	{0, 0, 0, 0}
> >  };
> >
> > @@ -255,9 +257,11 @@ eal_parse_corelist(const char *corelist)
> >  			if (min == RTE_MAX_LCORE)
> >  				min = idx;
> >  			for (idx = min; idx <= max; idx++) {
> > -				cfg->lcore_role[idx] = ROLE_RTE;
> > -				lcore_config[idx].core_index = count;
> > -				count++;
> > +				if (cfg->lcore_role[idx] != ROLE_RTE) {
> > +					cfg->lcore_role[idx] = ROLE_RTE;
> > +					lcore_config[idx].core_index = count;
> > +					count++;
> > +				}
> >  			}
> >  			min = RTE_MAX_LCORE;
> >  		} else
> > @@ -292,6 +296,279 @@ eal_parse_master_lcore(const char *arg)
> >  	return 0;
> >  }
> >
> > +/*
> > + * Parse elem, the elem could be single number/range or '(' ')' group
> > + * Within group elem, '-' used for a range seperator;
> > + *                    ',' used for a single number.
> > + */
> > +static int
> > +eal_parse_set(const char *input, uint16_t set[], unsigned num)
> 
> It's not very clear what elem is. Maybe it could be a bit reworded.
> What about naming the function "eal_parse_cpuset()" instead?
[LCM] As it not only parse cpuset but also used for lcore set, so 'eal_parse_cpuset' is not accurate.
The set/elem here identify for a single number (e.g. 1), a number range (e.g. 4-6) or a group (e.g. (3,4-8,9) ).
I'll reword the comment for better understand. Thanks.
> 
> 
> > +{
> > +	unsigned idx;
> > +	const char *str = input;
> > +	char *end = NULL;
> > +	unsigned min, max;
> > +
> > +	memset(set, 0, num * sizeof(uint16_t));
> > +
> > +	while (isblank(*str))
> > +		str++;
> > +
> > +	/* only digit or left bracket is qulify for start point */
> > +	if ((!isdigit(*str) && *str != '(') || *str == '\0')
> > +		return -1;
> > +
> > +	/* process single number or single range of number */
> > +	if (*str != '(') {
> > +		errno = 0;
> > +		idx = strtoul(str, &end, 10);
> > +		if (errno || end == NULL || idx >= num)
> > +			return -1;
> > +		else {
> > +			while (isblank(*end))
> > +				end++;
> > +
> > +			min = idx;
> > +			max = idx;
> > +			if (*end == '-') {
> > +				/* proccess single <number>-<number> */
> > +				end++;
> > +				while (isblank(*end))
> > +					end++;
> > +				if (!isdigit(*end))
> > +					return -1;
> > +
> > +				errno = 0;
> > +				idx = strtoul(end, &end, 10);
> > +				if (errno || end == NULL || idx >= num)
> > +					return -1;
> > +				max = idx;
> > +				while (isblank(*end))
> > +					end++;
> > +				if (*end != ',' && *end != '\0')
> > +					return -1;
> > +			}
> > +
> > +			if (*end != ',' && *end != '\0' &&
> > +			    *end != '@')
> > +				return -1;
> > +
> > +			for (idx = RTE_MIN(min, max);
> > +			     idx <= RTE_MAX(min, max); idx++)
> > +				set[idx] = 1;
> > +
> > +			return end - input;
> > +		}
> > +	}
> > +
> > +	/* process set within bracket */
> > +	str++;
> > +	while (isblank(*str))
> > +		str++;
> > +	if (*str == '\0')
> > +		return -1;
> > +
> > +	min = RTE_MAX_LCORE;
> > +	do {
> > +
> > +		/* go ahead to the first digit */
> > +		while (isblank(*str))
> > +			str++;
> > +		if (!isdigit(*str))
> > +			return -1;
> > +
> > +		/* get the digit value */
> > +		errno = 0;
> > +		idx = strtoul(str, &end, 10);
> > +		if (errno || end == NULL || idx >= num)
> > +			return -1;
> > +
> > +		/* go ahead to separator '-',',' and ')' */
> > +		while (isblank(*end))
> > +			end++;
> > +		if (*end == '-') {
> > +			if (min == RTE_MAX_LCORE)
> > +				min = idx;
> > +			else /* avoid continuous '-' */
> > +				return -1;
> > +		} else if ((*end == ',') || (*end == ')')) {
> > +			max = idx;
> > +			if (min == RTE_MAX_LCORE)
> > +				min = idx;
> > +			for (idx = RTE_MIN(min, max);
> > +			     idx <= RTE_MAX(min, max); idx++)
> > +				set[idx] = 1;
> > +
> > +			min = RTE_MAX_LCORE;
> > +		} else
> > +			return -1;
> > +
> > +		str = end + 1;
> > +	} while (*end != '\0' && *end != ')');
> > +
> > +	return str - input;
> > +}
> 
> In the function above, there are some typos in the comments
>  seperator -> separator
>  qulify -> qualify
>  proccess -> process
[LCM] Sorry for that, will fix it.
> 
> > +
> > +/* convert from set array to cpuset bitmap */
> > +static inline int
> > +convert_to_cpuset(rte_cpuset_t *cpusetp,
> > +	      uint16_t *set, unsigned num)
> 
> I don't think the function should be inlined
[LCM] accept.
> 
> 
> 
> Regards,
> Olivier