eal/windows: ensure all the CPUs in the set are checked

PATCH In review Narcisa Ana Maria Vasile 4 participants 9 messages Jun 25, 2021 – Oct 11, 2021

lore

Share & tools

lore

Narcisa Ana Maria Vasile

author

2021-06-25 00:27 UTC #0 lore raw

PATCH 1/1 eal/windows: ensure all the CPUs in the set are checked

From: Narcisa Vasile <[email protected]>

Fix count_cpu() to ensure it iterates through all the CPUs in a set. count_cpu() iterates through the CPUs in the set 's' and counts the selected ones.

Previously, it was incorrectly using the number of CPUSETS to iterate through the CPUs.

Signed-off-by Narcisa Vasile <[email protected]>

---
 lib/eal/windows/include/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/eal/windows/include/sched.h +1 -1

index ff572b5dcb..bc31cc8465 100644
@@ -49,7 +49,7 @@ count_cpu(rte_cpuset_t *s)
 	unsigned int _i;
 	int count = 0;
 
-	for (_i = 0; _i < _NUM_SETS(CPU_SETSIZE); _i++)
+	for (_i = 0; _i < CPU_SETSIZE; _i++)
 		if (CPU_ISSET(_i, s) != 0LL)
 			count++;
 	return count;

Signature · 1 line

2.31.0.vfs.0.1

Dmitry Kozlyuk

2021-06-25 08:36 UTC #1 lore raw

2021-06-24 17:27 (UTC-0700), Narcisa Ana Maria Vasile:

Quoted 19 lines

From: Narcisa Vasile <[email protected]>
Fix count_cpu() to ensure it iterates through all the CPUs in a set. count_cpu() iterates through the CPUs in the set 's' and counts the selected ones.
Previously, it was incorrectly using the number of CPUSETS to iterate through the CPUs.
Signed-off-by Narcisa Vasile <[email protected]>
---
 lib/eal/windows/include/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/eal/windows/include/sched.h +0 -0
index ff572b5dcb..bc31cc8465 100644

lib/eal/windows/include/sched.h count_cpu(rte_cpuset_t *s) · line 55

Quoted 8 lines

lib/eal/windows/include/sched.h +1 -1

@@ -49,7 +49,7 @@ count_cpu(rte_cpuset_t *s)
 	unsigned int _i;
 	int count = 0;
 
-	for (_i = 0; _i < _NUM_SETS(CPU_SETSIZE); _i++)
+	for (_i = 0; _i < CPU_SETSIZE; _i++)
 		if (CPU_ISSET(_i, s) != 0LL)
 			count++;
 	return count;

Hi Naty,

Thank you for the fix, but we also need a proper commit message:

https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body

Specifically, please, describe what was the observable issue (usually first comes what was wrong, then how it is fixed now) and add "Fixes" tag and Cc. Also, "number of CPUSETS" sounds unclear, as there's no "CPUSET".
Suggestion: "number of bitset limbs" or maybe if you describe what was wrong with the result you won't need to describe its reason precisely at all.

Narcisa Ana Maria Vasile

author

2021-06-30 02:02 UTC #2 lore raw

Quoted 19 lines · Dmitry Kozlyuk

2021-06-24 17:27 (UTC-0700), Narcisa Ana Maria Vasile:
Quoted 6 lines
From: Narcisa Vasile <[email protected]>
Fix count_cpu() to ensure it iterates through all the CPUs in a set. count_cpu() iterates through the CPUs in the set 's' and counts the selected ones.
Quoted 7 lines
Previously, it was incorrectly using the number of CPUSETS to iterate through the CPUs.
Signed-off-by Narcisa Vasile <[email protected]>
---
 lib/eal/windows/include/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Hi Naty,
Thank you for the fix, but we also need a proper commit message:

Quoted 7 lines

https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body
Specifically, please, describe what was the observable issue (usually first comes what was wrong, then how it is fixed now) and add "Fixes" tag and Cc. Also, "number of CPUSETS" sounds unclear, as there's no "CPUSET".
Suggestion: "number of bitset limbs" or maybe if you describe what was wrong with the result you won't need to describe its reason precisely at all.

Ah, I've mixed some terminology here..
Thank you Dmitry for the feedback!

Narcisa Ana Maria Vasile

author

2021-06-30 01:56 UTC #3 lore raw

PATCH v2 1/1 eal/windows: ensure all enabled CPUs are counted

