diff mbox series

[v4] efd: change data type of parameter

Message ID 20210928135839.3778960-1-pablo.de.lara.guarch@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers show
Series [v4] efd: change data type of parameter | expand

Checks

Context Check Description
ci/iol-testing warning apply patch failure
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/checkpatch warning coding style issues

Commit Message

Pablo de Lara Sept. 28, 2021, 1:58 p.m. UTC
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(-)

Comments

David Christensen Sept. 28, 2021, 3:52 p.m. UTC | #1
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
Pablo de Lara Sept. 29, 2021, 7:51 a.m. UTC | #2
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
David Christensen Sept. 29, 2021, 5:41 p.m. UTC | #3
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>
Wang, Yipeng1 Sept. 29, 2021, 6:13 p.m. UTC | #4
> -----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!
Thomas Monjalon Oct. 1, 2021, 2:34 p.m. UTC | #5
> > > 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.
diff mbox series

Patch

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