[PATCH] eal: fix parsing of argument of option --lcores

[PATCH] eal: fix parsing of argument of option --lcores

[Wei Dai]

The '-' in lcores set overrides cpu set of following
lcore set in the argument of EAL option --lcores.

Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 0a594d7..96eb1a9 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -563,6 +563,7 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
  * 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.
+ * The 'a-b' in lcores not within '(' and ')' means a,a+1,...,b-1,b .
  * e.g. '1,2@(5-7),(3-5)@(0,2),(0,6),7-8' means start 9 EAL thread as below
  *   lcore 0 runs on cpuset 0x41 (cpu 0,6)
  *   lcore 1 runs on cpuset 0x2 (cpu 1)
@@ -571,6 +572,15 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
  *   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)
+ * e.g. '0-2,(3-5)@(3,4),6@(5,6),7@(5-7)'means start 8 EAL threads as below
+ *   lcore 0 runs on cpuset 0x1 (cpu 0)
+ *   lcore 1 runs on cpuset 0x2 (cpu 1)
+ *   lcore 2 runs on cpuset ox4 (cpu 2)
+ *   lcore 3,4,5 runs on cpuset 0x18 (cpu 3,4)
+ *   lcore 6 runs on cpuset 0x60 (cpu 5,6)
+ *   lcore 7 runs on cpuset 0xe0 (cpu 5,6,7)
+ * The second case is used to test bugfix for lflags not be cleared after use
+ */
  */
 static int
 eal_parse_lcores(const char *lcores)
@@ -679,6 +689,8 @@ eal_parse_lcores(const char *lcores)
 				   sizeof(rte_cpuset_t));
 		}
 
+		lflags = 0;
+
 		lcores = end + 1;
 	} while (*end != '\0');
 
-- 
2.5.5

Re: [PATCH] eal: fix parsing of argument of option --lcores

[Thomas Monjalon]

Hi,

2016-07-21 14:03, Wei Dai:
> The '-' in lcores set overrides cpu set of following
> lcore set in the argument of EAL option --lcores.
> 
> Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>

Thanks for the catch!

> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -563,6 +563,7 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
>   * 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.
> + * The 'a-b' in lcores not within '(' and ')' means a,a+1,...,b-1,b .

It also a range when inside a group.
The difference is the mapping to the cpus.
I think this new comment brings more confusion. Better to skip.

>   * e.g. '1,2@(5-7),(3-5)@(0,2),(0,6),7-8' means start 9 EAL thread as below
>   *   lcore 0 runs on cpuset 0x41 (cpu 0,6)
>   *   lcore 1 runs on cpuset 0x2 (cpu 1)
> @@ -571,6 +572,15 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
>   *   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)
> + * e.g. '0-2,(3-5)@(3,4),6@(5,6),7@(5-7)'means start 8 EAL threads as below
> + *   lcore 0 runs on cpuset 0x1 (cpu 0)
> + *   lcore 1 runs on cpuset 0x2 (cpu 1)
> + *   lcore 2 runs on cpuset ox4 (cpu 2)
> + *   lcore 3,4,5 runs on cpuset 0x18 (cpu 3,4)
> + *   lcore 6 runs on cpuset 0x60 (cpu 5,6)
> + *   lcore 7 runs on cpuset 0xe0 (cpu 5,6,7)
> + * The second case is used to test bugfix for lflags not be cleared after use
> + */
>   */

Please do not add a second example just to show how to test your fix.

> @@ -679,6 +689,8 @@ eal_parse_lcores(const char *lcores)
>  				   sizeof(rte_cpuset_t));
>  		}
>  
> +		lflags = 0;
> +
>  		lcores = end + 1;
>  	} while (*end != '\0');

It would have more sense to init lflags at the beginning of the loop
and replace
	int lflags = 0;
by
	int lflags;

Re: [PATCH] eal: fix parsing of argument of option --lcores

[bynes adam]

