build: Increase the default value of RTE_MAX_LCORE

dpdk-dev In review David Hunt 5 participants 46 messages Sep 9, 2021 – Nov 17, 2021 Arm/Amlogic Meson SoC support

Share & tools

David Hunt

author

2021-09-09 13:45 UTC #0 lore raw

This patch set increases the default value of RTE_MAX_LCORE from 128 to 512, and reduces the memory footprint of several files in the power library, as the max lcore change would bloat the amount of static memory they use.

David Hunt

author

2021-09-09 13:45 UTC #1 lore raw

PATCH 1/6 build: increase default of max lcores to 512

Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.

This patch increases max_lcores default from 128 to 512.

Signed-off-by David Hunt <[email protected]>

---
 meson_options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

meson_options.txt +1 -1

index 0e92734c49..0ae03677f1 100644
@@ -34,7 +34,7 @@ option('machine', type: 'string', value: 'auto', description:
        'Alias of cpu_instruction_set.')
 option('max_ethports', type: 'integer', value: 32, description:
        'maximum number of Ethernet devices')
-option('max_lcores', type: 'integer', value: 128, description:
+option('max_lcores', type: 'integer', value: 512, description:
        'maximum number of cores/threads supported by EAL')
 option('max_numa_nodes', type: 'integer', value: 32, description:
        'maximum number of NUMA nodes supported by EAL')

Signature · 1 line

2.17.1

Bruce Richardson

2021-09-09 14:37 UTC #2 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 8 lines · David Hunt

Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>
---

Thanks Dave, I think this is needed for future-proofing. Question is, though, is 512 enough, or should we push it higher to 768 or 1k even?

David Marchand

2021-09-10 06:51 UTC #3 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 9 lines · Bruce Richardson

Quoted 7 lines · David Hunt
Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>

Why should we need this?

--lcores makes it possible to pin 128 lcores to any physical core on your system.
And for applications that have their own thread management, they can pin thread, then use rte_thread_register.

Do you have applications that require more than 128 lcores?

Signature · 1 line

David Marchand

Bruce Richardson

2021-09-10 07:54 UTC #4 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 12 lines · David Marchand

Quoted 9 lines · Bruce Richardson
Quoted 7 lines · David Hunt
Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>

Quoted 9 lines

Why should we need this?
--lcores makes it possible to pin 128 lcores to any physical core on your system.
And for applications that have their own thread management, they can pin thread, then use rte_thread_register.
Do you have applications that require more than 128 lcores?

The trouble is that using the --lcores syntax for mapping high core numbers to low lcore ids is much more awkward to use. Every case of DPDK use I've seen uses -c with a coremask, or -l with just giving a few core numbers on it. This simple scheme won't work with core numbers greater than 128, and there are already systems available with more than that number of cores.

Apart from the memory footprint issues - which this patch is already making a good start in addressing, why would we not increase the default max_lcores to that seen on real systems?

/Bruce

David Marchand

2021-09-10 08:06 UTC #5 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 29 lines · Bruce Richardson

Quoted 12 lines · David Marchand
Quoted 9 lines · Bruce Richardson
Quoted 7 lines · David Hunt
Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>
Quoted 9 lines
Why should we need this?
--lcores makes it possible to pin 128 lcores to any physical core on your system.
And for applications that have their own thread management, they can pin thread, then use rte_thread_register.
Do you have applications that require more than 128 lcores?
The trouble is that using the --lcores syntax for mapping high core numbers to low lcore ids is much more awkward to use. Every case of DPDK use I've seen uses -c with a coremask, or -l with just giving a few core numbers on it. This simple scheme won't work with core numbers greater than 128, and there are already systems available with more than that number of cores.

Quoted 3 lines

Apart from the memory footprint issues - which this patch is already making a good start in addressing, why would we not increase the default max_lcores to that seen on real systems?

The memory footprint is a major issue to me, and reserving all those lcores won't be needed in any system.
We will also have to decide on a "640k ought to be enough" value to avoid ABI issue with the next processor that comes out and has more than 512 cores.

Could we wire the -c / -l options to --lcores behavior ? It breaks the 1:1 lcore/physical core assumption, but it solves your usability issue.

Signature · 1 line

David Marchand

Thomas Monjalon

2021-09-10 08:25 UTC #6 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

10/09/2021 10:06, David Marchand:

Quoted 41 lines

Quoted 29 lines · Bruce Richardson
Quoted 12 lines · David Marchand
Quoted 9 lines · Bruce Richardson
Quoted 7 lines · David Hunt
Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>
Quoted 9 lines
Why should we need this?
--lcores makes it possible to pin 128 lcores to any physical core on your system.
And for applications that have their own thread management, they can pin thread, then use rte_thread_register.
Do you have applications that require more than 128 lcores?
The trouble is that using the --lcores syntax for mapping high core numbers to low lcore ids is much more awkward to use. Every case of DPDK use I've seen uses -c with a coremask, or -l with just giving a few core numbers on it. This simple scheme won't work with core numbers greater than 128, and there are already systems available with more than that number of cores.
Quoted 3 lines
Apart from the memory footprint issues - which this patch is already making a good start in addressing, why would we not increase the default max_lcores to that seen on real systems?
The memory footprint is a major issue to me, and reserving all those lcores won't be needed in any system.
We will also have to decide on a "640k ought to be enough" value to avoid ABI issue with the next processor that comes out and has more than 512 cores.

Quoted 3 lines

Could we wire the -c / -l options to --lcores behavior ? It breaks the 1:1 lcore/physical core assumption, but it solves your usability issue.

Why would we change existing options while we already have an option (--lcores) which solves the issue above?
I think the only issue is to educate users.
Is there something to improve in the documentation?

David Hunt

author

2021-09-14 09:34 UTC #7 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 38 lines · Thomas Monjalon

10/09/2021 10:06, David Marchand:
Quoted 37 lines
Quoted 26 lines · Bruce Richardson
Quoted 10 lines · David Marchand
Quoted 8 lines · Bruce Richardson
Quoted 7 lines · David Hunt
Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>
Quoted 9 lines
Why should we need this?
--lcores makes it possible to pin 128 lcores to any physical core on your system.
And for applications that have their own thread management, they can pin thread, then use rte_thread_register.
Do you have applications that require more than 128 lcores?
The trouble is that using the --lcores syntax for mapping high core numbers to low lcore ids is much more awkward to use. Every case of DPDK use I've seen uses -c with a coremask, or -l with just giving a few core numbers on it. This simple scheme won't work with core numbers greater than 128, and there are already systems available with more than that number of cores.
Quoted 3 lines
Apart from the memory footprint issues - which this patch is already making a good start in addressing, why would we not increase the default max_lcores to that seen on real systems?
The memory footprint is a major issue to me, and reserving all those lcores won't be needed in any system.
We will also have to decide on a "640k ought to be enough" value to avoid ABI issue with the next processor that comes out and has more than 512 cores.

Quoted 8 lines

Quoted 3 lines
Could we wire the -c / -l options to --lcores behavior ? It breaks the 1:1 lcore/physical core assumption, but it solves your usability issue.
Why would we change existing options while we already have an option (--lcores) which solves the issue above?
I think the only issue is to educate users.
Is there something to improve in the documentation?

Hi all,
I agree that it’s a good idea to switch to using the “--lcrores” option for cores above the default, that’s already future proofed.
However, I’m still a little concerned about usability, if our users are accustomed to the “-c” and “-l” options, I suggest that we add a warning to suggest using the “--lcores” option if any of the cores provided on the command line are above RTE_MAX_LCORE. That would help them with the solution to using physical cores above 128 (or whatever the compiled default is).

Example:

“ERROR: logical core 212 is above the maximum lcore number permitted.
Please use the --lcores option to map lcores onto physical cores, e.g. --lcores="(0-3)@(212-215).”

I’ll replace the first patch in the set with a patch that adds the additional information in the error message.

Thanks,
Dave.

David Marchand

2021-09-14 10:00 UTC #8 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 44 lines · David Hunt

Quoted 41 lines · Thomas Monjalon
10/09/2021 10:06, David Marchand:
Quoted 40 lines
Quoted 26 lines · Bruce Richardson
Quoted 10 lines · David Marchand
Quoted 8 lines · Bruce Richardson
Quoted 7 lines · David Hunt
Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>
Quoted 9 lines
Why should we need this?
--lcores makes it possible to pin 128 lcores to any physical core on your system.
And for applications that have their own thread management, they can pin thread, then use rte_thread_register.
Do you have applications that require more than 128 lcores?
The trouble is that using the --lcores syntax for mapping high core numbers to low lcore ids is much more awkward to use. Every case of DPDK use I've seen uses -c with a coremask, or -l with just giving a few core numbers on it. This simple scheme won't work with core numbers greater than 128, and there are already systems available with more than that number of cores.
Quoted 3 lines
Apart from the memory footprint issues - which this patch is already making a good start in addressing, why would we not increase the default max_lcores to that seen on real systems?
The memory footprint is a major issue to me, and reserving all those lcores won't be needed in any system.
We will also have to decide on a "640k ought to be enough" value to avoid ABI issue with the next processor that comes out and has more than 512 cores.
Could we wire the -c / -l options to --lcores behavior ? It breaks the 1:1 lcore/physical core assumption, but it solves your usability issue.

Quoted 8 lines

Quoted 5 lines
Why would we change existing options while we already have an option (--lcores) which solves the issue above?
I think the only issue is to educate users.
Is there something to improve in the documentation?
Hi all,
I agree that it’s a good idea to switch to using the “--lcrores” option

Let's avoid typo in the error message you'll add :-).

Quoted 8 lines

for cores above the default, that’s already future proofed. However, I’m still a little concerned about usability, if our users are accustomed to the “-c” and “-l” options, I suggest that we add a warning to suggest using the “--lcores” option if any of the cores provided on the command line are above RTE_MAX_LCORE. That would help them with the solution to using physical cores above 128 (or whatever the compiled default is).

Quoted 5 lines

Example:
“ERROR: logical core 212 is above the maximum lcore number permitted. Please use the --lcores option to map lcores onto physical cores, e.g. --lcores="(0-3)@(212-215).”

If you could directly provide the right --lcores syntax based on what user provided with -c or -l, it would be even better.
This should be not that difficult.

Quoted 3 lines

I’ll replace the first patch in the set with a patch that adds the additional information in the error message.

Signature · 1 line

David Marchand

David Hunt

author

2021-09-14 11:07 UTC #9 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 62 lines · David Marchand

Quoted 43 lines · David Hunt
Quoted 41 lines · Thomas Monjalon
10/09/2021 10:06, David Marchand:
Quoted 40 lines
Quoted 26 lines · Bruce Richardson
Quoted 10 lines · David Marchand
Quoted 8 lines · Bruce Richardson
Quoted 7 lines · David Hunt
Modern processors are coming with an ever increasing number of cores, and 128 does not seem like a sensible max limit any more, especially when you consider multi-socket systems with Hyper-Threading enabled.
This patch increases max_lcores default from 128 to 512.
Signed-off-by David Hunt <[email protected]>
Quoted 9 lines
Why should we need this?
--lcores makes it possible to pin 128 lcores to any physical core on your system.
And for applications that have their own thread management, they can pin thread, then use rte_thread_register.
Do you have applications that require more than 128 lcores?
The trouble is that using the --lcores syntax for mapping high core numbers to low lcore ids is much more awkward to use. Every case of DPDK use I've seen uses -c with a coremask, or -l with just giving a few core numbers on it. This simple scheme won't work with core numbers greater than 128, and there are already systems available with more than that number of cores.
Quoted 3 lines
Apart from the memory footprint issues - which this patch is already making a good start in addressing, why would we not increase the default max_lcores to that seen on real systems?
The memory footprint is a major issue to me, and reserving all those lcores won't be needed in any system.
We will also have to decide on a "640k ought to be enough" value to avoid ABI issue with the next processor that comes out and has more than 512 cores.
Could we wire the -c / -l options to --lcores behavior ? It breaks the 1:1 lcore/physical core assumption, but it solves your usability issue.
Quoted 7 lines
Quoted 5 lines
Why would we change existing options while we already have an option (--lcores) which solves the issue above?
I think the only issue is to educate users.
Is there something to improve in the documentation?
Hi all,
I agree that it’s a good idea to switch to using the “--lcrores” option
Let's avoid typo in the error message you'll add :-).
Quoted 8 lines
for cores above the default, that’s already future proofed. However, I’m still a little concerned about usability, if our users are accustomed to the “-c” and “-l” options, I suggest that we add a warning to suggest using the “--lcores” option if any of the cores provided on the command line are above RTE_MAX_LCORE. That would help them with the solution to using physical cores above 128 (or whatever the compiled default is).

