diff mbox series

[v2] eal: add additional info if lcore exceeds max cores

Message ID 20210915121104.30581-1-david.hunt@intel.com (mailing list archive)
State Superseded
Delegated to: David Marchand
Headers show
Series [v2] eal: add additional info if lcore exceeds max cores | expand

Checks

Context Check Description
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Testing fail Testing issues
ci/Intel-compilation success Compilation OK
ci/iol-x86_64-unit-testing success Testing PASS
ci/github-robot: build fail github build: failed
ci/checkpatch success coding style OK

Commit Message

David Hunt Sept. 15, 2021, 12:11 p.m. UTC
If the user requests to use an lcore above 128 using -l or -c,
the eal will exit with "EAL: invalid core list syntax" and
very little other useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application onto to physical cores.

There is no change in functionalty, just additional messages
suggesting how the --lcores option might be used for the supplied
list of lcores. For example, if "-l 12-14,130,132" is used, we
see the following additional output on the command line:

EAL: Error = One of the 5 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g. --lcores 0@12,1@13,2@14,3@130,4@132

Signed-off-by: David Hunt <david.hunt@intel.com>

---
changes in v2
   * Rather than increasing the default max lcores (as in v1),
     it was agreed to do this instead (switch to --lcores).
   * As the other patches in the v1 of the set are no longer related
     to this change, I'll submit as a separate patch set.
---
 lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

Comments

David Marchand Sept. 16, 2021, 12:34 p.m. UTC | #1
On Wed, Sep 15, 2021 at 2:11 PM David Hunt <david.hunt@intel.com> wrote:
>
> If the user requests to use an lcore above 128 using -l or -c,
> the eal will exit with "EAL: invalid core list syntax" and
> very little other useful information.
>
> This patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application onto to physical cores.
>
> There is no change in functionalty, just additional messages
> suggesting how the --lcores option might be used for the supplied
> list of lcores. For example, if "-l 12-14,130,132" is used, we
> see the following additional output on the command line:
>
> EAL: Error = One of the 5 cores provided exceeds RTE_MAX_LCORE (128)
> EAL: Please use --lcores instead, e.g. --lcores 0@12,1@13,2@14,3@130,4@132
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
>
> ---
> changes in v2
>    * Rather than increasing the default max lcores (as in v1),
>      it was agreed to do this instead (switch to --lcores).
>    * As the other patches in the v1 of the set are no longer related
>      to this change, I'll submit as a separate patch set.

The -c option can use the same kind of warning.