On Thu, Jul 21, 2016 at 02:03:38PM +0800, Wei Dai wrote:
Hi Wei,
> The '-' in lcores set overrides cpu set of following
> lcore set in the argument of EAL option --lcores.
> 
> Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 0a594d7..96eb1a9 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -563,6 +563,7 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
>   * 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.
> + * The 'a-b' in lcores not within '(' and ')' means a,a+1,...,b-1,b .
this description is not very clear, a-b and (a-b) are both the same meaning.
may be need a table for comparison
a-b@(c-d)
a-b@c-d
(a-b)@c-d
(a-b)@(c-d)
all the above I believe are the same
only the following two cases:
a-b,
(a-b),
so the key point here is the @ and (), not only @
>   * e.g. '1,2@(5-7),(3-5)@(0,2),(0,6),7-8' means start 9 EAL thread as below
>   *   lcore 0 runs on cpuset 0x41 (cpu 0,6)
>   *   lcore 1 runs on cpuset 0x2 (cpu 1)
> @@ -571,6 +572,15 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
>   *   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)
> + * e.g. '0-2,(3-5)@(3,4),6@(5,6),7@(5-7)'means start 8 EAL threads as below
> + *   lcore 0 runs on cpuset 0x1 (cpu 0)
> + *   lcore 1 runs on cpuset 0x2 (cpu 1)
> + *   lcore 2 runs on cpuset ox4 (cpu 2)
> + *   lcore 3,4,5 runs on cpuset 0x18 (cpu 3,4)
> + *   lcore 6 runs on cpuset 0x60 (cpu 5,6)
> + *   lcore 7 runs on cpuset 0xe0 (cpu 5,6,7)
> + * The second case is used to test bugfix for lflags not be cleared after use
you can put this sentance and description into the commit log
I don't think you should put bugfix description in comments here.
> + */
>   */
>  static int
>  eal_parse_lcores(const char *lcores)
> @@ -679,6 +689,8 @@ eal_parse_lcores(const char *lcores)
>  				   sizeof(rte_cpuset_t));
>  		}
>  
> +		lflags = 0;
> +
>  		lcores = end + 1;
>  	} while (*end != '\0');
>  
> -- 
> 2.5.5
Adam Bynes

Re: [PATCH] eal: fix parsing of argument of option --lcores

[Dai, Wei]

Hi Bynes
Thanks for your feedback.

> -----Original Message-----
> From: bynes adam [mailto:adambynes@outlook.com]
> Sent: Friday, July 22, 2016 4:45 AM
> To: Dai, Wei <wei.dai@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: fix parsing of argument of option --lcores
> 
> On Thu, Jul 21, 2016 at 02:03:38PM +0800, Wei Dai wrote:
> Hi Wei,
> > The '-' in lcores set overrides cpu set of following lcore set in the
> > argument of EAL option --lcores.
> >
> > Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> >
> > Signed-off-by: Wei Dai <wei.dai@intel.com>
> > ---
> >  lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/eal_common_options.c
> > b/lib/librte_eal/common/eal_common_options.c
> > index 0a594d7..96eb1a9 100644
> > --- a/lib/librte_eal/common/eal_common_options.c
> > +++ b/lib/librte_eal/common/eal_common_options.c
> > @@ -563,6 +563,7 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
> >   * 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.
> > + * The 'a-b' in lcores not within '(' and ')' means a,a+1,...,b-1,b .
> this description is not very clear, a-b and (a-b) are both the same meaning.
> may be need a table for comparison
> a-b@(c-d)
> a-b@c-d
> (a-b)@c-d
> (a-b)@(c-d)
> all the above I believe are the same
> only the following two cases:
> a-b,
> (a-b),