Quoted 8 lines

Quoted 5 lines
Example:
“ERROR: logical core 212 is above the maximum lcore number permitted. Please use the --lcores option to map lcores onto physical cores, e.g. --lcores="(0-3)@(212-215).”
If you could directly provide the right --lcores syntax based on what user provided with -c or -l, it would be even better.
This should be not that difficult.

Agreed. I now have something working that when given "-l 12-16,130,132", will output the following:

EAL: One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g. --lcores "(0-6)@(12-16,130,132)"

So you can just cut-and-paste that option into your command line. Makes it very easy for users to migrate.

Quoted 5 lines

Quoted 3 lines
I’ll replace the first patch in the set with a patch that adds the additional information in the error message.

David Marchand

2021-09-14 11:30 UTC #10 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 8 lines · David Hunt

Quoted 6 lines
Quoted 3 lines
“ERROR: logical core 212 is above the maximum lcore number permitted. Please use the --lcores option to map lcores onto physical cores, e.g. --lcores="(0-3)@(212-215).”
If you could directly provide the right --lcores syntax based on what user provided with -c or -l, it would be even better.
This should be not that difficult.

Quoted 5 lines

Agreed. I now have something working that when given "-l 12-16,130,132", will output the following:
EAL: One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g. --lcores "(0-6)@(12-16,130,132)"

That's not equivalent.

(0-6)@(12-16,130,132) means 7 lcores with each lcore running on the same group of physical cores.
-l 12-16,130,132 means 7 lcores running on dedicated physical cores. I would expect 0@12,1@13,2@14,3@15,4@16,5@130,6@132

You can see with debug logs:

$ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
-m 512 --lcores '(0-2)@(0-2)' -- --total-num-mbufs 2048 |& grep
lcore.*is.ready
EAL: Main lcore 0 is ready (tid=7feb9550bc00;cpuset=[0,1,2])
EAL: lcore 1 is ready (tid=7feb909ce700;cpuset=[0,1,2])
EAL: lcore 2 is ready (tid=7feb901cd700;cpuset=[0,1,2])

$ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
-m 512 --lcores 0@0,1@1,2@2 -- --total-num-mbufs 2048 |& grep
lcore.*is.ready
EAL: Main lcore 0 is ready (tid=7fba1cd1ac00;cpuset=[0])
EAL: lcore 2 is ready (tid=7fba179dc700;cpuset=[2])
EAL: lcore 1 is ready (tid=7fba181dd700;cpuset=[1])

Signature · 1 line

David Marchand

David Hunt

author

2021-09-15 12:14 UTC #11 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 30 lines · David Marchand

Quoted 7 lines · David Hunt
Quoted 6 lines
Quoted 3 lines
“ERROR: logical core 212 is above the maximum lcore number permitted. Please use the --lcores option to map lcores onto physical cores, e.g. --lcores="(0-3)@(212-215).”
If you could directly provide the right --lcores syntax based on what user provided with -c or -l, it would be even better.
This should be not that difficult.
Quoted 5 lines
Agreed. I now have something working that when given "-l 12-16,130,132", will output the following:
EAL: One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g. --lcores "(0-6)@(12-16,130,132)"
That's not equivalent.
(0-6)@(12-16,130,132) means 7 lcores with each lcore running on the same group of physical cores.
-l 12-16,130,132 means 7 lcores running on dedicated physical cores. I would expect 0@12,1@13,2@14,3@15,4@16,5@130,6@132
You can see with debug logs:
$ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
-m 512 --lcores '(0-2)@(0-2)' -- --total-num-mbufs 2048 |& grep
lcore.*is.ready
EAL: Main lcore 0 is ready (tid=7feb9550bc00;cpuset=[0,1,2])
EAL: lcore 1 is ready (tid=7feb909ce700;cpuset=[0,1,2])
EAL: lcore 2 is ready (tid=7feb901cd700;cpuset=[0,1,2])

Quoted 9 lines

$ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
-m 512 --lcores 0@0,1@1,2@2 -- --total-num-mbufs 2048 |& grep
lcore.*is.ready
EAL: Main lcore 0 is ready (tid=7fba1cd1ac00;cpuset=[0])
EAL: lcore 2 is ready (tid=7fba179dc700;cpuset=[2])
EAL: lcore 1 is ready (tid=7fba181dd700;cpuset=[1])

Hi David,

Thanks for the clarification. I've made the relevant changes and submitted a v2. Hopefully the suggested parameters are correct this time! :)

Regards,
Dave.

Morten Brørup

2021-11-17 15:55 UTC #12 lore raw

RE: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

In reply to David Hunt

My microphone was not working during the Techboard meeting, so here goes instead:

Please consider if the default of max 128 assigned lcores suffices for the expected lifetime of the 21.11 LTS, or if the default should be bumped to 512 as suggested.

@David Hunt, do you have idea about the memory cost of increasing from 128 to 512? This seems to be the primary objection.

-Morten

David Hunt

author

2021-11-17 19:02 UTC #13 lore raw

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

Quoted 7 lines · Morten Brørup

My microphone was not working during the Techboard meeting, so here goes instead:
Please consider if the default of max 128 assigned lcores suffices for the expected lifetime of the 21.11 LTS, or if the default should be bumped to 512 as suggested.
@David Hunt, do you have idea about the memory cost of increasing from 128 to 512? This seems to be the primary objection.
-Morten

Hi Morten,

I seem to recall that a simple increase from 128 to 512 cores would result in an increased static memory footprint of 2.5MBs per application, but there were additional patches in the opriginal patch set that switch from static allocation to dynamic allocation reducing this to a few hundred kilobytes. This was not deemed the best direction, so we went with the improvement of informing the user more clearly on how to use the command line syntax to use the physical core numbers above 128 as lcores in your application.

I believe the thinking is that 128 lcores is plenty for a single application, and if there is a case where more than that is needed (for a single application), then the default can be over-ridden by changing the max_lcores setting and recompiling.

Rgds,
Dave.

David Hunt

author

2021-09-15 12:11 UTC #14 lore raw

PATCH v2 1/1 eal: add additional info if lcore exceeds max cores

In reply to David Hunt

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 <[email protected]>

---
 lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

lib/eal/common/eal_common_options.c +27 -4

index ff5861b5f3..5c7a5a45a5 100644
@@ -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;
 }

Signature · 1 line

2.17.1

David Marchand

2021-09-16 12:35 UTC #15 lore raw

Re: [dpdk-dev] [PATCH v2] eal: add additional info if lcore exceeds max cores

Quoted 18 lines · David Hunt

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

Quoted 8 lines

Signed-off-by David Hunt <[email protected]>
---
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.

Quoted 18 lines

---
 lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

lib/eal/common/eal_common_options.c +2 -0

index ff5861b5f3..5c7a5a45a5 100644
@@ -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)
 {

lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 849

Quoted 5 lines

lib/eal/common/eal_common_options.c +2 -0

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

lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 873

Quoted 13 lines

lib/eal/common/eal_common_options.c +4 -1

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

Quoted 13 lines

lib/eal/common/eal_common_options.c +3 -3

                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;

lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 895

Quoted 10 lines

lib/eal/common/eal_common_options.c +9 -0

                                }
+                               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 ,.

Quoted 5 lines

lib/eal/common/eal_common_options.c +0 -0

                        }
                        min = RTE_MAX_LCORE;
                } else

@@ -886,6 +902,13 @@ eal_parse_corelist(const char *corelist, int *cores)

lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 912

Quoted 10 lines

lib/eal/common/eal_common_options.c +7 -0

        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

Signature · 1 line

David Marchand

David Hunt

author

2021-09-20 09:31 UTC #16 lore raw

Re: [dpdk-dev] [PATCH v2] eal: add additional info if lcore exceeds max cores

Hi David,

Quoted 20 lines · David Marchand

Quoted 19 lines · David Hunt
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 <[email protected]>

Quoted 7 lines

Quoted 6 lines
---
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.

Quoted 28 lines

Quoted 19 lines
---
  lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
  1 file changed, 27 insertions(+), 4 deletions(-)
lib/eal/common/eal_common_options.c +2 -0
index ff5861b5f3..5c7a5a45a5 100644
@@ -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)
  {
lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 849
Quoted 5 lines
lib/eal/common/eal_common_options.c +2 -0
@@ -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,

Quoted 2 lines

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.

Quoted 17 lines

Quoted 6 lines

lib/eal/common/eal_common_options.c +1 -0

+       int len = 0;
 
         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
                 cores[idx] = -1;

lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 873

Quoted 8 lines

lib/eal/common/eal_common_options.c +3 -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.

Quoted 18 lines

lib/eal/common/eal_common_options.c +6 -3

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

Quoted 8 lines

lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 895

Quoted 6 lines

lib/eal/common/eal_common_options.c +6 -0

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

Quoted 6 lines

lib/eal/common/eal_common_options.c +0 -0

                         }
                         min = RTE_MAX_LCORE;
                 } else

@@ -886,6 +902,13 @@ eal_parse_corelist(const char *corelist, int *cores)

Quoted 13 lines

lib/eal/common/eal_common_options.c eal_parse_corelist(const char *corelist, int *cores) · line 912

Quoted 10 lines

lib/eal/common/eal_common_options.c +7 -0

         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.

Quoted 79 lines

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

author

2021-09-21 11:50 UTC #17 lore raw

PATCH v3 1/2 eal: add additional info if core list too long

In reply to David Hunt

If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else 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 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-16,130,132" is used, we see the following additional output on the command line:

EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g.

     --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132

Signed-off-by David Hunt <[email protected]>

---
 lib/eal/common/eal_common_options.c | 102 ++++++++++++++++++++++++----
 1 file changed, 89 insertions(+), 13 deletions(-)

lib/eal/common/eal_common_options.c +89 ~~-13~~