> ---
>  lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
> index ff5861b5f3..5c7a5a45a5 100644
> --- a/lib/eal/common/eal_common_options.c
> +++ b/lib/eal/common/eal_common_options.c
> @@ -836,6 +836,8 @@ eal_parse_service_corelist(const char *corelist)
>         return 0;
>  }
>
> +#define MAX_LCORES_STRING 512
> +
>  static int
>  eal_parse_corelist(const char *corelist, int *cores)
>  {
> @@ -843,6 +845,9 @@ eal_parse_corelist(const char *corelist, int *cores)
>         char *end = NULL;
>         int min, max;
>         int idx;
> +       bool overflow = false;
> +       char lcores[MAX_LCORES_STRING] = "";

This code is not performance sensitive.
In the worst case, like for RTE_MAX_LCORES lcores, it gives this:
0@0,1@1,2@2,3@3,4@4,5@5,6@6,7@7,8@8,9@9,10@10,11@11,12@12,13@13,14@14,15@15,16@16,17@17,18@18,19@19,20@20,21@21,22@22,23@23,24@24,25@25,26@26,27@27,28@28,29@29,30@30,31@31,32@32,33@33,34@34,35@35,36@36,37@37,38@38,39@39,40@40,41@41,42@42,43@43,44@44,45@45,46@46,47@47,48@48,49@49,50@50,51@51,52@52,53@53,54@54,55@55,56@56,57@57,58@58,59@59,60@60,61@61,62@62,63@63,64@64,65@65,66@66,67@67,68@68,69@69,70@70,71@71,72@72,73@73,74@74,75@75,76@76,77@77,78@78,79@79,80@80,81@81,82@82,83@83,84@84,85@85,86@86,87@87,88@88,89@89,90@90,91@91,92@92,93@93,94@94,95@95,96@96,97@97,98@98,99@99,100@100,101@101,102@102,103@103,104@104,105@105,106@106,107@107,108@108,109@109,110@110,111@111,112@112,113@113,114@114,115@115,116@116,117@117,118@118,119@119,120@120,121@121,122@122,123@123,124@124,125@125,126@126,127@127,

Which is 800+ bytes long, let's switch do dynamic allocations.



> +       int len = 0;
>
>         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>                 cores[idx] = -1;
> @@ -862,8 +867,10 @@ eal_parse_corelist(const char *corelist, int *cores)
>                 idx = strtol(corelist, &end, 10);
>                 if (errno || end == NULL)
>                         return -1;
> -               if (idx < 0 || idx >= RTE_MAX_LCORE)
> +               if (idx < 0)
>                         return -1;
> +               if (idx >= RTE_MAX_LCORE)
> +                       overflow = true;

The code before was intermixing parsing and validation of values.
This intermix was not that great.
Let's separate those concerns.


>                 while (isblank(*end))
>                         end++;
>                 if (*end == '-') {
> @@ -873,10 +880,19 @@ eal_parse_corelist(const char *corelist, int *cores)
>                         if (min == RTE_MAX_LCORE)
>                                 min = idx;
>                         for (idx = min; idx <= max; idx++) {
> -                               if (cores[idx] == -1) {
> -                                       cores[idx] = count;
> -                                       count++;
> +                               if (idx < RTE_MAX_LCORE) {
> +                                       if (cores[idx] == -1)
> +                                               cores[idx] = count;
>                                 }
> +                               count++;
> +                               if (count == 1)
> +                                       len = len + snprintf(&lcores[len],
> +                                                       MAX_LCORES_STRING - len,
> +                                                       "%d@%d", count-1, idx);
> +                               else
> +                                       len = len + snprintf(&lcores[len],
> +                                                       MAX_LCORES_STRING - len,
> +                                                       ",%d@%d", count-1, idx);

Always appending a , is easier to read, then after the loop, you just
need to trim the last ,.


>                         }
>                         min = RTE_MAX_LCORE;
>                 } else
> @@ -886,6 +902,13 @@ eal_parse_corelist(const char *corelist, int *cores)
>
>         if (count == 0)
>                 return -1;
> +       if (overflow) {
> +               RTE_LOG(ERR, EAL, "Error = One of the %d cores provided exceeds RTE_MAX_LCORE (%d)\n",
> +                               count, RTE_MAX_LCORE);
> +               RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
> +                               lcores);
> +               return -1;
> +       }
>         return 0;


I'd rework both -c and -l parsing to fill a common data structure,
then validate and generate the suggestion in common helpers.

Something like: https://github.com/david-marchand/dpdk/commit/lcores
This probably needs some time to look at to enhance style and
carefully check for mem leaks.
Tested with max_lcores = 4 (for my 8 cores laptop):

$ for opt in "-c 0x" "-c 0x0" "-c 0x1" "-c 0xf" "-c 0x10" "-c 0x1f"
"-c 0x11" "-c 0x30" "-l 0" "-l 0-3" "-l 0-3,2" "-l 4" "-l 0-4" "-l
0,4" "-l 4,5"
do
  echo $opt
  echo quit | build/app/dpdk-testpmd $opt --log-level=lib.eal:debug
--no-huge -m 20 -a 0:0.0 -- --total-num-mbufs=2048 -ia |&
    grep -E '(ready|RTE_MAX_LCORE|Please use|No lcore|Too many)'
  echo
done

-c 0x
EAL: No lcore in coremask: 0x

-c 0x0
EAL: No lcore in coremask: 0x0

-c 0x1
EAL: Main lcore 0 is ready (tid=7f03956d1c00;cpuset=[0])

-c 0xf
EAL: Main lcore 0 is ready (tid=7fe464461c00;cpuset=[0])
EAL: lcore 1 is ready (tid=7fe45f924700;cpuset=[1])
EAL: lcore 2 is ready (tid=7fe45f123700;cpuset=[2])
EAL: lcore 3 is ready (tid=7fe45e922700;cpuset=[3])

-c 0x10
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4

-c 0x1f
EAL: Too many lcores in coremask: 0x1f

-c 0x11
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@0,1@4

-c 0x30
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: lcore 5 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4,1@5

-l 0
EAL: Main lcore 0 is ready (tid=7f833b17ac00;cpuset=[0])

-l 0-3
EAL: Main lcore 0 is ready (tid=7f9ff5216c00;cpuset=[0])
EAL: lcore 2 is ready (tid=7f9fefed8700;cpuset=[2])
EAL: lcore 3 is ready (tid=7f9fef6d7700;cpuset=[3])
EAL: lcore 1 is ready (tid=7f9ff06d9700;cpuset=[1])

-l 0-3,2
EAL: Main lcore 0 is ready (tid=7f106b937c00;cpuset=[0])
EAL: lcore 1 is ready (tid=7f1066dfa700;cpuset=[1])
EAL: lcore 2 is ready (tid=7f10665f9700;cpuset=[2])
EAL: lcore 3 is ready (tid=7f1065df8700;cpuset=[3])

-l 4
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4

-l 0-4
EAL: Too many lcores in core list: 0-4

-l 0,4
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@0,1@4

-l 4,5
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: lcore 5 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4,1@5
David Hunt Sept. 20, 2021, 9:30 a.m. UTC | #2
Hi David,

On 16/9/2021 1:34 PM, David Marchand wrote:
> On Wed, Sep 15, 2021 at 2:11 PM David Hunt <david.hunt@intel.com> wrote:
>> If the user requests to use an lcore above 128 using -l or -c,
>> the eal will exit with "EAL: invalid core list syntax" and
>> very little other useful information.
>>
>> This patch adds some extra information suggesting to use --lcores
>> so that physical cores above RTE_MAX_LCORE (default 128) can be
>> used. This is achieved by using the --lcores option by mapping
>> the logical cores in the application onto to physical cores.
>>
>> There is no change in functionalty, just additional messages
>> suggesting how the --lcores option might be used for the supplied
>> list of lcores. For example, if "-l 12-14,130,132" is used, we
>> see the following additional output on the command line:
>>
>> EAL: Error = One of the 5 cores provided exceeds RTE_MAX_LCORE (128)
>> EAL: Please use --lcores instead, e.g. --lcores 0@12,1@13,2@14,3@130,4@132
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>
>> ---
>> changes in v2
>>     * Rather than increasing the default max lcores (as in v1),
>>       it was agreed to do this instead (switch to --lcores).
>>     * As the other patches in the v1 of the set are no longer related
>>       to this change, I'll submit as a separate patch set.
> The -c option can use the same kind of warning.


Agreed, I'll include in the next version.


>
>> ---
>>   lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
>> index ff5861b5f3..5c7a5a45a5 100644
>> --- a/lib/eal/common/eal_common_options.c
>> +++ b/lib/eal/common/eal_common_options.c
>> @@ -836,6 +836,8 @@ eal_parse_service_corelist(const char *corelist)
>>          return 0;
>>   }
>>
>> +#define MAX_LCORES_STRING 512
>> +
>>   static int
>>   eal_parse_corelist(const char *corelist, int *cores)
>>   {
>> @@ -843,6 +845,9 @@ eal_parse_corelist(const char *corelist, int *cores)
>>          char *end = NULL;
>>          int min, max;
>>          int idx;
>> +       bool overflow = false;
>> +       char lcores[MAX_LCORES_STRING] = "";
> This code is not performance sensitive.
> In the worst case, like for RTE_MAX_LCORES lcores, it gives this:
> 0@0,1@1,2@2,3@3,4@4,5@5,6@6,7@7,8@8,9@9,10@10,11@11,12@12,13@13,14@14,15@15,16@16,17@17,18@18,19@19,20@20,21@21,22@22,23@23,24@24,25@25,26@26,27@27,28@28,29@29,30@30,31@31,32@32,33@33,34@34,35@35,36@36,37@37,38@38,39@39,40@40,41@41,42@42,43@43,44@44,45@45,46@46,47@47,48@48,49@49,50@50,51@51,52@52,53@53,54@54,55@55,56@56,57@57,58@58,59@59,60@60,61@61,62@62,63@63,64@64,65@65,66@66,67@67,68@68,69@69,70@70,71@71,72@72,73@73,74@74,75@75,76@76,77@77,78@78,79@79,80@80,81@81,82@82,83@83,84@84,85@85,86@86,87@87,88@88,89@89,90@90,91@91,92@92,93@93,94@94,95@95,96@96,97@97,98@98,99@99,100@100,101@101,102@102,103@103,104@104,105@105,106@106,107@107,108@108,109@109,110@110,111@111,112@112,113@113,114@114,115@115,116@116,117@117,118@118,119@119,120@120,121@121,122@122,123@123,124@124,125@125,126@126,127@127,
>
> Which is 800+ bytes long, let's switch do dynamic allocations.
>

Good point. I'll allocate a dozen bytes or so for each physical core 
detected, that should be enough.


>
>> +       int len = 0;
>>
>>          for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>>                  cores[idx] = -1;
>> @@ -862,8 +867,10 @@ eal_parse_corelist(const char *corelist, int *cores)
>>                  idx = strtol(corelist, &end, 10);
>>                  if (errno || end == NULL)
>>                          return -1;
>> -               if (idx < 0 || idx >= RTE_MAX_LCORE)
>> +               if (idx < 0)
>>                          return -1;
>> +               if (idx >= RTE_MAX_LCORE)
>> +                       overflow = true;
> The code before was intermixing parsing and validation of values.
> This intermix was not that great.
> Let's separate those concerns.

I see what you mean (in your comments below). Agreed this would be a 
good idea.


>
>>                  while (isblank(*end))
>>                          end++;
>>                  if (*end == '-') {
>> @@ -873,10 +880,19 @@ eal_parse_corelist(const char *corelist, int *cores)
>>                          if (min == RTE_MAX_LCORE)
>>                                  min = idx;
>>                          for (idx = min; idx <= max; idx++) {
>> -                               if (cores[idx] == -1) {
>> -                                       cores[idx] = count;
>> -                                       count++;
>> +                               if (idx < RTE_MAX_LCORE) {
>> +                                       if (cores[idx] == -1)
>> +                                               cores[idx] = count;
>>                                  }
>> +                               count++;
>> +                               if (count == 1)
>> +                                       len = len + snprintf(&lcores[len],
>> +                                                       MAX_LCORES_STRING - len,
>> +                                                       "%d@%d", count-1, idx);
>> +                               else
>> +                                       len = len + snprintf(&lcores[len],
>> +                                                       MAX_LCORES_STRING - len,
>> +                                                       ",%d@%d", count-1, idx);
> Always appending a , is easier to read, then after the loop, you just
> need to trim the last ,.

Sure.


>
>>                          }
>>                          min = RTE_MAX_LCORE;
>>                  } else
>> @@ -886,6 +902,13 @@ eal_parse_corelist(const char *corelist, int *cores)
>>
>>          if (count == 0)
>>                  return -1;
>> +       if (overflow) {
>> +               RTE_LOG(ERR, EAL, "Error = One of the %d cores provided exceeds RTE_MAX_LCORE (%d)\n",
>> +                               count, RTE_MAX_LCORE);
>> +               RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
>> +                               lcores);
>> +               return -1;
>> +       }
>>          return 0;
>
> I'd rework both -c and -l parsing to fill a common data structure,
> then validate and generate the suggestion in common helpers.


OK, I'll take a look.



> Something like: https://github.com/david-marchand/dpdk/commit/lcores
> This probably needs some time to look at to enhance style and
> carefully check for mem leaks.
> Tested with max_lcores = 4 (for my 8 cores laptop):
>
> $ for opt in "-c 0x" "-c 0x0" "-c 0x1" "-c 0xf" "-c 0x10" "-c 0x1f"
> "-c 0x11" "-c 0x30" "-l 0" "-l 0-3" "-l 0-3,2" "-l 4" "-l 0-4" "-l
> 0,4" "-l 4,5"
> do
>    echo $opt
>    echo quit | build/app/dpdk-testpmd $opt --log-level=lib.eal:debug
> --no-huge -m 20 -a 0:0.0 -- --total-num-mbufs=2048 -ia |&
>      grep -E '(ready|RTE_MAX_LCORE|Please use|No lcore|Too many)'
>    echo
> done
>
> -c 0x
> EAL: No lcore in coremask: 0x
>
> -c 0x0
> EAL: No lcore in coremask: 0x0
>
> -c 0x1
> EAL: Main lcore 0 is ready (tid=7f03956d1c00;cpuset=[0])
>
> -c 0xf
> EAL: Main lcore 0 is ready (tid=7fe464461c00;cpuset=[0])
> EAL: lcore 1 is ready (tid=7fe45f924700;cpuset=[1])
> EAL: lcore 2 is ready (tid=7fe45f123700;cpuset=[2])
> EAL: lcore 3 is ready (tid=7fe45e922700;cpuset=[3])
>
> -c 0x10
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4
>
> -c 0x1f
> EAL: Too many lcores in coremask: 0x1f
>
> -c 0x11
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@0,1@4
>
> -c 0x30
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: lcore 5 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4,1@5
>
> -l 0
> EAL: Main lcore 0 is ready (tid=7f833b17ac00;cpuset=[0])
>
> -l 0-3
> EAL: Main lcore 0 is ready (tid=7f9ff5216c00;cpuset=[0])
> EAL: lcore 2 is ready (tid=7f9fefed8700;cpuset=[2])
> EAL: lcore 3 is ready (tid=7f9fef6d7700;cpuset=[3])
> EAL: lcore 1 is ready (tid=7f9ff06d9700;cpuset=[1])
>
> -l 0-3,2
> EAL: Main lcore 0 is ready (tid=7f106b937c00;cpuset=[0])
> EAL: lcore 1 is ready (tid=7f1066dfa700;cpuset=[1])
> EAL: lcore 2 is ready (tid=7f10665f9700;cpuset=[2])
> EAL: lcore 3 is ready (tid=7f1065df8700;cpuset=[3])
>
> -l 4
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4
>
> -l 0-4
> EAL: Too many lcores in core list: 0-4
>
> -l 0,4
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@0,1@4
>
> -l 4,5
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: lcore 5 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4,1@5
>
>
diff mbox series

Patch

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index ff5861b5f3..5c7a5a45a5 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -836,6 +836,8 @@  eal_parse_service_corelist(const char *corelist)
 	return 0;
 }
 