With --lcores '0-3@12-15', eal_parse_cores( ) will fail because eal_parse_set( )
find the next char after lcore set is '@' and is not ',' or '\0'.
So the bug in eal_parse_set( ) should be fixed. 
A patch v3 will be provided.
After fixing, I test it with --lcores '0-3@12-15, 4-7@(8-11),  (8-11)@4-7, (12-15)@(0-3), 16-19, (20-23) '
It works well.

Thanks
Wei

> so the key point here is the @ and (), not only @
> >   * e.g. '1,2@(5-7),(3-5)@(0,2),(0,6),7-8' means start 9 EAL thread as below
> >   *   lcore 0 runs on cpuset 0x41 (cpu 0,6)
> >   *   lcore 1 runs on cpuset 0x2 (cpu 1)
> > @@ -571,6 +572,15 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
> >   *   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)
> > + * e.g. '0-2,(3-5)@(3,4),6@(5,6),7@(5-7)'means start 8 EAL threads as
> below
> > + *   lcore 0 runs on cpuset 0x1 (cpu 0)
> > + *   lcore 1 runs on cpuset 0x2 (cpu 1)
> > + *   lcore 2 runs on cpuset ox4 (cpu 2)
> > + *   lcore 3,4,5 runs on cpuset 0x18 (cpu 3,4)
> > + *   lcore 6 runs on cpuset 0x60 (cpu 5,6)
> > + *   lcore 7 runs on cpuset 0xe0 (cpu 5,6,7)
> > + * The second case is used to test bugfix for lflags not be cleared
> > + after use
> you can put this sentance and description into the commit log I don't think you
> should put bugfix description in comments here.
> > + */
> >   */
> >  static int
> >  eal_parse_lcores(const char *lcores)
> > @@ -679,6 +689,8 @@ eal_parse_lcores(const char *lcores)
> >  				   sizeof(rte_cpuset_t));
> >  		}
> >
> > +		lflags = 0;
> > +
> >  		lcores = end + 1;
> >  	} while (*end != '\0');
> >
> > --
> > 2.5.5
> Adam Bynes

[PATCH v2 1/2] eal: remove redundant codes to parse --lcores

[Wei Dai]

local variable i is not referred by other codes in
the function eal_parse_lcores( ), so it can be removed.

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 481c732..c5bf98c 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -578,7 +578,6 @@ eal_parse_lcores(const char *lcores)
 	struct rte_config *cfg = rte_eal_get_configuration();
 	static uint16_t set[RTE_MAX_LCORE];
 	unsigned idx = 0;
-	int i;
 	unsigned count = 0;
 	const char *lcore_start = NULL;
 	const char *end = NULL;
@@ -593,9 +592,6 @@ eal_parse_lcores(const char *lcores)
 	/* Remove all blank characters ahead and after */
 	while (isblank(*lcores))
 		lcores++;
-	i = strlen(lcores);
-	while ((i > 0) && isblank(lcores[i - 1]))
-		i--;
 
 	CPU_ZERO(&cpuset);
 
-- 
2.5.5

Re: [PATCH v2 1/2] eal: remove redundant codes to parse --lcores

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Dai
> Sent: Tuesday, July 26, 2016 10:52 AM
> To: dev@dpdk.org
> Cc: Dai, Wei <wei.dai@intel.com>
> Subject: [dpdk-dev] [PATCH v2 1/2] eal: remove redundant codes to parse --lcores
> 
> local variable i is not referred by other codes in the function eal_parse_lcores( ), so it can be removed.
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  lib/librte_eal/common/eal_common_options.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 481c732..c5bf98c 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -578,7 +578,6 @@ eal_parse_lcores(const char *lcores)
>  	struct rte_config *cfg = rte_eal_get_configuration();
>  	static uint16_t set[RTE_MAX_LCORE];
>  	unsigned idx = 0;
> -	int i;
>  	unsigned count = 0;
>  	const char *lcore_start = NULL;
>  	const char *end = NULL;
> @@ -593,9 +592,6 @@ eal_parse_lcores(const char *lcores)
>  	/* Remove all blank characters ahead and after */
>  	while (isblank(*lcores))
>  		lcores++;
> -	i = strlen(lcores);
> -	while ((i > 0) && isblank(lcores[i - 1]))
> -		i--;