index ff5861b5f3..6139b2b21e 100644
@@ -709,6 +709,47 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i, j;
+	char *lcorestr;
+	int len = 0;
+	bool overflow = false;
+
+	for (j = 0; j < count; j++) {
+		if (lcores[j] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[j], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (overflow) {
+		/*
+		 * If we've encountered a core that's greater than
+		 * RTE_MAX_LCORE, suggest using --lcores option to
+		 * map lcores onto physical cores greater than
+		 * RTE_MAX_LCORE, then return.
+		 */
+		lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+		if (lcorestr == NULL) {
+			RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+			return -ENOMEM;
+		}
+		for (i = 0; i < count; i++)
+			len = len + snprintf(&lcorestr[len],
+					RTE_MAX_LCORE * 10 - len,
+					"%d@%d,", i, lcores[i]);
+		if (len > 0)
+			lcorestr[len-1] = 0;
+		RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
+				lcorestr);
+		free(lcorestr);
+		return -1;
+	}
+	return 0;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {

@@ -839,54 +880,89 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	char *corelist_copy;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 
+	corelist_copy = calloc(1, strlen(corelist)+1);
+	if (corelist_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate corelist copy\n");
+		return -ENOMEM;
+	}
+
+	strlcpy(corelist_copy, corelist, strlen(corelist)+1);
+
 	/* Remove all blank characters ahead */
 	while (isblank(*corelist))
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;
 		if (*corelist == '\0')
-			return -1;
+			goto err;
 		errno = 0;
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
-			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
-			return -1;
+			goto err;
+		if (idx < 0)
+			goto err;
 		while (isblank(*end))
 			end++;
 		if (*end == '-') {
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				lcores[count] = idx;
+				count++;
+				if (count > RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
-			return -1;
+			goto err;
 		corelist = end + 1;
 	} while (*end != '\0');
 
 	if (count == 0)
-		return -1;
+		goto err;
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (corelist_copy)
+		free(corelist_copy);
+
 	return 0;
+err:
+	if (corelist_copy)
+		free(corelist_copy);
+
+	return -1;
 }
 
 /* Changes the lcore id of the main thread */

Signature · 1 line

2.17.1

David Hunt

author

2021-09-21 11:50 UTC #18 lore raw

PATCH v3 2/2 eal: add additional info if core mask too long

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid core list syntax" and very little else useful information.

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
"-c 0xf00000000000000000000000000000000" is used, we see the following additional output on the command line:

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

Signed-off-by David Hunt <[email protected]>

---
changes in v3
* added this patch to the set. Addresses the changes for the -c option.

---
 lib/eal/common/eal_common_options.c | 67 +++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 12 deletions(-)

lib/eal/common/eal_common_options.c +55 ~~-12~~

index 6139b2b21e..331dca1e38 100644
@@ -753,15 +753,25 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	char *coremask_copy = NULL;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 	idx = 0;
 
+	coremask_copy = calloc(1, strlen(coremask)+1);
+	if (coremask_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate coremask copy\n");
+		return -ENOMEM;
+	}
+
+	strlcpy(coremask_copy, coremask, strlen(coremask)+1);
+
 	/* Remove all blank characters ahead and after .
 	 * Remove 0x/0X if exists.
 	 */

@@ -773,30 +783,63 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
-		return -1;
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
-			return -1;
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
-				count++;
+				lcores[count++] = idx;
+				if (count > RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
+				}
 			}
 		}
 	}
 	for (; i >= 0; i--)
-		if (coremask[i] != '0')
-			return -1;
-	if (count == 0)
-		return -1;
+		if (coremask[i] != '0') {
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
+		}
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (coremask_copy)
+		free(coremask_copy);
+
 	return 0;
+err:
+	if (coremask_copy)
+		free(coremask_copy);
+	return -1;
 }
 
 static int

Signature · 1 line

2.17.1

Bruce Richardson

Acked-by on v3 2/2

2021-09-21 12:00 UTC #19 lore raw

Re: [dpdk-dev] [PATCH v3 2/2] eal: add additional info if core mask too long

Quoted 9 lines · David Hunt

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid core list syntax" and very little else 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 to physical cores.

Quoted 7 lines

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
"-c 0xf00000000000000000000000000000000" is used, we see the following additional output on the command line:
EAL: Error = One of the 4 cores provided exceeds RTE_MAX_LCORE (128)

"=" ?

Quoted 4 lines

EAL: Please use --lcores instead, e.g. --lcores 0@128,1@129,2@130,3@131
Signed-off-by David Hunt <[email protected]>

While I really can't see many people using "-c" to use cores >128 (unless from a script, I suppose), this change is worthwhile having for completeness. I suggest aligning the error message with that for "-l" flag.

Acked-by Bruce Richardson <[email protected]>

Bruce Richardson

Acked-by on v3 1/2

2021-09-21 11:57 UTC #20 lore raw

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

In reply to David Hunt

Quoted 9 lines · David Hunt

If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else 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 to physical cores.

Quoted 7 lines

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-16,130,132" is used, we see the following additional output on the command line:
EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g.

Minor suggestion: it would be good to clarify how to use lcores and what is happening here in the example. Something like: "Please use --lcores instead, to map lower lcore ids onto higher-numbered cores", could help the user understand better what is happening.

Quoted 3 lines

     --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
Signed-off-by David Hunt <[email protected]>

With some more info to help the user:

Acked-by Bruce Richardson <[email protected]>

David Hunt

author

2021-09-21 12:04 UTC #21 lore raw

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

Quoted 15 lines · Bruce Richardson

Quoted 14 lines · David Hunt
If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else 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 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-16,130,132" is used, we see the following additional output on the command line:

Quoted 6 lines

Quoted 2 lines
EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g.
Minor suggestion: it would be good to clarify how to use lcores and what is happening here in the example. Something like: "Please use --lcores instead, to map lower lcore ids onto higher-numbered cores", could help the user understand better what is happening.

Hi Bruce, how about:

EAL: Please use --lcores to map logical cores onto cores > RTE_LCORE_MAX ,e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132

Rgds,
Dave.

Quoted 7 lines

Quoted 4 lines
      --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
Signed-off-by David Hunt <[email protected]>
With some more info to help the user:
Acked-by Bruce Richardson <[email protected]>

David Hunt

author

2021-09-21 13:17 UTC #22 lore raw

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

Quoted 25 lines · David Hunt

Quoted 15 lines · Bruce Richardson
Quoted 14 lines · David Hunt
If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else 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 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-16,130,132" is used, we see the following additional output on the command line:
Quoted 6 lines
Quoted 2 lines
EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g.
Minor suggestion: it would be good to clarify how to use lcores and what is happening here in the example. Something like: "Please use --lcores instead, to map lower lcore ids onto higher-numbered cores", could help the user understand better what is happening.

Quoted 9 lines

Hi Bruce, how about:
EAL: Please use --lcores to map logical cores onto cores > RTE_LCORE_MAX ,e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
Rgds,
Dave.

I think this should do it, as it clarifies the mapping:

EAL: lcore 130 >= RTE_MAX_LCORE (128)
EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_LCORE_MAX, e.g.'--lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132'

Thanks,
Dave.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
--lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
Signed-off-by David Hunt <[email protected]>
With some more info to help the user:
Acked-by Bruce Richardson <[email protected]>

Bruce Richardson

2021-09-21 13:21 UTC #23 lore raw

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

Quoted 33 lines · David Hunt

On 21/9/2021 1:04 PM, David Hunt wrote:
On 21/9/2021 12:57 PM, Bruce Richardson wrote:
On Tue, Sep 21, 2021 at 12:50:14PM +0100, David Hunt wrote:
If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else 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 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-16,130,132" is used, we see the following additional output on the command line:
EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g.
Minor suggestion: it would be good to clarify how to use lcores and what is
happening here in the example. Something like: "Please use --lcores instead, to map lower lcore ids onto higher-numbered cores", could help the
user understand better what is happening.
Hi Bruce, how about:
EAL: Please use --lcores to map logical cores onto cores > RTE_LCORE_MAX ,e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132 Rgds,
Dave.

Quoted 8 lines

I think this should do it, as it clarifies the mapping:

   EAL: lcore 130 >= RTE_MAX_LCORE (128)
   EAL: lcore 132 >= RTE_MAX_LCORE (128)
   EAL: to use high physical core ids , please use --lcores to map them to
   lcore ids below RTE_LCORE_MAX, e.g. '--lcores
   0@12,1@13,2@14,3@15,4@16,5@130,6@132'

Text looks good to me.

Again minor nits: I think the continued lines should be indented, and you should probably wrap immediately after the "e.g." rather than in the middle of the parameter set.

/Bruce

David Marchand

2021-09-21 13:51 UTC #24 lore raw

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

In reply to David Hunt

lib/eal/common/eal_common_options.c eal_parse_service_corelist(const char *corelist) · line 887

Quoted 13 lines · David Hunt

static int
eal_parse_coremask(const char *coremask, int *cores) {

lib/eal/common/eal_common_options.c +2 -1

@@ -839,54 +880,89 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-       unsigned count = 0;
+       unsigned int count = 0, k;
        char *end = NULL;
        int min, max;
        int idx;
+       int lcores[RTE_MAX_LCORE];

Static array...

"-l 0-RTE_MAX_LCORE" / "-c 0x1<enough f to fill RTE_MAX_LCORE>" / "-l 0-(RTE_MAX_LCORE-1),0" crash.

Please set RTE_MAX_LCORE to 4 (or something that is smaller than your system core count) and run the tests I provided in my previous mail.

Signature · 1 line

David Marchand

David Hunt

author

2021-09-21 15:13 UTC #25 lore raw

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

Quoted 16 lines · David Marchand

lib/eal/common/eal_common_options.c eal_parse_service_corelist(const char *corelist) · line 887

Quoted 13 lines · David Hunt

  static int
  eal_parse_coremask(const char *coremask, int *cores)
  {

lib/eal/common/eal_common_options.c +2 -1

@@ -839,54 +880,89 @@ eal_parse_service_corelist(const char *corelist)
  static int
  eal_parse_corelist(const char *corelist, int *cores)
  {
-       unsigned count = 0;
+       unsigned int count = 0, k;
         char *end = NULL;
         int min, max;
         int idx;
+       int lcores[RTE_MAX_LCORE];

Static array...

Quoted 6 lines

"-l 0-RTE_MAX_LCORE" / "-c 0x1<enough f to fill RTE_MAX_LCORE>" / "-l 0-(RTE_MAX_LCORE-1),0" crash.
Please set RTE_MAX_LCORE to 4 (or something that is smaller than your system core count) and run the tests I provided in my previous mail.

Hi David,

I did set the lcore_max to 4, and ran the tests you provided, and they all looked OK.

However, as you have pointed out, there is a problem with the lcores array, which I have now fixed, I'll push up shortly. I'm not expecting to populate with more than RTE_MAX_LCORE elements, but there was an edge case where one extra was being added.

Thanks again,
Dave.

David Hunt

author

2021-09-22 12:29 UTC #26 lore raw

PATCH v4 1/2 eal: add additional info if core list too long

In reply to David Hunt

If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else useful information.

For example, if "-l 12-16,130,132" is used, we see the following additional output on the command line:

EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: lcore 133 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133

Signed-off-by David Hunt <[email protected]>

Acked-by Bruce Richardson <[email protected]>

---
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.
changes in v3
* separated out some of the corelist cheking into separate function
* added extra messages for the different failure conditions.
* changed allocation of the core strings from static to dynamic
* now prints out a message for each core above RTE_MAX_LCORE
changes in v4
* tweaked log messages to be a bit clearer about mapping lcores to physical cores.
* improved indenting of log messages.
* fixed bug in overrunning end of lcore array
* switched from strlcpy to strdup because of a clang error
---

---
 lib/eal/common/eal_common_options.c | 100 ++++++++++++++++++++++++----
 1 file changed, 87 insertions(+), 13 deletions(-)

lib/eal/common/eal_common_options.c +87 ~~-13~~

index eaef57312f..de1717946f 100644
@@ -703,6 +703,47 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i, j;
+	char *lcorestr;
+	int len = 0;
+	bool overflow = false;
+
+	for (j = 0; j < count; j++) {
+		if (lcores[j] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[j], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (overflow) {
+		/*
+		 * If we've encountered a core that's greater than
+		 * RTE_MAX_LCORE, suggest using --lcores option to
+		 * map lcores onto physical cores greater than
+		 * RTE_MAX_LCORE, then return.
+		 */
+		lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+		if (lcorestr == NULL) {
+			RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+			return -ENOMEM;
+		}
+		for (i = 0; i < count; i++)
+			len = len + snprintf(&lcorestr[len],
+					RTE_MAX_LCORE * 10 - len,
+					"%d@%d,", i, lcores[i]);
+		if (len > 0)
+			lcorestr[len-1] = 0;
+		RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
+		RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
+		free(lcorestr);
+		return -1;
+	}
+	return 0;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {

@@ -833,54 +874,87 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	char *corelist_copy;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 
+	corelist_copy = strdup(corelist);
+	if (corelist_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to duplicate corelist\n");
+		return -ENOMEM;
+	}
+
 	/* Remove all blank characters ahead */
 	while (isblank(*corelist))
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;
 		if (*corelist == '\0')
-			return -1;
+			goto err;
 		errno = 0;
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
-			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
-			return -1;
+			goto err;
+		if (idx < 0)
+			goto err;
 		while (isblank(*end))
 			end++;
 		if (*end == '-') {
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				if (count < RTE_MAX_LCORE)
+					lcores[count++] = idx;
+				else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
-			return -1;
+			goto err;
 		corelist = end + 1;
 	} while (*end != '\0');
 
 	if (count == 0)
-		return -1;
+		goto err;
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (corelist_copy)
+		free(corelist_copy);
+
 	return 0;
+err:
+	if (corelist_copy)
+		free(corelist_copy);
+
+	return -1;
 }
 
 /* Changes the lcore id of the main thread */

Signature · 1 line

2.17.1

David Hunt

author

2021-09-22 12:29 UTC #27 lore raw

PATCH v4 2/2 eal: add additional info if core mask too long

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid core list syntax" and very little else useful information.

For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:

EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@128,1@129

Signed-off-by David Hunt <[email protected]>

Acked-by Bruce Richardson <[email protected]>

---
changes in v3
* added this patch to the set. Addresses the changes for the -c option.
changes in v4
* fixed buffer overrun in populating lcore array
* switched from strlcpy to strdup due to a clang error
---

---
 lib/eal/common/eal_common_options.c | 66 +++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 12 deletions(-)

lib/eal/common/eal_common_options.c +54 ~~-12~~

index de1717946f..19d3588f78 100644
@@ -747,15 +747,23 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	char *coremask_copy = NULL;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 	idx = 0;
 
+	coremask_copy = strdup(coremask);
+	if (coremask_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to duplicate coremask\n");
+		return -ENOMEM;
+	}
+
 	/* Remove all blank characters ahead and after .
 	 * Remove 0x/0X if exists.
 	 */

@@ -767,30 +775,64 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
-		return -1;
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
-			return -1;
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
-				count++;
+				if (count < RTE_MAX_LCORE) {
+					lcores[count++] = idx;
+				} else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
+				}
 			}
 		}
 	}
 	for (; i >= 0; i--)
-		if (coremask[i] != '0')
-			return -1;
-	if (count == 0)
-		return -1;
+		if (coremask[i] != '0') {
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
+		}
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (coremask_copy)
+		free(coremask_copy);
+
 	return 0;
+err:
+	if (coremask_copy)
+		free(coremask_copy);
+	return -1;
 }
 
 static int