In reply to Narcisa Ana Maria Vasile

From: Narcisa Vasile <[email protected]>

rte_cpuset_t describes a set of CPUs by using an array of masks named '_bits'. Each element in the '_bits' array represents a bit mask, with each bit corresponding to a CPU.
The maximum number of CPUs is given by 'CPU_SETSIZE'. The number of bit masks is computed using '_NUM_SETS(CPU_SETSIZE)'.

count_cpu() should count the number of CPUs enabled in the set 's'. Currently, it iterates through the number of masks in the set 's', instead of iterating through all the bits in all the masks. For example, if '_NUM_SETS(CPU_SETSIZE)' returns 2,
which means there are 2 bit masks: _bits[0] and _bits[1], count_cpu() would only check if CPUs '0' and '1' are enabled. The correct behavior is to iterate through all the CPUs in the set and count the ones that are enabled.

This patch fixes count_cpu() to ensure all the bits in all the masks are checked to compute the correct number of CPUs enabled in 's'.

Fixes e8428a9d89f1 ("eal/windows: add some basic functions and macros")

Cc [email protected]

Signed-off-by Narcisa Vasile <[email protected]>

---
v2:
* Fix commit message.

 lib/eal/windows/include/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/eal/windows/include/sched.h +1 -1

index ff572b5dcb..bc31cc8465 100644
@@ -49,7 +49,7 @@ count_cpu(rte_cpuset_t *s)
 	unsigned int _i;
 	int count = 0;
 
-	for (_i = 0; _i < _NUM_SETS(CPU_SETSIZE); _i++)
+	for (_i = 0; _i < CPU_SETSIZE; _i++)
 		if (CPU_ISSET(_i, s) != 0LL)
 			count++;
 	return count;

Signature · 1 line

2.31.0.vfs.0.1

Dmitry Kozlyuk

Acked-by on v2 1/1

2021-08-04 14:57 UTC #4 lore raw

Re: [dpdk-dev] [PATCH v2] eal/windows: ensure all enabled CPUs are counted

2021-06-29 18:56 (UTC-0700), Narcisa Ana Maria Vasile:

Quoted 37 lines

From: Narcisa Vasile <[email protected]>
rte_cpuset_t describes a set of CPUs by using an array of masks named '_bits'. Each element in the '_bits' array represents a bit mask, with each bit corresponding to a CPU.
The maximum number of CPUs is given by 'CPU_SETSIZE'. The number of bit masks is computed using '_NUM_SETS(CPU_SETSIZE)'.
count_cpu() should count the number of CPUs enabled in the set 's'. Currently, it iterates through the number of masks in the set 's', instead of iterating through all the bits in all the masks. For example, if '_NUM_SETS(CPU_SETSIZE)' returns 2,
which means there are 2 bit masks: _bits[0] and _bits[1], count_cpu() would only check if CPUs '0' and '1' are enabled. The correct behavior is to iterate through all the CPUs in the set and count the ones that are enabled.
This patch fixes count_cpu() to ensure all the bits in all the masks are checked to compute the correct number of CPUs enabled in 's'.
Fixes e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Cc [email protected]
Cc [email protected]
Signed-off-by Narcisa Vasile <[email protected]>
---
v2:
* Fix commit message.
 lib/eal/windows/include/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/eal/windows/include/sched.h +0 -0
index ff572b5dcb..bc31cc8465 100644

lib/eal/windows/include/sched.h count_cpu(rte_cpuset_t *s) · line 55

Quoted 8 lines

lib/eal/windows/include/sched.h +1 -1

@@ -49,7 +49,7 @@ count_cpu(rte_cpuset_t *s)
 	unsigned int _i;
 	int count = 0;
 
-	for (_i = 0; _i < _NUM_SETS(CPU_SETSIZE); _i++)
+	for (_i = 0; _i < CPU_SETSIZE; _i++)
 		if (CPU_ISSET(_i, s) != 0LL)
 			count++;
 	return count;

Sorry, I lost track of this patch.
Great and thorough explanation, but I don't think it needs to be so long. Suggestion:

On Windows, -l/--lcores EAL option was unable to process CPU sets containing CPUs other than 0 and 1, because CPU_COUNT() macro only checked these CPUs in the set. Fix CPU_COUNT() by enumerating all possible CPU indices.

Whatever message is taken in the end,

Acked-by Dmitry Kozlyuk <[email protected]>

Kadam, Pallavi