I suppose originally it meant to do something  like that:
while ((i > 0) && isblank(lcores[i - 1]))
	lcores[i--] = 0;

to get rid of blank characters at the end of the line, no?
Konstantin

> 
>  	CPU_ZERO(&cpuset);
> 
> --
> 2.5.5

Re: [PATCH v2 1/2] eal: remove redundant codes to parse --lcores

[Adam Bynes]

On Tue, Jul 26, 2016 at 11:51:57AM +0000, Ananyev, Konstantin wrote:
> 
> 
hi Wei,
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Dai
> > Sent: Tuesday, July 26, 2016 10:52 AM
> > To: dev@dpdk.org
> > Cc: Dai, Wei <wei.dai@intel.com>
> > Subject: [dpdk-dev] [PATCH v2 1/2] eal: remove redundant codes to parse --lcores
> > 
> > local variable i is not referred by other codes in the function eal_parse_lcores( ), so it can be removed.
> > 
> > Signed-off-by: Wei Dai <wei.dai@intel.com>
> > ---
> >  lib/librte_eal/common/eal_common_options.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> > index 481c732..c5bf98c 100644
> > --- a/lib/librte_eal/common/eal_common_options.c
> > +++ b/lib/librte_eal/common/eal_common_options.c
> > @@ -578,7 +578,6 @@ eal_parse_lcores(const char *lcores)
> >  	struct rte_config *cfg = rte_eal_get_configuration();
> >  	static uint16_t set[RTE_MAX_LCORE];
> >  	unsigned idx = 0;
> > -	int i;
> >  	unsigned count = 0;
> >  	const char *lcore_start = NULL;
> >  	const char *end = NULL;
> > @@ -593,9 +592,6 @@ eal_parse_lcores(const char *lcores)
> >  	/* Remove all blank characters ahead and after */
> >  	while (isblank(*lcores))
> >  		lcores++;
> > -	i = strlen(lcores);
> > -	while ((i > 0) && isblank(lcores[i - 1]))
> > -		i--;
> 
> I suppose originally it meant to do something  like that:
> while ((i > 0) && isblank(lcores[i - 1]))
> 	lcores[i--] = 0;
totally agreed Konstantin, need to add lcore[i--] = '\0'

> 
> to get rid of blank characters at the end of the line, no?
> Konstantin
> 
> > 
> >  	CPU_ZERO(&cpuset);
> > 
> > --
> > 2.5.5
Adam Bynes

Re: [PATCH v2 1/2] eal: remove redundant codes to parse --lcores

[Dai, Wei]

Hi, Adam & Ananyev
Thanks for your feedback.