Signature · 1 line

2.17.1

David Marchand

2021-09-23 08:12 UTC #28 lore raw

Re: [dpdk-dev] [PATCH v4 2/2] eal: add additional info if core mask too long

Quoted 4 lines · David Hunt

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid core list syntax" and very little else useful information.

invalid coremask*

Quoted 34 lines

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 to physical cores.
For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:
EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@128,1@129
Signed-off-by David Hunt <[email protected]>
Acked-by Bruce Richardson <[email protected]>
---
changes in v3
* added this patch to the set. Addresses the changes for the -c option.
changes in v4
* fixed buffer overrun in populating lcore array
* switched from strlcpy to strdup due to a clang error
---
---
 lib/eal/common/eal_common_options.c | 66 +++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 12 deletions(-)
lib/eal/common/eal_common_options.c +0 -0
index de1717946f..19d3588f78 100644

lib/eal/common/eal_common_options.c check_core_list(int *lcores, unsigned int count) · line 750

Quoted 5 lines

lib/eal/common/eal_common_options.c +1 -1

@@ -747,15 +747,23 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-       unsigned count = 0;
+       unsigned int count = 0, k;

Same as patch 1, no need for count.

lib/eal/common/eal_common_options.c check_core_list(int *lcores, unsigned int count) · line 755

Quoted 5 lines

lib/eal/common/eal_common_options.c +2 -0

        int i, j, idx;
        int val;
        char c;
+       int lcores[RTE_MAX_LCORE];
+       char *coremask_copy = NULL;

Contrary to patch 1, coremask_copy is used.
But, coremask is const, we can directly use it in log messages.

So same as patch 1, please remove coremask_copy.

Quoted 39 lines

lib/eal/common/eal_common_options.c +15 -5

        for (idx = 0; idx < RTE_MAX_LCORE; idx++)
                cores[idx] = -1;
        idx = 0;
 
+       coremask_copy = strdup(coremask);
+       if (coremask_copy == NULL) {
+               RTE_LOG(ERR, EAL, "Unable to duplicate coremask\n");
+               return -ENOMEM;
+       }
+
        /* Remove all blank characters ahead and after .
         * Remove 0x/0X if exists.
         */

@@ -767,30 +775,64 @@ eal_parse_coremask(const char *coremask, int *cores)
        i = strlen(coremask);
        while ((i > 0) && isblank(coremask[i - 1]))
                i--;
-       if (i == 0)
-               return -1;
+       if (i == 0) {
+               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+               goto err;
+       }
 
-       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+       for (i = i - 1; i >= 0; i--) {
                c = coremask[i];
                if (isxdigit(c) == 0) {
                        /* invalid characters */
-                       return -1;
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+                                       coremask_copy);
+                       goto err;
                }
                val = xdigit2val(c);
-               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+               for (j = 0; j < BITS_PER_HEX; j++, idx++)
                {

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 801

Quoted 10 lines

lib/eal/common/eal_common_options.c +7 -2

                        if ((1 << j) & val) {
-                               cores[idx] = count;
-                               count++;
+                               if (count < RTE_MAX_LCORE) {
+                                       lcores[count++] = idx;
+                               } else {
+                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+                                                       RTE_MAX_LCORE);
+                                       goto err;
+                               }

You can invert those blocks:

if (count >= RTE_MAX_LCORE) {
RTE_LOG();
goto err;
}

lcores[count] = idx;
count++;

Quoted 44 lines

lib/eal/common/eal_common_options.c +29 -5

                        }
                }
        }
        for (; i >= 0; i--)
-               if (coremask[i] != '0')
-                       return -1;
-       if (count == 0)
-               return -1;
+               if (coremask[i] != '0') {
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+                                       coremask_copy);
+                       goto err;
+               }
+       if (count == 0) {
+               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+               goto err;
+       }
+
+       if (check_core_list(lcores, count))
+               goto err;
+
+       /*
+        * Now that we've gto a list of cores no longer than
+        * RTE_MAX_LCORE, and no lcore in that list is greater
+        * than RTE_MAX_LCORE, populate the cores
+        * array and return.
+        */
+       for (k = 0; k < count; k++)
+               cores[lcores[k]] = k;
+
+       if (coremask_copy)
+               free(coremask_copy);
+
        return 0;
+err:
+       if (coremask_copy)
+               free(coremask_copy);
+       return -1;
 }
 
 static int
--

2.17.1

Signature · 1 line

David Marchand

David Hunt

author

2021-09-23 10:21 UTC #29 lore raw

Re: [dpdk-dev] [PATCH v4 2/2] eal: add additional info if core mask too long

Quoted 11 lines · David Marchand

lib/eal/common/eal_common_options.c check_core_list(int *lcores, unsigned int count) · line 755
Quoted 6 lines · David Hunt
         int i, j, idx;
         int val;
         char c;
lib/eal/common/eal_common_options.c +2 -0
+       int lcores[RTE_MAX_LCORE];
+       char *coremask_copy = NULL;
Contrary to patch 1, coremask_copy is used.
But, coremask is const, we can directly use it in log messages.
So same as patch 1, please remove coremask_copy.

const protects the data being pointed at from being modified, but the pointer itself can still be modified, and the pointer is indeed incremented to cut off leading zeroes and spaces.

For example, if I provide " 0x0" as a parameter,coremask prints as "0", where as coremask_copy prints as " 0x0".

Similarly, if I provide "0x" as a parameter,coremask prints as "", where as coremask_copy prints as " 0x".

Instead of using strdup, I can use "const char *coremask_orig = coremask;", and remove the strdup and the frees...

Quoted 51 lines

Quoted 39 lines

         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
                 cores[idx] = -1;
         idx = 0;

lib/eal/common/eal_common_options.c +15 -5

+       coremask_copy = strdup(coremask);
+       if (coremask_copy == NULL) {
+               RTE_LOG(ERR, EAL, "Unable to duplicate coremask\n");
+               return -ENOMEM;
+       }
+
         /* Remove all blank characters ahead and after .
          * Remove 0x/0X if exists.
          */

@@ -767,30 +775,64 @@ eal_parse_coremask(const char *coremask, int *cores)
         i = strlen(coremask);
         while ((i > 0) && isblank(coremask[i - 1]))
                 i--;
-       if (i == 0)
-               return -1;
+       if (i == 0) {
+               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+               goto err;
+       }
 
-       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+       for (i = i - 1; i >= 0; i--) {
                 c = coremask[i];
                 if (isxdigit(c) == 0) {
                         /* invalid characters */
-                       return -1;
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+                                       coremask_copy);
+                       goto err;
                 }
                 val = xdigit2val(c);
-               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+               for (j = 0; j < BITS_PER_HEX; j++, idx++)
                 {

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 801

Quoted 10 lines

lib/eal/common/eal_common_options.c +7 -2

                         if ((1 << j) & val) {
-                               cores[idx] = count;
-                               count++;
+                               if (count < RTE_MAX_LCORE) {
+                                       lcores[count++] = idx;
+                               } else {
+                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+                                                       RTE_MAX_LCORE);
+                                       goto err;
+                               }

You can invert those blocks:

Quoted 8 lines

if (count >= RTE_MAX_LCORE) {
    RTE_LOG();
    goto err;
}
lcores[count] = idx;
count++;

Quoted 47 lines

Quoted 45 lines

lib/eal/common/eal_common_options.c +29 -5

                         }
                 }
         }
         for (; i >= 0; i--)
-               if (coremask[i] != '0')
-                       return -1;
-       if (count == 0)
-               return -1;
+               if (coremask[i] != '0') {
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+                                       coremask_copy);
+                       goto err;
+               }
+       if (count == 0) {
+               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+               goto err;
+       }
+
+       if (check_core_list(lcores, count))
+               goto err;
+
+       /*
+        * Now that we've gto a list of cores no longer than
+        * RTE_MAX_LCORE, and no lcore in that list is greater
+        * than RTE_MAX_LCORE, populate the cores
+        * array and return.
+        */
+       for (k = 0; k < count; k++)
+               cores[lcores[k]] = k;
+
+       if (coremask_copy)
+               free(coremask_copy);
+
         return 0;
+err:
+       if (coremask_copy)
+               free(coremask_copy);
+       return -1;
  }
 
  static int
--

2.17.1

David Marchand

2021-09-23 08:12 UTC #30 lore raw

Re: [dpdk-dev] [PATCH v4 1/2] eal: add additional info if core list too long

In reply to David Hunt

Quoted 13 lines · David Hunt

If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else 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 to physical cores.
For example, if "-l 12-16,130,132" is used, we see the following additional output on the command line:

Quoted 8 lines

EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: lcore 133 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133
Signed-off-by David Hunt <[email protected]>
Acked-by Bruce Richardson <[email protected]>

This series is there to help users, it should not break existing working configurations.

I mentionned the "-l 0-3,0" case before.
This syntax is debatable, but it worked before (see comment below) and this patch now refuses it.

Quoted 27 lines

---
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.
changes in v3
* separated out some of the corelist cheking into separate function
* added extra messages for the different failure conditions.
* changed allocation of the core strings from static to dynamic
* now prints out a message for each core above RTE_MAX_LCORE
changes in v4
* tweaked log messages to be a bit clearer about mapping lcores to physical cores.
* improved indenting of log messages.
* fixed bug in overrunning end of lcore array
* switched from strlcpy to strdup because of a clang error
---
---
 lib/eal/common/eal_common_options.c | 100 ++++++++++++++++++++++++----
 1 file changed, 87 insertions(+), 13 deletions(-)
lib/eal/common/eal_common_options.c +0 -0
index eaef57312f..de1717946f 100644

lib/eal/common/eal_common_options.c update_lcore_config(int *cores) · line 709

Quoted 7 lines

lib/eal/common/eal_common_options.c +4 -0

@@ -703,6 +703,47 @@ update_lcore_config(int *cores)
        return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+       unsigned int i, j;

One index variable is enough.

lib/eal/common/eal_common_options.c update_lcore_config(int *cores) · line 720

Quoted 11 lines

lib/eal/common/eal_common_options.c +11 -0

