[dpdk-dev] [PATCH] efd: change data type of parameter

[dpdk-dev] [PATCH] efd: change data type of parameter

[Pablo de Lara]

rte_efd_create() function was using uint8_t for a socket bitmask,
for one of its parameters.
This limits the maximum of NUMA sockets to be 8.
Changing to to uint64_t increases it to 64, which should be
more future-proof.

Coverity issue: 366390
Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---

This fix requires an API breakage and therefore it is not
a good candidate for backporting (besides, it is a very low impact bug).
Hence, I am not CC'ing stable.

---

 lib/efd/rte_efd.c | 2 +-
 lib/efd/rte_efd.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
index 77f46809f8..68a2378e88 100644
--- a/lib/efd/rte_efd.c
+++ b/lib/efd/rte_efd.c
@@ -495,7 +495,7 @@ efd_search_hash(struct rte_efd_table * const table,
 
 struct rte_efd_table *
 rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
-		uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
+		uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
 {
 	struct rte_efd_table *table = NULL;
 	uint8_t *key_array = NULL;
diff --git a/lib/efd/rte_efd.h b/lib/efd/rte_efd.h
index c2be4c09ae..d3d7befd0c 100644
--- a/lib/efd/rte_efd.h
+++ b/lib/efd/rte_efd.h
@@ -139,7 +139,7 @@ typedef uint16_t efd_hashfunc_t;
  */
 struct rte_efd_table *
 rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
-	uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
+	uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
 
 /**
  * Releases the resources from an EFD table
-- 
2.25.1

Re: [dpdk-dev] [PATCH] efd: change data type of parameter

[Mcnamara, John]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Pablo de Lara
> Sent: Friday, September 10, 2021 5:54 PM
> To: Wang, Yipeng1 <yipeng1.wang@intel.com>; Marohn, Byron
> <byron.marohn@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: [dpdk-dev] [PATCH] efd: change data type of parameter
> 
> rte_efd_create() function was using uint8_t for a socket bitmask, for one
> of its parameters.
> This limits the maximum of NUMA sockets to be 8.
> Changing to to uint64_t increases it to 64, which should be more future-
> proof.
> 
> Coverity issue: 366390
> Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
> 


As noted in the commit message this change is required to fix Coverity defect 366390. 

https://scan4.coverity.com/reports.htm#v34998/p10075/fileInstanceId=107231002&defectInstanceId=14260754&mergedDefectId=366390


Acked-by: John McNamara <john.mcnamara@intel.com>

Re: [dpdk-dev] [PATCH] efd: change data type of parameter

[David Marchand]

On Fri, Sep 10, 2021 at 6:54 PM Pablo de Lara
<pablo.de.lara.guarch@intel.com> wrote:
>
> rte_efd_create() function was using uint8_t for a socket bitmask,
> for one of its parameters.
> This limits the maximum of NUMA sockets to be 8.
> Changing to to uint64_t increases it to 64, which should be
> more future-proof.

Cc: ppc maintainer, since I think powerX servers have non contiguous
NUMA sockets.


>
> Coverity issue: 366390
> Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
>
> This fix requires an API breakage and therefore it is not
> a good candidate for backporting (besides, it is a very low impact bug).
> Hence, I am not CC'ing stable.

This is an unannounced breakage for a stable API.
Cc: techboard + Ray for awareness.


>
> ---
>
>  lib/efd/rte_efd.c | 2 +-
>  lib/efd/rte_efd.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
> index 77f46809f8..68a2378e88 100644
> --- a/lib/efd/rte_efd.c
> +++ b/lib/efd/rte_efd.c
> @@ -495,7 +495,7 @@ efd_search_hash(struct rte_efd_table * const table,
>
>  struct rte_efd_table *
>  rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
> -               uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
> +               uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
>  {
>         struct rte_efd_table *table = NULL;
>         uint8_t *key_array = NULL;
> diff --git a/lib/efd/rte_efd.h b/lib/efd/rte_efd.h
> index c2be4c09ae..d3d7befd0c 100644
> --- a/lib/efd/rte_efd.h
> +++ b/lib/efd/rte_efd.h
> @@ -139,7 +139,7 @@ typedef uint16_t efd_hashfunc_t;
>   */
>  struct rte_efd_table *
>  rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
> -       uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
> +       uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
>
>  /**
>   * Releases the resources from an EFD table
> --
> 2.25.1
>


-- 
David Marchand

Re: [dpdk-dev] [PATCH] efd: change data type of parameter

[Kinsella, Ray]

On 14/09/2021 08:10, David Marchand wrote:
> On Fri, Sep 10, 2021 at 6:54 PM Pablo de Lara
> <pablo.de.lara.guarch@intel.com> wrote:
>>
>> rte_efd_create() function was using uint8_t for a socket bitmask,
>> for one of its parameters.
>> This limits the maximum of NUMA sockets to be 8.
>> Changing to to uint64_t increases it to 64, which should be
>> more future-proof.
> 
> Cc: ppc maintainer, since I think powerX servers have non contiguous
> NUMA sockets.
> 
> 
>>
>> Coverity issue: 366390
>> Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
>>
>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>> ---
>>
>> This fix requires an API breakage and therefore it is not
>> a good candidate for backporting (besides, it is a very low impact bug).
>> Hence, I am not CC'ing stable.
> 
> This is an unannounced breakage for a stable API.
> Cc: techboard + Ray for awareness.

Understood.
Its low impact, at a time we are changing the ABI in any case.

> 
> 
>>
>> ---
>>
>>  lib/efd/rte_efd.c | 2 +-
>>  lib/efd/rte_efd.h | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
>> index 77f46809f8..68a2378e88 100644
>> --- a/lib/efd/rte_efd.c
>> +++ b/lib/efd/rte_efd.c
>> @@ -495,7 +495,7 @@ efd_search_hash(struct rte_efd_table * const table,
>>
>>  struct rte_efd_table *
>>  rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
>> -               uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
>> +               uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
>>  {
>>         struct rte_efd_table *table = NULL;
>>         uint8_t *key_array = NULL;
>> diff --git a/lib/efd/rte_efd.h b/lib/efd/rte_efd.h
>> index c2be4c09ae..d3d7befd0c 100644
>> --- a/lib/efd/rte_efd.h
>> +++ b/lib/efd/rte_efd.h
>> @@ -139,7 +139,7 @@ typedef uint16_t efd_hashfunc_t;
>>   */
>>  struct rte_efd_table *
>>  rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
>> -       uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
>> +       uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
>>
>>  /**
>>   * Releases the resources from an EFD table
>> --
>> 2.25.1
>>
> 
> 

Re: [dpdk-dev] [PATCH] efd: change data type of parameter

[David Christensen]

On 9/14/21 12:10 AM, David Marchand wrote:
> On Fri, Sep 10, 2021 at 6:54 PM Pablo de Lara
> <pablo.de.lara.guarch@intel.com> wrote:
>>
>> rte_efd_create() function was using uint8_t for a socket bitmask,
>> for one of its parameters.
>> This limits the maximum of NUMA sockets to be 8.
>> Changing to to uint64_t increases it to 64, which should be
>> more future-proof.
> 
> Cc: ppc maintainer, since I think powerX servers have non contiguous
> NUMA sockets.

Definitely correct, POWER CPU NUMA sockets are not necessarily contiguous.

Can you update efd_autotest and efd_perf_autotest as well?  After 
applying this patch the test still fails on my POWER9 system:

$ sudo /home/drc/src/dpdk/build/app/test/dpdk-test -l 64-127 -n 4 --no-pci
...
RTE>>efd_autotest
Entering test_add_delete
EFD: At least one CPU socket must be enabled in the bitmask
EAL: Test assert test_add_delete line 125 failed: Error creating the EFD 
table

Test Failed
RTE>>

On this system lcores 64-127 reside on NUMA socket 8.

Dave

[dpdk-dev] [PATCH v3] efd: change data type of parameter

[Pablo de Lara]

rte_efd_create() function was using uint8_t for a socket bitmask,
for one of its parameters.
This limits the maximum of NUMA sockets to be 8.
Changing to uint64_t increases it to 64, which should be
more future-proof.

Coverity issue: 366390
Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: John McNamara <john.mcnamara@intel.com>
---

v3: Fixed commit message

v2: Fixed EFD tests

---

 app/test/test_efd.c      | 4 ++--
 app/test/test_efd_perf.c | 4 ++--
 lib/efd/rte_efd.c        | 2 +-
 lib/efd/rte_efd.h        | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/test/test_efd.c b/app/test/test_efd.c
index 180dc4748e..581519c1e0 100644
--- a/app/test/test_efd.c
+++ b/app/test/test_efd.c
@@ -91,9 +91,9 @@ static struct flow_key keys[5] = {
 /* Array to store the data */
 static efd_value_t data[5];
 
-static inline uint8_t efd_get_all_sockets_bitmask(void)
+static inline uint64_t efd_get_all_sockets_bitmask(void)
 {
-	uint8_t all_cpu_sockets_bitmask = 0;
+	uint64_t all_cpu_sockets_bitmask = 0;
 	unsigned int i;
 	unsigned int next_lcore = rte_get_main_lcore();
 	const int val_true = 1, val_false = 0;
diff --git a/app/test/test_efd_perf.c b/app/test/test_efd_perf.c
index 1c47704475..f3fe3b1736 100644
--- a/app/test/test_efd_perf.c
+++ b/app/test/test_efd_perf.c
@@ -29,9 +29,9 @@
 #endif
 static unsigned int test_socket_id;
 
-static inline uint8_t efd_get_all_sockets_bitmask(void)
+static inline uint64_t efd_get_all_sockets_bitmask(void)
 {
-	uint8_t all_cpu_sockets_bitmask = 0;
+	uint64_t all_cpu_sockets_bitmask = 0;
 	unsigned int i;
 	unsigned int next_lcore = rte_get_main_lcore();
 	const int val_true = 1, val_false = 0;
diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
index 77f46809f8..68a2378e88 100644
--- a/lib/efd/rte_efd.c
+++ b/lib/efd/rte_efd.c
@@ -495,7 +495,7 @@ efd_search_hash(struct rte_efd_table * const table,
 
 struct rte_efd_table *
 rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
-		uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
+		uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
 {
 	struct rte_efd_table *table = NULL;
 	uint8_t *key_array = NULL;
diff --git a/lib/efd/rte_efd.h b/lib/efd/rte_efd.h
index c2be4c09ae..d3d7befd0c 100644
--- a/lib/efd/rte_efd.h
+++ b/lib/efd/rte_efd.h
@@ -139,7 +139,7 @@ typedef uint16_t efd_hashfunc_t;
  */
 struct rte_efd_table *
 rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
-	uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
+	uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
 
 /**
  * Releases the resources from an EFD table
-- 
2.25.1

Re: [dpdk-dev] [PATCH v3] efd: change data type of parameter

[David Christensen]

On 9/17/21 5:56 AM, Pablo de Lara wrote:
> rte_efd_create() function was using uint8_t for a socket bitmask,
> for one of its parameters.
> This limits the maximum of NUMA sockets to be 8.
> Changing to uint64_t increases it to 64, which should be
> more future-proof.
> 
> Coverity issue: 366390
> Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>
> ---
> 
> v3: Fixed commit message
> 
> v2: Fixed EFD tests
> 
> ---
> 
>   app/test/test_efd.c      | 4 ++--
>   app/test/test_efd_perf.c | 4 ++--
>   lib/efd/rte_efd.c        | 2 +-
>   lib/efd/rte_efd.h        | 2 +-
>   4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test/test_efd.c b/app/test/test_efd.c
> index 180dc4748e..581519c1e0 100644
> --- a/app/test/test_efd.c
> +++ b/app/test/test_efd.c
> @@ -91,9 +91,9 @@ static struct flow_key keys[5] = {
>   /* Array to store the data */
>   static efd_value_t data[5];
> 
> -static inline uint8_t efd_get_all_sockets_bitmask(void)
> +static inline uint64_t efd_get_all_sockets_bitmask(void)
>   {
> -	uint8_t all_cpu_sockets_bitmask = 0;
> +	uint64_t all_cpu_sockets_bitmask = 0;
>   	unsigned int i;
>   	unsigned int next_lcore = rte_get_main_lcore();
>   	const int val_true = 1, val_false = 0;
> diff --git a/app/test/test_efd_perf.c b/app/test/test_efd_perf.c
> index 1c47704475..f3fe3b1736 100644
> --- a/app/test/test_efd_perf.c
> +++ b/app/test/test_efd_perf.c
> @@ -29,9 +29,9 @@
>   #endif
>   static unsigned int test_socket_id;
> 
> -static inline uint8_t efd_get_all_sockets_bitmask(void)
> +static inline uint64_t efd_get_all_sockets_bitmask(void)
>   {
> -	uint8_t all_cpu_sockets_bitmask = 0;
> +	uint64_t all_cpu_sockets_bitmask = 0;
>   	unsigned int i;
>   	unsigned int next_lcore = rte_get_main_lcore();
>   	const int val_true = 1, val_false = 0;
> diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
> index 77f46809f8..68a2378e88 100644
> --- a/lib/efd/rte_efd.c
> +++ b/lib/efd/rte_efd.c
> @@ -495,7 +495,7 @@ efd_search_hash(struct rte_efd_table * const table,
> 
>   struct rte_efd_table *
>   rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
> -		uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
> +		uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
>   {
>   	struct rte_efd_table *table = NULL;
>   	uint8_t *key_array = NULL;
> diff --git a/lib/efd/rte_efd.h b/lib/efd/rte_efd.h
> index c2be4c09ae..d3d7befd0c 100644
> --- a/lib/efd/rte_efd.h
> +++ b/lib/efd/rte_efd.h
> @@ -139,7 +139,7 @@ typedef uint16_t efd_hashfunc_t;
>    */
>   struct rte_efd_table *
>   rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
> -	uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
> +	uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
> 
>   /**
>    * Releases the resources from an EFD table
> 

After applying the patch I receive a segmentation fault when I use 
lcores on the second NUMA node (node 8).

$ lscpu
Architecture:        ppc64le
..
NUMA node0 CPU(s):   0-63
NUMA node8 CPU(s):   64-127

Working case:
---------------------------
$ sudo /home/drc/src/dpdk/build/app/test/dpdk-test -l 59-63 -n 4 --no-pci
EAL: Detected 128 lcore(s)
EAL: Detected 2 NUMA nodes
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: No available 1048576 kB hugepages reported
EAL: VFIO support initialized
APP: HPET is not enabled, using TSC as default timer
RTE>>efd_autotest
Entering test_add_delete
Entering test_efd_find_existing
Entering test_add_update_delete
Entering test_five_keys
Entering test_efd_creation_with_bad_parameters, **Errors are expected **
EFD: Allocating key array on socket 0 failed
EFD: At least one CPU socket must be enabled in the bitmask
EFD: Allocating EFD table management structure on socket 255 failed
# Test successful. No more errors expected
Evaluating table utilization and correctness, please wait
Added    2097152	Succeeded    2097152	Lost          0
Added    2097152	Succeeded    2097152	Lost          0
Added    2097152	Succeeded    2097152	Lost          0

Average table utilization = 100.00% (2097152/2097152)
Test OK
RTE>>quit

Failing case:
---------------------------
sudo /home/drc/src/dpdk/build/app/test/dpdk-test -l 64-69 -n 4 --no-pci
EAL: Detected 128 lcore(s)
EAL: Detected 2 NUMA nodes
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: No available 1048576 kB hugepages reported
EAL: VFIO support initialized
APP: HPET is not enabled, using TSC as default timer
RTE>>efd_autotest
Entering test_add_delete
Segmentation fault

What's the purpose of "test_socket_id" in the file app/test/test_efd.c? 
  I don't see it set during the test, default to 0, and it looks like it 
should be 8 in this situation:

sudo gdb --args /home/drc/src/dpdk/build/app/test/dpdk-test -l 64-69 -n 
4 --no-pci
GNU gdb (GDB) Red Hat Enterprise Linux 8.2-12.el8
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "ppc64le-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
     <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /home/drc/src/dpdk/build/app/test/dpdk-test...done.
(gdb) b test_and_delete
Function "test_and_delete" not defined.
Make breakpoint pending on future shared library load? (y or [n]) n
(gdb) b efd_get_choice
Breakpoint 1 at 0x1090dbdc: file ../lib/efd/rte_efd.c, line 319.
(gdb) b test_add_delete
Breakpoint 2 at 0x1009d7a8: test_add_delete. (2 locations)
(gdb) start
Temporary breakpoint 3 at 0x1000f340: file ../app/test/test.c, line 105.
Starting program: /home/drc/src/dpdk/build/app/test/dpdk-test -l 64-69 
-n 4 --no-pci
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/power9/libthread_db.so.1".

Temporary breakpoint 3, main (argc=6, argv=0x7ffffffff138) at 
../app/test/test.c:105
105	{
Missing separate debuginfos, use: yum debuginfo-install 
libibverbs-52mlnx1-1.53100.ppc64le
(gdb) c
Continuing.
EAL: Detected 128 lcore(s)
EAL: Detected 2 NUMA nodes
EAL: Detected static linkage of DPDK
[New Thread 0x7ffff746d090 (LWP 290647)]
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
[New Thread 0x7ffff6c5d090 (LWP 290648)]
EAL: Selected IOVA mode 'VA'
EAL: No available 1048576 kB hugepages reported
EAL: VFIO support initialized
[New Thread 0x7ffff644d090 (LWP 290649)]
[New Thread 0x7ffff5c3d090 (LWP 290650)]
[New Thread 0x7ffff542d090 (LWP 290651)]
[New Thread 0x7ffff4c1d090 (LWP 290652)]
[New Thread 0x7ffff440d090 (LWP 290653)]
[New Thread 0x7ffff3bfd090 (LWP 290654)]
[New Thread 0x7ffff33ed090 (LWP 290655)]
APP: HPET is not enabled, using TSC as default timer
RTE>>efd_autotest

Thread 1 "dpdk-test" hit Breakpoint 2, test_add_delete () at 
../app/test/test_efd.c:120
120		printf("Entering %s\n", __func__);
(gdb) bt
#0  test_add_delete () at ../app/test/test_efd.c:120
#1  0x000000001009ec54 in test_efd () at ../app/test/test_efd.c:448
#2  0x0000000010009b4c in cmd_autotest_parsed 
(parsed_result=0x7fffffff8620, cl=0x200196d0, data=0x0) at 
../app/test/commands.c:71
#3  0x000000001095a808 in cmdline_parse (cl=0x200196d0, buf=0x20019718 
"efd_autotest\n") at ../lib/cmdline/cmdline_parse.c:290
#4  0x0000000010957c90 in cmdline_valid_buffer (rdl=0x200196e0, 
buf=0x20019718 "efd_autotest\n", size=14)
     at ../lib/cmdline/cmdline.c:26
#5  0x000000001095e8a8 in rdline_char_in (rdl=0x200196e0, c=10 '\n') at 
../lib/cmdline/cmdline_rdline.c:421
#6  0x0000000010958258 in cmdline_in (cl=0x200196d0, buf=0x7fffffffe800 
"\n\226\001 ", size=1) at ../lib/cmdline/cmdline.c:149
#7  0x00000000109585f0 in cmdline_interact (cl=0x200196d0) at 
../lib/cmdline/cmdline.c:223
#8  0x000000001000f988 in main (argc=1, argv=0x7ffffffff160) at 
../app/test/test.c:234
(gdb) n
Entering test_add_delete
122		handle = rte_efd_create("test_add_delete",
(gdb) n
125		TEST_ASSERT_NOT_NULL(handle, "Error creating the EFD table\n");
(gdb) n
127		data[0] = mrand48() & VALUE_BITMASK;
(gdb) n
128		TEST_ASSERT_SUCCESS(rte_efd_update(handle, test_socket_id, &keys[0],
(gdb) p test_socket_id
$1 = 0
(gdb) c
Continuing.

Thread 1 "dpdk-test" hit Breakpoint 1, efd_get_choice 
(table=0x1003fff00, socket_id=0, chunk_id=218, bin_id=120)
     at ../lib/efd/rte_efd.c:319
319		struct efd_online_chunk *chunk = &table->chunks[socket_id][chunk_id];
(gdb) p table
$2 = (const struct rte_efd_table * const) 0x1003fff00
(gdb) p *table
$3 = {name = "test_add_delete", '\000' <repeats 16 times>, key_len = 13, 
max_num_rules = 3670016, num_rules = 0,
   num_chunks = 2048, num_chunks_shift = 11, lookup_fn = 
EFD_LOOKUP_SCALAR, chunks = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
     0x11011dff00, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, offline_chunks = 
0x10327e000, free_slots = 0x1049ffd00,
   keys = 0x100480000 ""}
(gdb) p *table->chunk[0]
There is no member named chunk.
(gdb) p *table->chunks[0]
Cannot access memory at address 0x0
(gdb) p *table->chunks[8]
$4 = {bin_choice_list = '\000' <repeats 63 times>, groups = {{hash_idx = 
{0, 0, 0, 0, 0, 0, 0, 0}, lookup_table = {0, 0, 0, 0, 0,
         0, 0, 0}} <repeats 64 times>}}
(gdb) c
Continuing.

Thread 1 "dpdk-test" received signal SIGSEGV, Segmentation fault.
0x000000001090dc18 in efd_get_choice (table=0x1003fff00, socket_id=0, 
chunk_id=218, bin_id=120) at ../lib/efd/rte_efd.c:325
325		uint8_t choice_chunk =
(gdb) p chunk
$5 = (struct efd_online_chunk *) 0x70680
(gdb) p *chunk
Cannot access memory at address 0x70680
(gdb)

Not clear to me if this is an existing problem with POWER or related to 
your patch.  Can someone more familiar with EFD chime in?

Dave

Re: [dpdk-dev] [PATCH v3] efd: change data type of parameter

[De Lara Guarch, Pablo]

Hi David,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of David Christensen
> Sent: Monday, September 20, 2021 8:31 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Wang, Yipeng1
> <yipeng1.wang@intel.com>; Marohn, Byron <byron.marohn@intel.com>
> Cc: dev@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3] efd: change data type of parameter
> 
> 
> 
> On 9/17/21 5:56 AM, Pablo de Lara wrote:
> > rte_efd_create() function was using uint8_t for a socket bitmask, for
> > one of its parameters.
> > This limits the maximum of NUMA sockets to be 8.
> > Changing to uint64_t increases it to 64, which should be more
> > future-proof.
> >
> > Coverity issue: 366390
> > Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > Acked-by: John McNamara <john.mcnamara@intel.com>
> > ---
> >
> > v3: Fixed commit message
> >
> > v2: Fixed EFD tests
> >

Could you check version 4 of this patch?

Thanks,
Pablo

[dpdk-dev] [PATCH v4] efd: change data type of parameter

[Pablo de Lara]

rte_efd_create() function was using uint8_t for a socket bitmask,
for one of its parameters.
This limits the maximum of NUMA sockets to be 8.
Changing to to uint64_t increases it to 64, which should be
more future-proof.

Coverity issue: 366390
Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: John McNamara <john.mcnamara@intel.com>
---

v4: Set socket id in EFD tests

v3: Fixed commit message

v2: Fixed EFD tests

---

 app/test/test_efd.c      | 5 +++--
 app/test/test_efd_perf.c | 4 ++--
 lib/efd/rte_efd.c        | 2 +-
 lib/efd/rte_efd.h        | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/app/test/test_efd.c b/app/test/test_efd.c
index 180dc4748e..1b249e0447 100644
--- a/app/test/test_efd.c
+++ b/app/test/test_efd.c
@@ -91,9 +91,9 @@ static struct flow_key keys[5] = {
 /* Array to store the data */
 static efd_value_t data[5];
 
-static inline uint8_t efd_get_all_sockets_bitmask(void)
+static inline uint64_t efd_get_all_sockets_bitmask(void)
 {
-	uint8_t all_cpu_sockets_bitmask = 0;
+	uint64_t all_cpu_sockets_bitmask = 0;
 	unsigned int i;
 	unsigned int next_lcore = rte_get_main_lcore();
 	const int val_true = 1, val_false = 0;
@@ -443,6 +443,7 @@ static int test_efd_creation_with_bad_parameters(void)
 static int
 test_efd(void)
 {
+	test_socket_id = rte_socket_id();
 
 	/* Unit tests */
 	if (test_add_delete() < 0)
diff --git a/app/test/test_efd_perf.c b/app/test/test_efd_perf.c
index 1c47704475..f3fe3b1736 100644
--- a/app/test/test_efd_perf.c
+++ b/app/test/test_efd_perf.c
@@ -29,9 +29,9 @@
 #endif
 static unsigned int test_socket_id;
 
-static inline uint8_t efd_get_all_sockets_bitmask(void)
+static inline uint64_t efd_get_all_sockets_bitmask(void)
 {
-	uint8_t all_cpu_sockets_bitmask = 0;
+	uint64_t all_cpu_sockets_bitmask = 0;
 	unsigned int i;
 	unsigned int next_lcore = rte_get_main_lcore();
 	const int val_true = 1, val_false = 0;
diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
index 77f46809f8..68a2378e88 100644
--- a/lib/efd/rte_efd.c
+++ b/lib/efd/rte_efd.c
@@ -495,7 +495,7 @@ efd_search_hash(struct rte_efd_table * const table,
 
 struct rte_efd_table *
 rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
-		uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
+		uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket)
 {
 	struct rte_efd_table *table = NULL;
 	uint8_t *key_array = NULL;
diff --git a/lib/efd/rte_efd.h b/lib/efd/rte_efd.h
index c2be4c09ae..d3d7befd0c 100644
--- a/lib/efd/rte_efd.h
+++ b/lib/efd/rte_efd.h
@@ -139,7 +139,7 @@ typedef uint16_t efd_hashfunc_t;
  */
 struct rte_efd_table *
 rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
-	uint8_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
+	uint64_t online_cpu_socket_bitmask, uint8_t offline_cpu_socket);
 
 /**
  * Releases the resources from an EFD table
-- 
2.25.1

Re: [dpdk-dev] [PATCH v4] efd: change data type of parameter

[David Christensen]

On 9/28/21 6:58 AM, Pablo de Lara wrote:
> rte_efd_create() function was using uint8_t for a socket bitmask,
> for one of its parameters.
> This limits the maximum of NUMA sockets to be 8.
> Changing to to uint64_t increases it to 64, which should be
> more future-proof.
> 
> Coverity issue: 366390
> Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>
> ---
> 
> v4: Set socket id in EFD tests
> 
> v3: Fixed commit message
> 
> v2: Fixed EFD tests

Results with v4 on a non-consecutive NUMA system:

$ lscpu
Architecture:        ppc64le
Byte Order:          Little Endian
CPU(s):              128
On-line CPU(s) list: 0-127
Thread(s) per core:  4
Core(s) per socket:  16
Socket(s):           2
NUMA node(s):        6
Model:               2.3 (pvr 004e 1203)
Model name:          POWER9, altivec supported
CPU max MHz:         3800.0000
CPU min MHz:         2300.0000
L1d cache:           32K
L1i cache:           32K
L2 cache:            512K
L3 cache:            10240K
NUMA node0 CPU(s):   0-63
NUMA node8 CPU(s):   64-127
NUMA node252 CPU(s):
NUMA node253 CPU(s):
NUMA node254 CPU(s):
NUMA node255 CPU(s):

$ sudo /home/drc/src/dpdk/build/app/test/dpdk-test -l 64-127 -n 4 --no-pci
EAL: Detected 128 lcore(s)
EAL: Detected 2 NUMA nodes
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: No available 1048576 kB hugepages reported
EAL: VFIO support initialized
APP: HPET is not enabled, using TSC as default timer
RTE>>efd_autotest
Entering test_add_delete
Entering test_efd_find_existing
Entering test_add_update_delete
Entering test_five_keys
Entering test_efd_creation_with_bad_parameters, **Errors are expected **
EFD: Allocating key array on socket 8 failed
EFD: At least one CPU socket must be enabled in the bitmask
EFD: Allocating EFD table management structure on socket 255 failed
# Test successful. No more errors expected
Evaluating table utilization and correctness, please wait
Added    2097152	Succeeded    2097152	Lost          0
Added    2097152	Succeeded    2097152	Lost          0
Added    2097152	Succeeded    2097152	Lost          0

Average table utilization = 100.00% (2097152/2097152)
Test OK
RTE>>efd_perf_autotest
Measuring performance, please wait
..........
Results (in CPU cycles/operation)
-----------------------------------

Keysize           Add               Lookup            Lookup_bulk 
Delete
4                 471               17                11                59
8                 502               18                14                73
16                542               23                21                81
32                662               37                36                97
48                782               54                53                110
64                881               75                74                119
9                 512               18                16                75
13                539               23                22                84
37                726               48                47                115
40                726               47                46                112
Test OK

Re: [dpdk-dev] [PATCH v4] efd: change data type of parameter

[De Lara Guarch, Pablo]

Hi

> -----Original Message-----
> From: David Christensen <drc@linux.vnet.ibm.com>
> Sent: Tuesday, September 28, 2021 4:53 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Wang, Yipeng1
> <yipeng1.wang@intel.com>; Marohn, Byron <byron.marohn@intel.com>
> Cc: dev@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>
> Subject: Re: [PATCH v4] efd: change data type of parameter
> 
> 
> 
> On 9/28/21 6:58 AM, Pablo de Lara wrote:
> > rte_efd_create() function was using uint8_t for a socket bitmask, for
> > one of its parameters.
> > This limits the maximum of NUMA sockets to be 8.
> > Changing to to uint64_t increases it to 64, which should be more
> > future-proof.
> >
> > Coverity issue: 366390
> > Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > Acked-by: John McNamara <john.mcnamara@intel.com>
> > ---
> >
> > v4: Set socket id in EFD tests
> >
> > v3: Fixed commit message
> >
> > v2: Fixed EFD tests
> 
> Results with v4 on a non-consecutive NUMA system:

...

> Test OK

Great! Thanks a lot for checking. Would you mind adding tested-by to the patch?

Pablo

Re: [dpdk-dev] [PATCH v4] efd: change data type of parameter

[David Christensen]

On 9/29/21 12:51 AM, De Lara Guarch, Pablo wrote:
> Hi
> 
>> -----Original Message-----
>> From: David Christensen <drc@linux.vnet.ibm.com>
>> Sent: Tuesday, September 28, 2021 4:53 PM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Wang, Yipeng1
>> <yipeng1.wang@intel.com>; Marohn, Byron <byron.marohn@intel.com>
>> Cc: dev@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>
>> Subject: Re: [PATCH v4] efd: change data type of parameter
>>
>>
>>
>> On 9/28/21 6:58 AM, Pablo de Lara wrote:
>>> rte_efd_create() function was using uint8_t for a socket bitmask, for
>>> one of its parameters.
>>> This limits the maximum of NUMA sockets to be 8.
>>> Changing to to uint64_t increases it to 64, which should be more
>>> future-proof.
>>>
>>> Coverity issue: 366390
>>> Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
>>>
>>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>>> Acked-by: John McNamara <john.mcnamara@intel.com>
>>> ---
>>>
>>> v4: Set socket id in EFD tests
>>>
>>> v3: Fixed commit message
>>>
>>> v2: Fixed EFD tests
>>
>> Results with v4 on a non-consecutive NUMA system:
> 
> ...
> 
>> Test OK
> 
> Great! Thanks a lot for checking. Would you mind adding tested-by to the patch?
> 
> Pablo
> 

Tested-by: David Christensen <drc@linux.vnet.ibm.com>

Re: [dpdk-dev] [PATCH v4] efd: change data type of parameter

[Wang, Yipeng1]

> -----Original Message-----
> From: David Christensen <drc@linux.vnet.ibm.com>
> Sent: Tuesday, September 28, 2021 8:53 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Wang, Yipeng1
> <yipeng1.wang@intel.com>; Marohn, Byron <byron.marohn@intel.com>
> Cc: dev@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>
> Subject: Re: [PATCH v4] efd: change data type of parameter
> 
> 
> 
> On 9/28/21 6:58 AM, Pablo de Lara wrote:
> > rte_efd_create() function was using uint8_t for a socket bitmask, for
> > one of its parameters.
> > This limits the maximum of NUMA sockets to be 8.
> > Changing to to uint64_t increases it to 64, which should be more
> > future-proof.
> >
> > Coverity issue: 366390
> > Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > Acked-by: John McNamara <john.mcnamara@intel.com>
> > ---
> >
> > v4: Set socket id in EFD tests
> >
> > v3: Fixed commit message
> >
> > v2: Fixed EFD tests
>
[Wang, Yipeng] 
Acked-by: Yipeng Wang <yipeng1.wang@intel.com>

Thanks Pablo!

Re: [dpdk-dev] [PATCH v4] efd: change data type of parameter

[Thomas Monjalon]

> > > rte_efd_create() function was using uint8_t for a socket bitmask, for
> > > one of its parameters.
> > > This limits the maximum of NUMA sockets to be 8.
> > > Changing to to uint64_t increases it to 64, which should be more
> > > future-proof.
> > >
> > > Coverity issue: 366390
> > > Fixes: 56b6ef874f8 ("efd: new Elastic Flow Distributor library")
> > >
> > > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > > Acked-by: John McNamara <john.mcnamara@intel.com>
> Acked-by: Yipeng Wang <yipeng1.wang@intel.com>

Applied, thanks.