> -----Original Message-----
> From: Adam Bynes [mailto:adambynes@outlook.com]
> Sent: Wednesday, July 27, 2016 1:36 AM
> To: Dai, Wei <wei.dai@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal: remove redundant codes to parse
> --lcores
> 
> On Tue, Jul 26, 2016 at 11:51:57AM +0000, Ananyev, Konstantin wrote:
> >
> >
> hi Wei,
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Dai
> > > Sent: Tuesday, July 26, 2016 10:52 AM
> > > To: dev@dpdk.org
> > > Cc: Dai, Wei <wei.dai@intel.com>
> > > Subject: [dpdk-dev] [PATCH v2 1/2] eal: remove redundant codes to parse
> --lcores
> > >
> > > local variable i is not referred by other codes in the function
> eal_parse_lcores( ), so it can be removed.
> > >
> > > Signed-off-by: Wei Dai <wei.dai@intel.com>
> > > ---
> > >  lib/librte_eal/common/eal_common_options.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> > > index 481c732..c5bf98c 100644
> > > --- a/lib/librte_eal/common/eal_common_options.c
> > > +++ b/lib/librte_eal/common/eal_common_options.c
> > > @@ -578,7 +578,6 @@ eal_parse_lcores(const char *lcores)
> > >  	struct rte_config *cfg = rte_eal_get_configuration();
> > >  	static uint16_t set[RTE_MAX_LCORE];
> > >  	unsigned idx = 0;
> > > -	int i;
> > >  	unsigned count = 0;
> > >  	const char *lcore_start = NULL;
> > >  	const char *end = NULL;
> > > @@ -593,9 +592,6 @@ eal_parse_lcores(const char *lcores)
> > >  	/* Remove all blank characters ahead and after */
> > >  	while (isblank(*lcores))
> > >  		lcores++;
> > > -	i = strlen(lcores);
> > > -	while ((i > 0) && isblank(lcores[i - 1]))
> > > -		i--;
> >
> > I suppose originally it meant to do something  like that:
> > while ((i > 0) && isblank(lcores[i - 1]))
> > 	lcores[i--] = 0;
> totally agreed Konstantin, need to add lcore[i--] = '\0'
> 
> >
> > to get rid of blank characters at the end of the line, no?
> > Konstantin

The tail blank is not necessary to be removed from lcores here for following reasons:
1. The tail blanks can also be swallowed later in function eal_parse_set( ) 
  by "while (isblank(*end)) end++". And such operation/sentence also
  deal with blanks in the middle of arguments (for example: blank before
  - and after (7,8) in '0   -2    ,(3   - 6)@(3-6),7@(7-8)     ,8@(8-10)      '), 
  so above removal of blank is redundant.
  By the way, with --lcores '(0-3)@(0-3), (4-5)@(4-5)    ', I also find a new bug.
  After processing cpu set (4-5), the variable end in eal_parse_lcores doesn't point t
  ',' or '\0',  so this function return an error.
  The tail blank after cpu set (4-5) still need to be swallowed. 
  The patch v3 will be removed.eal

2. if let lcores[i--] = 0 here, due to type of input argument lcores (const char *), 
  building will fail. And if the type is changed to char *, the type of input argument of
  several other function also need to be changed. So according to above reason 1, it
  is not need to change the type.

Thanks
Wei Dai 

> >
> > >
> > >  	CPU_ZERO(&cpuset);
> > >
> > > --
> > > 2.5.5
> Adam Bynes

[PATCH v3 1/4] eal: remove redundant codes to parse --lcores

[Wei Dai]

local variable i is not referred by other codes in
the function eal_parse_lcores( ), so it can be removed.

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 481c732..c5bf98c 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -578,7 +578,6 @@ eal_parse_lcores(const char *lcores)
 	struct rte_config *cfg = rte_eal_get_configuration();
 	static uint16_t set[RTE_MAX_LCORE];
 	unsigned idx = 0;
-	int i;
 	unsigned count = 0;
 	const char *lcore_start = NULL;
 	const char *end = NULL;
@@ -593,9 +592,6 @@ eal_parse_lcores(const char *lcores)
 	/* Remove all blank characters ahead and after */
 	while (isblank(*lcores))
 		lcores++;
-	i = strlen(lcores);
-	while ((i > 0) && isblank(lcores[i - 1]))
-		i--;
 
 	CPU_ZERO(&cpuset);
 
-- 
2.5.5

Re: [PATCH v3 1/4] eal: remove redundant codes to parse --lcores

[Ferruh Yigit]

On 7/27/2016 12:22 PM, Wei Dai wrote:
> local variable i is not referred by other codes in
> the function eal_parse_lcores( ), so it can be removed.
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

[PATCH v3 3/4] eal: fix tail blank check in --lcores argument

[Wei Dai]