+       char *lcorestr;
+       int len = 0;
+       bool overflow = false;
+
+       for (j = 0; j < count; j++) {
+               if (lcores[j] >= RTE_MAX_LCORE) {
+                       RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+                                       lcores[j], RTE_MAX_LCORE);
+                       overflow = true;
+               }
+       }

if (!overflow)

    return 0;

lib/eal/common/eal_common_options.c update_lcore_config(int *cores) · line 728

Quoted 8 lines

lib/eal/common/eal_common_options.c +8 -0

+       if (overflow) {
+               /*
+                * If we've encountered a core that's greater than
+                * RTE_MAX_LCORE, suggest using --lcores option to
+                * map lcores onto physical cores greater than
+                * RTE_MAX_LCORE, then return.
+                */
+               lcorestr = calloc(1, RTE_MAX_LCORE * 10);

Ugly but works as long as RTE_MAX_LCORE < 10k.
I hope static analysis tools won't complain with the snprintf below where there is no check on return value.

lib/eal/common/eal_common_options.c update_lcore_config(int *cores) · line 738

Quoted 10 lines

lib/eal/common/eal_common_options.c +10 -0

+               if (lcorestr == NULL) {
+                       RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+                       return -ENOMEM;
+               }
+               for (i = 0; i < count; i++)
+                       len = len + snprintf(&lcorestr[len],
+                                       RTE_MAX_LCORE * 10 - len,
+                                       "%d@%d,", i, lcores[i]);
+               if (len > 0)
+                       lcorestr[len-1] = 0;

len - 1

Quoted 12 lines

lib/eal/common/eal_common_options.c +8 -0

+               RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
+               RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
+               free(lcorestr);
+               return -1;
+       }
+       return 0;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {

lib/eal/common/eal_common_options.c eal_parse_service_corelist(const char *corelist) · line 877

Quoted 5 lines

lib/eal/common/eal_common_options.c +1 -1

@@ -833,54 +874,87 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-       unsigned count = 0;
+       unsigned int count = 0, k;

No need for k, count is enough.

lib/eal/common/eal_common_options.c eal_parse_service_corelist(const char *corelist) · line 882

Quoted 5 lines

lib/eal/common/eal_common_options.c +2 -0

        char *end = NULL;
        int min, max;
        int idx;
+       int lcores[RTE_MAX_LCORE];
+       char *corelist_copy;

corelist_copy is unused.

Quoted 35 lines

lib/eal/common/eal_common_options.c +11 -5

        for (idx = 0; idx < RTE_MAX_LCORE; idx++)
                cores[idx] = -1;
 
+       corelist_copy = strdup(corelist);
+       if (corelist_copy == NULL) {
+               RTE_LOG(ERR, EAL, "Unable to duplicate corelist\n");
+               return -ENOMEM;
+       }
+
        /* Remove all blank characters ahead */
        while (isblank(*corelist))
                corelist++;
 
        /* Get list of cores */
-       min = RTE_MAX_LCORE;
+       min = -1;
        do {
                while (isblank(*corelist))
                        corelist++;
                if (*corelist == '\0')
-                       return -1;
+                       goto err;
                errno = 0;
                idx = strtol(corelist, &end, 10);
                if (errno || end == NULL)
-                       return -1;
-               if (idx < 0 || idx >= RTE_MAX_LCORE)
-                       return -1;
+                       goto err;
+               if (idx < 0)
+                       goto err;
                while (isblank(*end))
                        end++;
                if (*end == '-') {

lib/eal/common/eal_common_options.c eal_parse_service_corelist(const char *corelist) · line 872

Quoted 10 lines

lib/eal/common/eal_common_options.c +1 -4

                        min = idx;
                } else if ((*end == ',') || (*end == '\0')) {
                        max = idx;
-                       if (min == RTE_MAX_LCORE)
+                       if (min == -1)
                                min = idx;
                        for (idx = min; idx <= max; idx++) {
-                               if (cores[idx] == -1) {
-                                       cores[idx] = count;
-                                       count++;

Here, in the original code, "duplicates" were accepted/ignored.

Quoted 15 lines

lib/eal/common/eal_common_options.c +8 -2

+                               if (count < RTE_MAX_LCORE)
+                                       lcores[count++] = idx;
+                               else {
+                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+                                                       RTE_MAX_LCORE);
+                                       goto err;
                                }
                        }
-                       min = RTE_MAX_LCORE;
+                       min = -1;
                } else
-                       return -1;
+                       goto err;
                corelist = end + 1;
        } while (*end != '\0');

lib/eal/common/eal_common_options.c eal_parse_service_corelist(const char *corelist) · line 940

Quoted 10 lines

lib/eal/common/eal_common_options.c +7 -1

 
        if (count == 0)
-               return -1;
+               goto err;
+
+       if (check_core_list(lcores, count))
+               goto err;
+
+       /*
+        * Now that we've gto a list of cores no longer than

typo.

lib/eal/common/eal_common_options.c eal_parse_service_corelist(const char *corelist) · line 947

Quoted 7 lines

lib/eal/common/eal_common_options.c +7 -0

+        * RTE_MAX_LCORE, and no lcore in that list is greater
+        * than RTE_MAX_LCORE, populate the cores
+        * array and return.
+        */
+
+       for (k = 0; k < count; k++)
+               cores[lcores[k]] = k;

do {
count--;
cores[lcores[count]] = count;
while (count != 0);

cores[count]

Quoted 16 lines

lib/eal/common/eal_common_options.c +9 -1

+
+       if (corelist_copy)
+               free(corelist_copy);
+
        return 0;
+err:
+       if (corelist_copy)
+               free(corelist_copy);
+
+       return -1;
 }
 
 /* Changes the lcore id of the main thread */
--

2.17.1

Signature · 1 line

David Marchand

David Hunt

author

2021-09-23 09:47 UTC #31 lore raw

Re: [dpdk-dev] [PATCH v4 1/2] eal: add additional info if core list too long

Quoted 19 lines · David Marchand

Quoted 18 lines · David Hunt
If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else 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 to physical cores.
For example, if "-l 12-16,130,132" is used, we see the following additional output on the command line:
EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: lcore 133 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133

Quoted 9 lines

Quoted 2 lines
Signed-off-by David Hunt <[email protected]>
Acked-by Bruce Richardson <[email protected]>
This series is there to help users, it should not break existing working configurations.
I mentionned the "-l 0-3,0" case before.
This syntax is debatable, but it worked before (see comment below) and this patch now refuses it.

Hi David,

Good point that the patch should not change behaviour. Will address, along with your other comments.

Thanks,
Dave.

--snip--

David Hunt

author

2021-09-23 11:03 UTC #32 lore raw

PATCH v5 1/2 eal: add additional info if core list too long

In reply to David Hunt

If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else useful information.

For example, if "-l 12-16,130,132" is used, we see the following additional output on the command line:

Signed-off-by David Hunt <[email protected]>

Acked-by Bruce Richardson <[email protected]>

---
 lib/eal/common/eal_common_options.c | 87 ++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 8 deletions(-)

lib/eal/common/eal_common_options.c +79 -8

index eaef57312f..72735e0b09 100644
@@ -703,6 +703,50 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i;
+	char *lcorestr;
+	int len = 0, ret;
+	bool overflow = false;
+
+	for (i = 0; i < count; i++) {
+		if (lcores[i] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[i], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (!overflow)
+		return 0;
+
+	/*
+	 * We've encountered a core that's greater than
+	 * RTE_MAX_LCORE, suggest using --lcores option to
+	 * map lcores onto physical cores greater than
+	 * RTE_MAX_LCORE, then return.
+	 */
+	lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+	if (lcorestr == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < count; i++) {
+		ret = snprintf(&lcorestr[len],
+				RTE_MAX_LCORE * 10 - len,
+				"%d@%d,", i, lcores[i]);
+		if (ret > 0)
+			len = len + ret;
+	}
+	if (len > 0)
+		lcorestr[len - 1] = 0;
+	RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
+	RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
+	free(lcorestr);
+	return -1;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {

@@ -833,10 +877,12 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, i;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	bool dup;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;

@@ -846,7 +892,7 @@ eal_parse_corelist(const char *corelist, int *cores)
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;

@@ -856,7 +902,7 @@ 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;
 		while (isblank(*end))
 			end++;

@@ -864,15 +910,25 @@ eal_parse_corelist(const char *corelist, int *cores)
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				dup = false;
+				/* Check if this idx is already present */
+				for (i = 0; i < count; i++)
+					if (lcores[i] == idx)
+						dup = true;
+				if (dup)
+					continue;
+				if (count < RTE_MAX_LCORE)
+					lcores[count++] = idx;
+				else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
 			return -1;
 		corelist = end + 1;

@@ -880,6 +936,21 @@ eal_parse_corelist(const char *corelist, int *cores)
 
 	if (count == 0)
 		return -1;
+
+	if (check_core_list(lcores, count))
+		return -1;
+
+	/*
+	 * Now that we've got a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }

Signature · 1 line

2.17.1

David Hunt

author

2021-09-23 11:03 UTC #33 lore raw

PATCH v5 2/2 eal: add additional info if core mask too long

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid coremask syntax" and very little else useful information.

For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:

Signed-off-by David Hunt <[email protected]>

Acked-by Bruce Richardson <[email protected]>

---
changes in v3
* added this patch to the set. Addresses the changes for the -c option.
changes in v4
* fixed buffer overrun in populating lcore array.
* switched from strlcpy to strdup due to a clang error.
changes in v5
* replaced strdup and frees with a const char *, as we just need to keep track of original pointer location.
* reverted err: usage to return -1, as no free() needed.
* other minod code cleanups.
---

---
 lib/eal/common/eal_common_options.c | 47 ++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 7 deletions(-)

lib/eal/common/eal_common_options.c +40 -7

index 72735e0b09..7f715e4c15 100644
@@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	const char *coremask_orig = coremask;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;

@@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
 		return -1;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+					coremask_orig);
 			return -1;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
+				if (count >= RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
+				}
+				lcores[count] = idx;
 				count++;
 			}
 		}
 	}
 	for (; i >= 0; i--)
-		if (coremask[i] != '0')
+		if (coremask[i] != '0') {
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+					coremask_orig);
 			return -1;
-	if (count == 0)
+		}
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
+		return -1;
+	}
+
+	if (check_core_list(lcores, count))
 		return -1;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }

Signature · 1 line

2.17.1

David Marchand

2021-11-02 17:46 UTC #34 lore raw

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

Quoted 59 lines · David Hunt

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid coremask syntax" and very little else 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 to physical cores.
For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:
EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@128,1@129
Signed-off-by David Hunt <[email protected]>
Acked-by Bruce Richardson <[email protected]>
---
changes in v3
* added this patch to the set. Addresses the changes for the -c option.
changes in v4
* fixed buffer overrun in populating lcore array.
* switched from strlcpy to strdup due to a clang error.
changes in v5
* replaced strdup and frees with a const char *, as we just need to keep track of original pointer location.
* reverted err: usage to return -1, as no free() needed.
* other minod code cleanups.
---
---
 lib/eal/common/eal_common_options.c | 47 ++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 7 deletions(-)
lib/eal/common/eal_common_options.c +3 -1
index 72735e0b09..7f715e4c15 100644
@@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-       unsigned count = 0;
+       unsigned int count = 0;
        int i, j, idx;
        int val;
        char c;
+       int lcores[RTE_MAX_LCORE];
+       const char *coremask_orig = coremask;
 
        for (idx = 0; idx < RTE_MAX_LCORE; idx++)
                cores[idx] = -1;
@@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int *cores)
        i = strlen(coremask);
        while ((i > 0) && isblank(coremask[i - 1]))

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 781

Quoted 10 lines

lib/eal/common/eal_common_options.c +5 -2

                i--;
-       if (i == 0)
+       if (i == 0) {
+               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+                               coremask_orig);
                return -1;
+       }
 
-       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+       for (i = i - 1; i >= 0; i--) {

This loop exit condition changes here: this ensures that, once we leave the loop, i == -1.
As a consequence... (see below)

Quoted 13 lines

lib/eal/common/eal_common_options.c +3 -2

                c = coremask[i];
                if (isxdigit(c) == 0) {
                        /* invalid characters */
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+                                       coremask_orig);
                        return -1;
                }
                val = xdigit2val(c);
-               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+               for (j = 0; j < BITS_PER_HEX; j++, idx++)
                {
                        if ((1 << j) & val) {
-                               cores[idx] = count;

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 802

Quoted 10 lines

lib/eal/common/eal_common_options.c +6 -0

+                               if (count >= RTE_MAX_LCORE) {
+                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+                                                       RTE_MAX_LCORE);
+                                       return -1;
+                               }
+                               lcores[count] = idx;
                                count++;
                        }
                }
        }