Acked-by on v2 1/1

2021-08-04 20:40 UTC #5 lore raw

Re: [dpdk-dev] [PATCH v2] eal/windows: ensure all enabled CPUs are counted

In reply to Narcisa Ana Maria Vasile

Quoted 20 lines · Narcisa Ana Maria Vasile

From: Narcisa Vasile <[email protected]>
rte_cpuset_t describes a set of CPUs by using an array of masks named '_bits'. Each element in the '_bits' array represents a bit mask, with each bit corresponding to a CPU.
The maximum number of CPUs is given by 'CPU_SETSIZE'. The number of bit masks is computed using '_NUM_SETS(CPU_SETSIZE)'.
count_cpu() should count the number of CPUs enabled in the set 's'. Currently, it iterates through the number of masks in the set 's', instead of iterating through all the bits in all the masks. For example, if '_NUM_SETS(CPU_SETSIZE)' returns 2,
which means there are 2 bit masks: _bits[0] and _bits[1], count_cpu() would only check if CPUs '0' and '1' are enabled. The correct behavior is to iterate through all the CPUs in the set and count the ones that are enabled.
This patch fixes count_cpu() to ensure all the bits in all the masks are checked to compute the correct number of CPUs enabled in 's'.

Quoted 6 lines

Fixes e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Cc [email protected]
Cc [email protected]
Signed-off-by Narcisa Vasile <[email protected]>
---

Acked-by Pallavi Kadam <[email protected]>

Narcisa Ana Maria Vasile

author

2021-08-18 13:46 UTC #6 lore raw

PATCH v3 1/1 eal/windows: ensure all enabled CPUs are counted

In reply to Narcisa Ana Maria Vasile

From: Narcisa Vasile <[email protected]>

Fixes e8428a9d89f1 ("eal/windows: add some basic functions and macros")

Cc [email protected]

Signed-off-by Narcisa Vasile <[email protected]>

Acked-by Dmitry Kozlyuk <[email protected]>

Acked-by Pallavi Kadam <[email protected]>

---
 lib/eal/windows/include/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/eal/windows/include/sched.h +1 -1

index ff572b5dcb..bc31cc8465 100644
@@ -49,7 +49,7 @@ count_cpu(rte_cpuset_t *s)
 	unsigned int _i;
 	int count = 0;
 
-	for (_i = 0; _i < _NUM_SETS(CPU_SETSIZE); _i++)
+	for (_i = 0; _i < CPU_SETSIZE; _i++)
 		if (CPU_ISSET(_i, s) != 0LL)
 			count++;
 	return count;

Signature · 1 line

2.31.0.vfs.0.1

Narcisa Ana Maria Vasile

author

2021-10-08 23:56 UTC #7 lore raw

Re: [dpdk-dev] [PATCH v3] eal/windows: ensure all enabled CPUs are counted

Quoted 7 lines · Narcisa Ana Maria Vasile

From: Narcisa Vasile <[email protected]>
On Windows, -l/--lcores EAL option was unable to process CPU sets containing CPUs other than 0 and 1, because CPU_COUNT() macro only checked these CPUs in the set. Fix CPU_COUNT() by enumerating all possible CPU indices.

Quoted 8 lines

Fixes e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Cc [email protected]
Cc [email protected]
Signed-off-by Narcisa Vasile <[email protected]>
Acked-by Dmitry Kozlyuk <[email protected]>
Acked-by Pallavi Kadam <[email protected]>
---

Can this be merged? This small fix will solve the issues related to threads affinity on Windows. Thanks!

Thomas Monjalon

Applied

2021-10-11 18:48 UTC #8 lore raw

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] eal/windows: ensure all enabled CPUs are counted

In reply to Narcisa Ana Maria Vasile

18/08/2021 15:46, Narcisa Ana Maria Vasile:

Quoted 7 lines

From: Narcisa Vasile <[email protected]>
On Windows, -l/--lcores EAL option was unable to process CPU sets containing CPUs other than 0 and 1, because CPU_COUNT() macro only checked these CPUs in the set. Fix CPU_COUNT() by enumerating all possible CPU indices.

Quoted 7 lines

Fixes e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Cc [email protected]
Cc [email protected]
Signed-off-by Narcisa Vasile <[email protected]>
Acked-by Dmitry Kozlyuk <[email protected]>
Acked-by Pallavi Kadam <[email protected]>

Applied, thanks.
Sorry for the delay.