the tail blank after a group of lcore or cpu set
will make check of its end character fail.
for example: --lcores '(0-3)@(0-3)   ,(4-5)@(4-5)',
the next character after cpu set (0-3) is not ','
or '\0', which fail the check in eal_parse_lcores( ).

Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 217d08b..1a1bab3 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -530,6 +530,13 @@ eal_parse_set(const char *input, uint16_t set[], unsigned num)
 		str = end + 1;
 	} while (*end != '\0' && *end != ')');
 
+	/*
+	 * to avoid failure that tail blank makes end character check fail
+	 * in eal_parse_lcores( )
+	 */
+	while (isblank(*str))
+		str++;
+
 	return str - input;
 }
 
-- 
2.5.5

Re: [PATCH v3 3/4] eal: fix tail blank check in --lcores argument

[Ferruh Yigit]

On 7/27/2016 12:25 PM, Wei Dai wrote:
> the tail blank after a group of lcore or cpu set
> will make check of its end character fail.
> for example: --lcores '(0-3)@(0-3)   ,(4-5)@(4-5)',
> the next character after cpu set (0-3) is not ','
> or '\0', which fail the check in eal_parse_lcores( ).
> 
> Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

[PATCH v2 2/2] eal: fix parsing of eal option --lcores

[Wei Dai]

The '-' in lcore set overrides cpu set of following
lcore set in the argument of EAL option --lcores.
for example --locres '0-2,(3-5)@(3,4),6@(5,6),7@(5-7)',
0-2 make lflags=1 which indeed suppress following
cpu set (3,4), (5,6) and (5-7) after @ .

Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index c5bf98c..217d08b 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -583,7 +583,7 @@ eal_parse_lcores(const char *lcores)
 	const char *end = NULL;
 	int offset;
 	rte_cpuset_t cpuset;
-	int lflags = 0;
+	int lflags;
 	int ret = -1;
 
 	if (lcores == NULL)
@@ -609,6 +609,8 @@ eal_parse_lcores(const char *lcores)
 		if (*lcores == '\0')
 			goto err;
 
+		lflags = 0;
+
 		/* record lcore_set start point */
 		lcore_start = lcores;
 
-- 
2.5.5

[PATCH v3 2/4] eal: fix parsing of eal option --lcores

[Wei Dai]

The '-' in lcore set overrides cpu set of following
lcore set in the argument of EAL option --lcores.
for example --locres '0-2,(3-5)@(3,4),6@(5,6),7@(5-7)',
0-2 make lflags=1 which indeed suppress following
cpu set (3,4), (5,6) and (5-7) after @ .

Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index c5bf98c..217d08b 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -583,7 +583,7 @@ eal_parse_lcores(const char *lcores)
 	const char *end = NULL;
 	int offset;
 	rte_cpuset_t cpuset;
-	int lflags = 0;
+	int lflags;
 	int ret = -1;
 
 	if (lcores == NULL)
@@ -609,6 +609,8 @@ eal_parse_lcores(const char *lcores)
 		if (*lcores == '\0')
 			goto err;
 
+		lflags = 0;
+
 		/* record lcore_set start point */
 		lcore_start = lcores;
 
-- 
2.5.5

Re: [PATCH v3 2/4] eal: fix parsing of eal option --lcores

[Ferruh Yigit]

On 7/27/2016 12:23 PM, Wei Dai wrote:
> The '-' in lcore set overrides cpu set of following
> lcore set in the argument of EAL option --lcores.
> for example --locres '0-2,(3-5)@(3,4),6@(5,6),7@(5-7)',
> 0-2 make lflags=1 which indeed suppress following
> cpu set (3,4), (5,6) and (5-7) after @ .
> 
> Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

[PATCH v3 4/4] eal: fix end character check in --lcores argument

[Wei Dai]

With --lcores 'a-b@c-d', eal_parse_cores() fails because
eal_parse_set() fails due to the next character after
lcore set a-b, which is '@'and not ',' or '\0'.
There is also a right check immediately
after this incorrect check.

Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 1a1bab3..f443a61 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -469,8 +469,6 @@ eal_parse_set(const char *input, uint16_t set[], unsigned num)
 				max = idx;
 				while (isblank(*end))
 					end++;
