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