... this loop below is dead code.

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 806

Quoted 5 lines

lib/eal/common/eal_common_options.c +3 -1

        for (; i >= 0; i--)
-               if (coremask[i] != '0')
+               if (coremask[i] != '0') {
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+                                       coremask_orig);

Nit: capital letter.

Quoted 4 lines

lib/eal/common/eal_common_options.c +2 -1

                        return -1;
-       if (count == 0)
+               }
+       if (count == 0) {

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 819

Quoted 10 lines

lib/eal/common/eal_common_options.c +9 -0

+               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+                               coremask_orig);
+               return -1;
+       }
+
+       if (check_core_list(lcores, count))
                return -1;
+
+       /*
+        * Now that we've gto a list of cores no longer than

Same typo I commented on patch 1 for v4.

Quoted 15 lines

lib/eal/common/eal_common_options.c +9 -1

+        * RTE_MAX_LCORE, and no lcore in that list is greater
+        * than RTE_MAX_LCORE, populate the cores
+        * array and return.
+        */
+       do {
+               count--;
+               cores[lcores[count]] = count;
+       } while (count != 0);
+
        return 0;
 }
 
--

2.17.1

Signature · 1 line

David Marchand

David Hunt

author

2021-11-03 10:27 UTC #35 lore raw

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

Hi David,

Quoted 97 lines · David Marchand

Quoted 58 lines · David Hunt
If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid coremask syntax" and very little else 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 to physical cores.
For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:
EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@128,1@129
Signed-off-by David Hunt <[email protected]>
Acked-by Bruce Richardson <[email protected]>
---
changes in v3
* added this patch to the set. Addresses the changes for the -c option.
changes in v4
* fixed buffer overrun in populating lcore array.
* switched from strlcpy to strdup due to a clang error.
changes in v5
* replaced strdup and frees with a const char *, as we just need to keep track of original pointer location.
* reverted err: usage to return -1, as no free() needed.
* other minod code cleanups.
---
---
  lib/eal/common/eal_common_options.c | 47 ++++++++++++++++++++++++-----
  1 file changed, 40 insertions(+), 7 deletions(-)
lib/eal/common/eal_common_options.c +3 -1
index 72735e0b09..7f715e4c15 100644
@@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
  static int
  eal_parse_coremask(const char *coremask, int *cores)
  {
-       unsigned count = 0;
+       unsigned int count = 0;
         int i, j, idx;
         int val;
         char c;
+       int lcores[RTE_MAX_LCORE];
+       const char *coremask_orig = coremask;
 
         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
                 cores[idx] = -1;
@@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int *cores)
         i = strlen(coremask);
         while ((i > 0) && isblank(coremask[i - 1]))
lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 781
Quoted 10 lines
lib/eal/common/eal_common_options.c +5 -2
                 i--;
-       if (i == 0)
+       if (i == 0) {
+               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+                               coremask_orig);
                 return -1;
+       }
 
-       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+       for (i = i - 1; i >= 0; i--) {
This loop exit condition changes here: this ensures that, once we leave the loop, i == -1.
As a consequence... (see below)
Quoted 13 lines
lib/eal/common/eal_common_options.c +3 -2
                 c = coremask[i];
                 if (isxdigit(c) == 0) {
                         /* invalid characters */
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+                                       coremask_orig);
                         return -1;
                 }
                 val = xdigit2val(c);
-               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+               for (j = 0; j < BITS_PER_HEX; j++, idx++)
                 {
                         if ((1 << j) & val) {
-                               cores[idx] = count;
lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 802
Quoted 10 lines
lib/eal/common/eal_common_options.c +6 -0
+                               if (count >= RTE_MAX_LCORE) {
+                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+                                                       RTE_MAX_LCORE);
+                                       return -1;
+                               }
+                               lcores[count] = idx;
                                 count++;
                         }
                 }
         }

Quoted 1 line

... this loop below is dead code.

Sure, no need to loop. I'll take out the loop, and just check for the first two characters to be '0x', as they're already trimmed.

Quoted 6 lines

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 806

Quoted 5 lines

lib/eal/common/eal_common_options.c +3 -1

         for (; i >= 0; i--)
-               if (coremask[i] != '0')
+               if (coremask[i] != '0') {
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+                                       coremask_orig);

Nit: capital letter.

Many other instances of 'invalid' in this file, will I change them all to "Invalid", or just this one?

Quoted 16 lines

Quoted 5 lines

lib/eal/common/eal_common_options.c +2 -1

                         return -1;
-       if (count == 0)
+               }
+       if (count == 0) {

lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 819

Quoted 10 lines

lib/eal/common/eal_common_options.c +9 -0

+               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+                               coremask_orig);
+               return -1;
+       }
+
+       if (check_core_list(lcores, count))
                 return -1;
+
+       /*
+        * Now that we've gto a list of cores no longer than

Same typo I commented on patch 1 for v4.

Will fix.

Quoted 17 lines

lib/eal/common/eal_common_options.c +9 -1

+        * RTE_MAX_LCORE, and no lcore in that list is greater
+        * than RTE_MAX_LCORE, populate the cores
+        * array and return.
+        */
+       do {
+               count--;
+               cores[lcores[count]] = count;
+       } while (count != 0);
+
         return 0;
  }
 
--

2.17.1

David Marchand

2021-11-03 10:29 UTC #36 lore raw

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

Quoted 11 lines · David Hunt

Quoted 7 lines
lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int *cores) · line 806
Quoted 6 lines
         for (; i >= 0; i--)
lib/eal/common/eal_common_options.c +3 -1
-               if (coremask[i] != '0')
+               if (coremask[i] != '0') {
+                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+                                       coremask_orig);
Nit: capital letter.
Many other instances of 'invalid' in this file, will I change them all to "Invalid", or just this one?

Let's be consistent for additions of this patch.
Thanks.

Signature · 1 line

David Marchand

David Hunt

author

2021-11-03 13:30 UTC #37 lore raw

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

In reply to David Hunt

Quoted 105 lines · David Hunt

Hi David,
Quoted 102 lines · David Marchand
Quoted 60 lines · David Hunt
If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid coremask syntax" and very little else 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 to physical cores.
For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:
EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL:     e.g. --lcores 0@128,1@129
Signed-off-by David Hunt <[email protected]>
Acked-by Bruce Richardson <[email protected]>
---
changes in v3
    * added this patch to the set. Addresses the changes for the -c option.
changes in v4
    * fixed buffer overrun in populating lcore array.
    * switched from strlcpy to strdup due to a clang error. changes in v5
    * replaced strdup and frees with a const char *, as we just need to keep track of original pointer location.
    * reverted err: usage to return -1, as no free() needed.
    * other minod code cleanups.
---
---
lib/eal/common/eal_common_options.c | 47
++++++++++++++++++++++++-----
  1 file changed, 40 insertions(+), 7 deletions(-)
lib/eal/common/eal_common_options.c +0 -0
index 72735e0b09..7f715e4c15 100644
@@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
static int
eal_parse_coremask(const char *coremask, int *cores) {
-       unsigned count = 0;
+       unsigned int count = 0;
         int i, j, idx;
         int val;
         char c;
+       int lcores[RTE_MAX_LCORE];
+       const char *coremask_orig = coremask;
         for (idx = 0; idx < RTE_MAX_LCORE; idx++) cores[idx] = -1;
lib/eal/common/eal_common_options.c +0 -0
@@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int 
 *cores)
          i = strlen(coremask);
          while ((i > 0) && isblank(coremask[i - 1]))
lib/eal/common/eal_common_options.c eal_parse_coremask(const char *coremask, int · line 782
Quoted 10 lines
lib/eal/common/eal_common_options.c +5 -2
                  i--;
-       if (i == 0)
+       if (i == 0) {
+               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+                               coremask_orig);
                  return -1;
+       }
 
-       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+       for (i = i - 1; i >= 0; i--) {
This loop exit condition changes here: this ensures that, once we leave the loop, i == -1.
As a consequence... (see below)
Quoted 26 lines
lib/eal/common/eal_common_options.c +9 -2
                  c = coremask[i];
                  if (isxdigit(c) == 0) {
                          /* invalid characters */
+                       RTE_LOG(ERR, EAL, "invalid characters in 
 coremask: [%s]\n",
+                                       coremask_orig);
                          return -1;
                  }
                  val = xdigit2val(c);
-               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; 
 j++, idx++)
+               for (j = 0; j < BITS_PER_HEX; j++, idx++)
                  {
                          if ((1 << j) & val) {
-                               cores[idx] = count;
+                               if (count >= RTE_MAX_LCORE) {
+                                       RTE_LOG(ERR, EAL, "Too many 
 lcores provided. Cannot exceed %d\n",
+ RTE_MAX_LCORE);
+                                       return -1;
+                               }
+                               lcores[count] = idx;
                                  count++;
                          }
                  }
          }

Quoted 6 lines

Quoted 1 line
... this loop below is dead code.
Sure, no need to loop. I'll take out the loop, and just check for the first two characters to be '0x', as they're already trimmed.

On second thoughts, looking at this closer, the any '0x' or '0X' at the start is skipped earlier in the code, so all we're checking for here is a leading zero to the hex, which does not seem valid, as that would mean that 0x0ff is valid, but 0xff is not.

Take the following 2 cases:

-c f
EAL: Invalid start [0] to coremask: [f]

and even worse:

-c 0xf
EAL: Invalid start [0] to coremask: [0xf]

So I think it makes sense to remove that particular check altogether, in which case both "-c f" and "-c 0xf" will work as expected.

I will make this change in next version.

--snip--

David Hunt

author

2021-11-03 14:32 UTC #38 lore raw

PATCH v6 1/2 eal: add additional info if core list too long

In reply to David Hunt

If the user requests to use an lcore above 128 using -l, the eal will exit with "EAL: invalid core list syntax" and very little else useful information.

For example, if "-l 12-16,130,132" is used, we see the following additional output on the command line:

Signed-off-by David Hunt <[email protected]>

Acked-by Bruce Richardson <[email protected]>

---
changes in v6
* none
changes in v5
* removed un-needed index variables
* now ensures snprintf ret > 0 before using
* removed corelist_copy, not needed.
* reverted err: usage to -1, as no free() needed.
* added in code to allow duplicates
* other code cleanups
changes in v4
* tweaked log messages to be a bit clearer about mapping lcores to physical cores.
* improved indenting of log messages.
* fixed bug in overrunning end of lcore array
* switched from strlcpy to strdup because of a clang error
changes in v3
* separated out some of the corelist cheking into separate function
* added extra messages for the different failure conditions.
* changed allocation of the core strings from static to dynamic
* now prints out a message for each core above RTE_MAX_LCORE
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 | 87 ++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 8 deletions(-)

lib/eal/common/eal_common_options.c +79 -8