-				if (*end != ',' && *end != '\0')
-					return -1;
 			}
 
 			if (*end != ',' && *end != '\0' &&
-- 
2.5.5

Re: [PATCH v3 4/4] eal: fix end character check in --lcores argument

[Ferruh Yigit]

On 7/27/2016 12:27 PM, Wei Dai wrote:
> With --lcores 'a-b@c-d', eal_parse_cores() fails because
> eal_parse_set() fails due to the next character after
> lcore set a-b, which is '@'and not ',' or '\0'.
> There is also a right check immediately
> after this incorrect check.
> 
> Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>

I am not sure if a-b@c-d syntax should be supported. (a-b)@(c-d) is
supported and already working.

It looks like a-b is only planned to use without @

and the usage a-b@c-d, like 1-2@4-5 implies to me:
1 -> 4
2 -> 5

but it doesn't work that way, it is always grouped after @, like it has
parenthesis, what it does is:
1 -> 4,5
2 -> 4,5

Even a-b@c is not working, but as I said if @ will be used, using
grouping seems more proper to me, like:
(a-b)@(c-d)
(a-b)@c

So this is mainly about defining the syntax, more than implementing it.
Somebody know more about syntax can comment better.

Regards,
ferruh

Re: [PATCH v3 4/4] eal: fix end character check in --lcores argument

[Thomas Monjalon]

2016-07-28 16:26, Ferruh Yigit:
> On 7/27/2016 12:27 PM, Wei Dai wrote:
> > With --lcores 'a-b@c-d', eal_parse_cores() fails because
> > eal_parse_set() fails due to the next character after
> > lcore set a-b, which is '@'and not ',' or '\0'.
> > There is also a right check immediately
> > after this incorrect check.
> > 
> > Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> > 
> > Signed-off-by: Wei Dai <wei.dai@intel.com>
> 
> I am not sure if a-b@c-d syntax should be supported. (a-b)@(c-d) is
> supported and already working.

Agreed.

Series applied, except this last patch 4, thanks

Re: [PATCH v3 4/4] eal: fix end character check in --lcores argument

[Dai, Wei]

Hi, Thomas, Yigit

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, July 29, 2016 12:06 AM
> To: Dai, Wei <wei.dai@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] eal: fix end character check in --lcores
> argument
> 
> 2016-07-28 16:26, Ferruh Yigit:
> > On 7/27/2016 12:27 PM, Wei Dai wrote:
> > > With --lcores 'a-b@c-d', eal_parse_cores() fails because
> > > eal_parse_set() fails due to the next character after lcore set a-b,
> > > which is '@'and not ',' or '\0'.
> > > There is also a right check immediately after this incorrect check.
> > >
> > > Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> > >
> > > Signed-off-by: Wei Dai <wei.dai@intel.com>
> >
> > I am not sure if a-b@c-d syntax should be supported. (a-b)@(c-d) is
> > supported and already working.
> 
> Agreed.
> 
> Series applied, except this last patch 4, thanks

Sorry, I can't find any document to clarify whether a-b@c-d shoule be supported.
So I design following case to verify all kinds of combination of - and @. 
I tested my patch with --lcores '0-3@12-15, 4-7@(8-11),  (8-11)@4-7, (12-15)@(0-3), 16-19, (20-23) ',
After calling eal_parse_args(argc, argv),  I observed lcore_config[i].cpuset to check cpuset of each lcore.
I added following codes immediately after calling fctret = eal_parse_args(argc, argv); in function rte_eal_init( )
in eal.c to check this patch.

	{
		struct rte_config *p;
		const char *rte_role_str[2] = {"ROLE_RTE", "ROLE_OFF"};
		p = rte_eal_get_configuration();
		for (i=0; i<28; i++) {
			printf("lcore %2d status = %s,  runs on cpuset = 0x%08lX\n", 
					i, rte_role_str[p->lcore_role[i]], lcore_config[i].cpuset.__bits[0]);
		}
	}