+#define MAX_LCORES_STRING 512
+
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
@@ -843,6 +845,9 @@  eal_parse_corelist(const char *corelist, int *cores)
 	char *end = NULL;
 	int min, max;
 	int idx;
+	bool overflow = false;
+	char lcores[MAX_LCORES_STRING] = "";
+	int len = 0;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
@@ -862,8 +867,10 @@  eal_parse_corelist(const char *corelist, int *cores)
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
 			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
+		if (idx < 0)
 			return -1;
+		if (idx >= RTE_MAX_LCORE)
+			overflow = true;
 		while (isblank(*end))
 			end++;
 		if (*end == '-') {
@@ -873,10 +880,19 @@  eal_parse_corelist(const char *corelist, int *cores)
 			if (min == RTE_MAX_LCORE)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				if (idx < RTE_MAX_LCORE) {
+					if (cores[idx] == -1)
+						cores[idx] = count;
 				}
+				count++;
+				if (count == 1)
+					len = len + snprintf(&lcores[len],
+							MAX_LCORES_STRING - len,
+							"%d@%d", count-1, idx);
+				else
+					len = len + snprintf(&lcores[len],
+							MAX_LCORES_STRING - len,
+							",%d@%d", count-1, idx);
 			}
 			min = RTE_MAX_LCORE;
 		} else
@@ -886,6 +902,13 @@  eal_parse_corelist(const char *corelist, int *cores)
 
 	if (count == 0)
 		return -1;
+	if (overflow) {
+		RTE_LOG(ERR, EAL, "Error = One of the %d cores provided exceeds RTE_MAX_LCORE (%d)\n",
+				count, RTE_MAX_LCORE);
+		RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
+				lcores);
+		return -1;
+	}
 	return 0;
 }