index 1802e3d9e1..c35798b288 100644
@@ -703,6 +703,50 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i;
+	char *lcorestr;
+	int len = 0, ret;
+	bool overflow = false;
+
+	for (i = 0; i < count; i++) {
+		if (lcores[i] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[i], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (!overflow)
+		return 0;
+
+	/*
+	 * We've encountered a core that's greater than
+	 * RTE_MAX_LCORE, suggest using --lcores option to
+	 * map lcores onto physical cores greater than
+	 * RTE_MAX_LCORE, then return.
+	 */
+	lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+	if (lcorestr == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < count; i++) {
+		ret = snprintf(&lcorestr[len],
+				RTE_MAX_LCORE * 10 - len,
+				"%d@%d,", i, lcores[i]);
+		if (ret > 0)
+			len = len + ret;
+	}
+	if (len > 0)
+		lcorestr[len - 1] = 0;
+	RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
+	RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
+	free(lcorestr);
+	return -1;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {

@@ -833,10 +877,12 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, i;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	bool dup;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;

@@ -846,7 +892,7 @@ eal_parse_corelist(const char *corelist, int *cores)
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;

@@ -856,7 +902,7 @@ 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;
 		while (isblank(*end))
 			end++;

@@ -864,15 +910,25 @@ eal_parse_corelist(const char *corelist, int *cores)
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				dup = false;
+				/* Check if this idx is already present */
+				for (i = 0; i < count; i++)
+					if (lcores[i] == idx)
+						dup = true;
+				if (dup)
+					continue;
+				if (count < RTE_MAX_LCORE)
+					lcores[count++] = idx;
+				else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
 			return -1;
 		corelist = end + 1;

@@ -880,6 +936,21 @@ eal_parse_corelist(const char *corelist, int *cores)
 
 	if (count == 0)
 		return -1;
+
+	if (check_core_list(lcores, count))
+		return -1;
+
+	/*
+	 * Now that we've got a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }

Signature · 1 line

2.17.1

David Hunt

author

2021-11-03 14:33 UTC #39 lore raw

PATCH v6 2/2 eal: add additional info if core mask too long

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid coremask syntax" and very little else useful information.

For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:

Signed-off-by David Hunt <[email protected]>

Acked-by Bruce Richardson <[email protected]>

---
changes in v6
* Fixed typo (gto -> got).
* Removed dead code for which the intention was to check for 0 at

     the start of the coremask. Also now means that '0xf' is now as
     valid coremask (previously needed to be '0x0f').

changes in v5
* replaced strdup and frees with a const char *, as we just need to keep track of original pointer location.
* reverted err: usage to return -1, as no free() needed.
* other minod code cleanups.
changes in v4
* fixed buffer overrun in populating lcore array.
* switched from strlcpy to strdup due to a clang error.
changes in v3
* added this patch to the set. Addresses the changes for the -c option.

---
 lib/eal/common/eal_common_options.c | 45 +++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 9 deletions(-)

lib/eal/common/eal_common_options.c +36 -9

index c35798b288..0fe3220892 100644
@@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	const char *coremask_orig = coremask;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;

@@ -770,29 +772,54 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
 		return -1;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+					coremask_orig);
 			return -1;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
+				if (count >= RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
+				}
+				lcores[count] = idx;
 				count++;
 			}
 		}
 	}
-	for (; i >= 0; i--)
-		if (coremask[i] != '0')
-			return -1;
-	if (count == 0)
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
+		return -1;
+	}
+
+	if (check_core_list(lcores, count))
 		return -1;
+
+	/*
+	 * Now that we've got a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }

Signature · 1 line

2.17.1

David Marchand

2021-11-05 10:51 UTC #40 lore raw

Re: [dpdk-dev] [PATCH v6 2/2] eal: add additional info if core mask too long

Quoted 13 lines · David Hunt

If the user requests to use an lcore above 128 using -c, the eal will exit with "EAL: invalid coremask syntax" and very little else 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 to physical cores.
For example, if "-c 0x300000000000000000000000000000000" is used, we see the following additional output on the command line:

Quoted 8 lines

EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,
EAL: e.g. --lcores 0@128,1@129
Signed-off-by David Hunt <[email protected]>
Acked-by Bruce Richardson <[email protected]>

Series applied, thanks.

Signature · 1 line

David Marchand

David Hunt

author

2021-09-09 13:45 UTC #41 lore raw

PATCH 2/6 lib/power: reduce memory footprint of acpi lib

In reply to David Hunt

Switch from static memory allocation of core info structs to dynamic allocation. The library used to statically allocate max_lcores number of lcore_power_info structs, so change this to rte_malloc as needed. Reduces static footprint from 192K to 1K.

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by David Hunt <[email protected]>

---
 lib/power/power_acpi_cpufreq.c | 109 +++++++++++++++++++++++++++------
 1 file changed, 90 insertions(+), 19 deletions(-)

lib/power/power_acpi_cpufreq.c +90 ~~-19~~

index 1e8aeb8403..2fa239d02c 100644
@@ -14,6 +14,7 @@
 
 #include <rte_memcpy.h>
 #include <rte_memory.h>
+#include <rte_malloc.h>
 #include <rte_string_fns.h>
 
 #include "power_acpi_cpufreq.h"

@@ -48,7 +49,7 @@ enum power_state {
  * Power info per lcore.
  */
 struct acpi_power_info {
-	unsigned int lcore_id;                   /**< Logical core id */
+	unsigned int lcore_id;               /**< Logical core id */
 	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
 	uint32_t nb_freqs;                   /**< number of available freqs */
 	FILE *f;                             /**< FD of scaling_setspeed */

@@ -59,7 +60,7 @@ struct acpi_power_info {
 	uint16_t turbo_enable;               /**< Turbo Boost enable/disable */
 } __rte_cache_aligned;
 
-static struct acpi_power_info lcore_power_info[RTE_MAX_LCORE];
+static struct acpi_power_info *lcore_power_info[RTE_MAX_LCORE] = { NULL };
 
 /**
  * It is to set specific freq for specific logical core, according to the index

@@ -248,7 +249,17 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		lcore_power_info[lcore_id] =
+			rte_malloc(NULL, sizeof(struct acpi_power_info), 0);
+		if (lcore_power_info[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER, "Cannot allocate core %u\n",
+					lcore_id);
+			return -1;
+		}
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_IDLE;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.

@@ -320,7 +331,13 @@ power_acpi_cpufreq_exit(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE - 1U);
 		return -1;
 	}
-	pi = &lcore_power_info[lcore_id];
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_USED;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.

@@ -354,6 +371,9 @@ power_acpi_cpufreq_exit(unsigned int lcore_id)
 	__atomic_compare_exchange_n(&(pi->state), &exp_state, POWER_IDLE,
 				    0, __ATOMIC_RELEASE, __ATOMIC_RELAXED);
 
+	if (lcore_power_info[lcore_id] != NULL)
+		rte_free(lcore_power_info[lcore_id]);
+
 	return 0;
 
 fail:

@@ -379,7 +399,12 @@ power_acpi_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
 		return 0;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (num < pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
 		return 0;

@@ -397,7 +422,12 @@ power_acpi_cpufreq_get_freq(unsigned int lcore_id)
 		return RTE_POWER_INVALID_FREQ_INDEX;
 	}
 
-	return lcore_power_info[lcore_id].curr_idx;
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return lcore_power_info[lcore_id]->curr_idx;
 }
 
 int

@@ -408,7 +438,12 @@ power_acpi_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
 		return -1;
 	}
 
-	return set_freq_internal(&(lcore_power_info[lcore_id]), index);
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return set_freq_internal(lcore_power_info[lcore_id], index);
 }
 
 int

@@ -421,7 +456,12 @@ power_acpi_cpufreq_freq_down(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx + 1 == pi->nb_freqs)
 		return 0;

@@ -439,7 +479,12 @@ power_acpi_cpufreq_freq_up(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx == 0 ||
 	    (pi->curr_idx == 1 && pi->turbo_available && !pi->turbo_enable))
 		return 0;

@@ -457,17 +502,17 @@ power_acpi_cpufreq_freq_max(unsigned int lcore_id)
 	}
 
 	/* Frequencies in the array are from high to low. */
-	if (lcore_power_info[lcore_id].turbo_available) {
-		if (lcore_power_info[lcore_id].turbo_enable)
+	if (lcore_power_info[lcore_id]->turbo_available) {
+		if (lcore_power_info[lcore_id]->turbo_enable)
 			/* Set to Turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 0);
+					lcore_power_info[lcore_id], 0);
 		else
 			/* Set to max non-turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 1);
+					lcore_power_info[lcore_id], 1);
 	} else
-		return set_freq_internal(&lcore_power_info[lcore_id], 0);
+		return set_freq_internal(lcore_power_info[lcore_id], 0);
 }
 
 int

@@ -480,7 +525,12 @@ power_acpi_cpufreq_freq_min(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	/* Frequencies in the array are from high to low. */
 	return set_freq_internal(pi, pi->nb_freqs - 1);

@@ -497,7 +547,12 @@ power_acpi_turbo_status(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	return pi->turbo_enable;
 }

@@ -513,7 +568,12 @@ power_acpi_enable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	if (pi->turbo_available)
 		pi->turbo_enable = 1;

@@ -546,7 +606,12 @@ power_acpi_disable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	 pi->turbo_enable = 0;

@@ -572,12 +637,18 @@ int power_acpi_get_capabilities(unsigned int lcore_id,
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
 		return -1;
 	}
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
 	if (caps == NULL) {
 		RTE_LOG(ERR, POWER, "Invalid argument\n");
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	pi = lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);

Signature · 1 line

2.17.1

David Hunt

author

2021-09-09 13:45 UTC #42 lore raw

PATCH 3/6 lib/power: reduce memory footprint of pstate lib

In reply to David Hunt

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by David Hunt <[email protected]>

---
 lib/power/power_pstate_cpufreq.c | 111 ++++++++++++++++++++++++++-----
 1 file changed, 93 insertions(+), 18 deletions(-)

lib/power/power_pstate_cpufreq.c +93 ~~-18~~

index 86f8a76e46..3e6529f589 100644
@@ -16,6 +16,7 @@
 
 #include <rte_memcpy.h>
 #include <rte_memory.h>
+#include <rte_malloc.h>
 #include <rte_string_fns.h>
 
 #include "power_pstate_cpufreq.h"

@@ -76,7 +77,7 @@ struct pstate_power_info {
 } __rte_cache_aligned;
 
 
-static struct pstate_power_info lcore_power_info[RTE_MAX_LCORE];
+static struct pstate_power_info *lcore_power_info[RTE_MAX_LCORE] = { NULL };
 
 /**
  * It is to read the specific MSR.

@@ -518,7 +519,17 @@ power_pstate_cpufreq_init(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		lcore_power_info[lcore_id] =
+			rte_malloc(NULL, sizeof(struct pstate_power_info), 0);
+		if (lcore_power_info[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER, "Cannot allocate core %u\n",
+					lcore_id);
+			return -1;
+		}
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_IDLE;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.

@@ -595,7 +606,13 @@ power_pstate_cpufreq_exit(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE - 1U);
 		return -1;
 	}
-	pi = &lcore_power_info[lcore_id];
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	exp_state = POWER_USED;
 	/* The power in use state works as a guard variable between

@@ -632,6 +649,9 @@ power_pstate_cpufreq_exit(unsigned int lcore_id)
 	__atomic_compare_exchange_n(&(pi->state), &exp_state, POWER_IDLE,
 				    0, __ATOMIC_RELEASE, __ATOMIC_RELAXED);
 
+	if (lcore_power_info[lcore_id] != NULL)
+		rte_free(lcore_power_info[lcore_id]);
+
 	return 0;
 
 fail:

@@ -658,7 +678,12 @@ power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
 		return 0;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (num < pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
 		return 0;

@@ -676,7 +701,12 @@ power_pstate_cpufreq_get_freq(unsigned int lcore_id)
 		return RTE_POWER_INVALID_FREQ_INDEX;
 	}
 
-	return lcore_power_info[lcore_id].curr_idx;
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return lcore_power_info[lcore_id]->curr_idx;
 }

@@ -688,7 +718,12 @@ power_pstate_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
 		return -1;
 	}
 
-	return set_freq_internal(&(lcore_power_info[lcore_id]), index);
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return set_freq_internal(lcore_power_info[lcore_id], index);
 }
 
 int

@@ -701,7 +736,12 @@ power_pstate_cpufreq_freq_up(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx == 0 ||
 	    (pi->curr_idx == 1 && pi->turbo_available && !pi->turbo_enable))
 		return 0;

@@ -720,7 +760,12 @@ power_pstate_cpufreq_freq_down(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx + 1 == pi->nb_freqs)
 		return 0;

@@ -736,18 +781,23 @@ power_pstate_cpufreq_freq_max(unsigned int lcore_id)
 		return -1;
 	}
 
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
 	/* Frequencies in the array are from high to low. */
-	if (lcore_power_info[lcore_id].turbo_available) {
-		if (lcore_power_info[lcore_id].turbo_enable)
+	if (lcore_power_info[lcore_id]->turbo_available) {
+		if (lcore_power_info[lcore_id]->turbo_enable)
 			/* Set to Turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 0);
+					lcore_power_info[lcore_id], 0);
 		else
 			/* Set to max non-turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 1);
+					lcore_power_info[lcore_id], 1);
 	} else
-		return set_freq_internal(&lcore_power_info[lcore_id], 0);
+		return set_freq_internal(lcore_power_info[lcore_id], 0);
 }

@@ -761,7 +811,12 @@ power_pstate_cpufreq_freq_min(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	/* Frequencies in the array are from high to low. */
 	return set_freq_internal(pi, pi->nb_freqs - 1);

@@ -778,7 +833,12 @@ power_pstate_turbo_status(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	return pi->turbo_enable;
 }

@@ -793,7 +853,12 @@ power_pstate_enable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	if (pi->turbo_available)
 		pi->turbo_enable = 1;

@@ -819,7 +884,12 @@ power_pstate_disable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	pi->turbo_enable = 0;

@@ -851,7 +921,12 @@ int power_pstate_get_capabilities(unsigned int lcore_id,
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
 	caps->priority = pi->priority_core;

Signature · 1 line

2.17.1

David Hunt

author

2021-09-09 13:45 UTC #43 lore raw

PATCH 4/6 lib/power: reduce memory footprint of cppc lib

In reply to David Hunt

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by David Hunt <[email protected]>

---
 lib/power/power_cppc_cpufreq.c | 111 +++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 18 deletions(-)

lib/power/power_cppc_cpufreq.c +93 ~~-18~~

index 6afd310e4e..952daf312e 100644
@@ -5,6 +5,7 @@
 
 #include <rte_memcpy.h>
 #include <rte_memory.h>
+#include <rte_malloc.h>
 
 #include "power_cppc_cpufreq.h"
 #include "power_common.h"

@@ -61,7 +62,7 @@ struct cppc_power_info {
 	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
 } __rte_cache_aligned;
 
-static struct cppc_power_info lcore_power_info[RTE_MAX_LCORE];
+static struct cppc_power_info *lcore_power_info[RTE_MAX_LCORE] = { NULL };
 
 /**
  * It is to set specific freq for specific logical core, according to the index

@@ -344,7 +345,17 @@ power_cppc_cpufreq_init(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		lcore_power_info[lcore_id] =
+			rte_malloc(NULL, sizeof(struct cppc_power_info), 0);
+		if (lcore_power_info[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER, "Cannot allocate core %u\n",
+					lcore_id);
+			return -1;
+		}
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_IDLE;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.

@@ -422,7 +433,13 @@ power_cppc_cpufreq_exit(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE - 1U);
 		return -1;
 	}
-	pi = &lcore_power_info[lcore_id];
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_USED;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.

@@ -454,6 +471,9 @@ power_cppc_cpufreq_exit(unsigned int lcore_id)
 			"original\n", lcore_id);
 	__atomic_store_n(&(pi->state), POWER_IDLE, __ATOMIC_RELEASE);
 
+	if (lcore_power_info[lcore_id] != NULL)
+		rte_free(lcore_power_info[lcore_id]);
+
 	return 0;
 
 fail:

@@ -477,7 +497,12 @@ power_cppc_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
 		return 0;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (num < pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
 		return 0;

@@ -495,7 +520,12 @@ power_cppc_cpufreq_get_freq(unsigned int lcore_id)
 		return RTE_POWER_INVALID_FREQ_INDEX;
 	}
 
-	return lcore_power_info[lcore_id].curr_idx;
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return lcore_power_info[lcore_id]->curr_idx;
 }
 
 int

@@ -506,7 +536,12 @@ power_cppc_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
 		return -1;
 	}
 
-	return set_freq_internal(&(lcore_power_info[lcore_id]), index);
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return set_freq_internal(lcore_power_info[lcore_id], index);
 }
 
 int

@@ -519,7 +554,12 @@ power_cppc_cpufreq_freq_down(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx + 1 == pi->nb_freqs)
 		return 0;

@@ -537,7 +577,12 @@ power_cppc_cpufreq_freq_up(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx == 0 || (pi->curr_idx == 1 &&
 		pi->turbo_available && !pi->turbo_enable))
 		return 0;

@@ -554,18 +599,23 @@ power_cppc_cpufreq_freq_max(unsigned int lcore_id)
 		return -1;
 	}
 
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
 	/* Frequencies in the array are from high to low. */
-	if (lcore_power_info[lcore_id].turbo_available) {
-		if (lcore_power_info[lcore_id].turbo_enable)
+	if (lcore_power_info[lcore_id]->turbo_available) {
+		if (lcore_power_info[lcore_id]->turbo_enable)
 			/* Set to Turbo */
 			return set_freq_internal(
-				&lcore_power_info[lcore_id], 0);
+				lcore_power_info[lcore_id], 0);
 		else
 			/* Set to max non-turbo */
 			return set_freq_internal(
-				&lcore_power_info[lcore_id], 1);
+				lcore_power_info[lcore_id], 1);
 	} else
-		return set_freq_internal(&lcore_power_info[lcore_id], 0);
+		return set_freq_internal(lcore_power_info[lcore_id], 0);
 }
 
 int

@@ -578,7 +628,12 @@ power_cppc_cpufreq_freq_min(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	/* Frequencies in the array are from high to low. */
 	return set_freq_internal(pi, pi->nb_freqs - 1);

@@ -594,7 +649,12 @@ power_cppc_turbo_status(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	return pi->turbo_enable;
 }

@@ -609,7 +669,12 @@ power_cppc_enable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	if (pi->turbo_available)
 		pi->turbo_enable = 1;

@@ -645,7 +710,12 @@ power_cppc_disable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	pi->turbo_enable = 0;

@@ -677,7 +747,12 @@ power_cppc_get_capabilities(unsigned int lcore_id,
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);

Signature · 1 line

2.17.1

David Hunt

author

2021-09-09 13:45 UTC #44 lore raw

PATCH 5/6 lib/power: reduce memory footprint of channels

In reply to David Hunt

Switch from static memory allocation of channel pkt structs to dynamic allocation. The library used to statically allocate max_lcores number of rte_power_channel_packet structs, so change this to rte_malloc as needed. Reduces static footprint from 236K to 1K.

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by David Hunt <[email protected]>

---
 lib/power/power_kvm_vm.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

lib/power/power_kvm_vm.c +31 -5

index ab7d4b8cee..c60e7acf16 100644
@@ -5,6 +5,7 @@
 #include <string.h>
 
 #include <rte_log.h>
+#include <rte_malloc.h>
 
 #include "rte_power_guest_channel.h"
 #include "guest_channel.h"

@@ -13,7 +14,8 @@
 
 #define FD_PATH "/dev/virtio-ports/virtio.serial.port.poweragent"
 
-static struct rte_power_channel_packet pkt[RTE_MAX_LCORE];
+static struct rte_power_channel_packet
+		*power_channel_pkt[RTE_MAX_LCORE] = { NULL };
 
 int
 power_kvm_vm_check_supported(void)

@@ -29,8 +31,21 @@ power_kvm_vm_init(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE-1);
 		return -1;
 	}
-	pkt[lcore_id].command = RTE_POWER_CPU_POWER;
-	pkt[lcore_id].resource_id = lcore_id;
+
+	if (power_channel_pkt[lcore_id] == NULL) {
+		power_channel_pkt[lcore_id] =
+			rte_malloc(NULL,
+				sizeof(struct rte_power_channel_packet), 0);
+		if (power_channel_pkt[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER,
+				"Cannot allocate channel for core %u\n",
+				lcore_id);
+			return -1;
+		}
+	}
+
+	power_channel_pkt[lcore_id]->command = RTE_POWER_CPU_POWER;
+	power_channel_pkt[lcore_id]->resource_id = lcore_id;
 	return guest_channel_host_connect(FD_PATH, lcore_id);
 }

@@ -38,6 +53,10 @@ int
 power_kvm_vm_exit(unsigned int lcore_id)
 {
 	guest_channel_host_disconnect(lcore_id);
+
+	if (power_channel_pkt[lcore_id] != NULL)
+		rte_free(power_channel_pkt[lcore_id]);
+
 	return 0;
 }

@@ -78,8 +97,15 @@ send_msg(unsigned int lcore_id, uint32_t scale_direction)
 				lcore_id, RTE_MAX_LCORE-1);
 		return -1;
 	}
-	pkt[lcore_id].unit = scale_direction;
-	ret = guest_channel_send_msg(&pkt[lcore_id], lcore_id);
+
+	if (power_channel_pkt[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "channel for core %u not initialised\n",
+				lcore_id);
+		return -1;
+	}
+
+	power_channel_pkt[lcore_id]->unit = scale_direction;
+	ret = guest_channel_send_msg(power_channel_pkt[lcore_id], lcore_id);
 	if (ret == 0)
 		return 1;
 	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",

Signature · 1 line

2.17.1

David Hunt

author

2021-09-09 13:46 UTC #45 lore raw

PATCH 6/6 lib/power: switch empty poll to max cores config

In reply to David Hunt

empty poll algorithm uses NUM_NODES as 256, but should really use RTE_MAX_LCORE, which is configurable.

Signed-off-by David Hunt <[email protected]>

---
 lib/power/rte_power_empty_poll.c | 12 ++++++------
 lib/power/rte_power_empty_poll.h |  4 +---
 2 files changed, 7 insertions(+), 9 deletions(-)

lib/power/rte_power_empty_poll.c +6 -6

index 975aa92997..fe20b9f7b2 100644
@@ -373,7 +373,7 @@ rte_empty_poll_detection(struct rte_timer *tim, void *arg)
 
 	RTE_SET_USED(arg);
 
-	for (i = 0; i < NUM_NODES; i++) {
+	for (i = 0; i < RTE_MAX_LCORE; i++) {
 
 		poll_stats = &(ep_params->wrk_data.wrk_stats[i]);

@@ -436,7 +436,7 @@ rte_power_empty_poll_stat_init(struct ep_params **eptr, uint8_t *freq_tlb,
 	*eptr = ep_params;
 
 	/* initialize all wrk_stats state */
-	for (i = 0; i < NUM_NODES; i++) {
+	for (i = 0; i < RTE_MAX_LCORE; i++) {
 
 		if (rte_lcore_is_enabled(i) == 0)
 			continue;

@@ -476,7 +476,7 @@ rte_power_empty_poll_stat_update(unsigned int lcore_id)
 {
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);

@@ -495,7 +495,7 @@ rte_power_poll_stat_update(unsigned int lcore_id, uint8_t nb_pkt)
 
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);

@@ -514,7 +514,7 @@ rte_power_empty_poll_stat_fetch(unsigned int lcore_id)
 {
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);

@@ -530,7 +530,7 @@ rte_power_poll_stat_fetch(unsigned int lcore_id)
 {
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);

lib/power/rte_power_empty_poll.h +1 -3

index 6ba0a37074..22678b4f4b 100644
@@ -31,8 +31,6 @@ extern "C" {
 
 #define NUM_PRIORITIES          2
 
-#define NUM_NODES         256  /* Max core number*/
-
 /* Processor Power State */
 enum freq_val {
 	LOW,

@@ -98,7 +96,7 @@ struct priority_worker {
 
 struct stats_data {
 
-	struct priority_worker wrk_stats[NUM_NODES];
+	struct priority_worker wrk_stats[RTE_MAX_LCORE];
 
 	/* flag to stop rx threads processing packets until training over */
 	bool start_rx;

Signature · 1 line

2.17.1