The result is as follow:
lcore  0 status = ROLE_RTE,  runs on cpuset = 0x0000F000
lcore  1 status = ROLE_RTE,  runs on cpuset = 0x0000F000
lcore  2 status = ROLE_RTE,  runs on cpuset = 0x0000F000
lcore  3 status = ROLE_RTE,  runs on cpuset = 0x0000F000
lcore  4 status = ROLE_RTE,  runs on cpuset = 0x00000F00
lcore  5 status = ROLE_RTE,  runs on cpuset = 0x00000F00
lcore  6 status = ROLE_RTE,  runs on cpuset = 0x00000F00
lcore  7 status = ROLE_RTE,  runs on cpuset = 0x00000F00
lcore  8 status = ROLE_RTE,  runs on cpuset = 0x000000F0
lcore  9 status = ROLE_RTE,  runs on cpuset = 0x000000F0
lcore 10 status = ROLE_RTE,  runs on cpuset = 0x000000F0
lcore 11 status = ROLE_RTE,  runs on cpuset = 0x000000F0
lcore 12 status = ROLE_RTE,  runs on cpuset = 0x0000000F
lcore 13 status = ROLE_RTE,  runs on cpuset = 0x0000000F
lcore 14 status = ROLE_RTE,  runs on cpuset = 0x0000000F
lcore 15 status = ROLE_RTE,  runs on cpuset = 0x0000000F
lcore 16 status = ROLE_RTE,  runs on cpuset = 0x00010000
lcore 17 status = ROLE_RTE,  runs on cpuset = 0x00020000
lcore 18 status = ROLE_RTE,  runs on cpuset = 0x00040000
lcore 19 status = ROLE_RTE,  runs on cpuset = 0x00080000
lcore 20 status = ROLE_RTE,  runs on cpuset = 0x00F00000
lcore 21 status = ROLE_RTE,  runs on cpuset = 0x00F00000
lcore 22 status = ROLE_RTE,  runs on cpuset = 0x00F00000
lcore 23 status = ROLE_RTE,  runs on cpuset = 0x00F00000
lcore 24 status = ROLE_OFF,  runs on cpuset = 0x00000000
lcore 25 status = ROLE_OFF,  runs on cpuset = 0x00000000
lcore 26 status = ROLE_OFF,  runs on cpuset = 0x00000000
lcore 27 status = ROLE_OFF,  runs on cpuset = 0x00000000

	

Re: [PATCH v3 4/4] eal: fix end character check in --lcores argument

[Thomas Monjalon]

2016-08-02 08:22, Dai, Wei:
> Hi, Thomas, Yigit
> 
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-07-28 16:26, Ferruh Yigit:
> > > On 7/27/2016 12:27 PM, Wei Dai wrote:
> > > > With --lcores 'a-b@c-d', eal_parse_cores() fails because
> > > > eal_parse_set() fails due to the next character after lcore set a-b,
> > > > which is '@'and not ',' or '\0'.
> > > > There is also a right check immediately after this incorrect check.
> > > >
> > > > Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> > > >
> > > > Signed-off-by: Wei Dai <wei.dai@intel.com>
> > >
> > > I am not sure if a-b@c-d syntax should be supported. (a-b)@(c-d) is
> > > supported and already working.
> > 
> > Agreed.
> > 
> > Series applied, except this last patch 4, thanks
> 
> Sorry, I can't find any document to clarify whether a-b@c-d shoule be supported.
> So I design following case to verify all kinds of combination of - and @. 
> I tested my patch with --lcores '0-3@12-15, 4-7@(8-11),  (8-11)@4-7, (12-15)@(0-3), 16-19, (20-23) ',

OK. So do you agree we can discard this patch and forget this syntax?