* RE: [PATCH] test/bitops: check worker lcore availability
2024-10-11 15:25 [PATCH] test/bitops: check worker lcore availability David Marchand
@ 2024-10-11 15:27 ` Morten Brørup
2024-10-13 6:53 ` Mattias Rönnblom
2024-10-25 7:39 ` [PATCH v2] " David Marchand
2 siblings, 0 replies; 8+ messages in thread
From: Morten Brørup @ 2024-10-11 15:27 UTC (permalink / raw)
To: David Marchand, dev
Cc: Jack Bond-Preston, Mattias Rönnblom, Tyler Retzlaff
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Friday, 11 October 2024 17.26
>
> Coverity is not able to understand that having 2 lcores means that
> rte_get_next_lcore(-1, 0, 1) can't return RTE_MAX_LCORE.
> Add an assert.
>
> Coverity issue: 445382, 445383, 445384, 445387, 445389, 445391
> Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Note:
> - a better fix would be to check lcore id validity in the EAL launch
> API,
> but it requires inspecting all functions and it could result in some
> API change, so sending this as a simple fix for now,
>
> ---
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] test/bitops: check worker lcore availability
2024-10-11 15:25 [PATCH] test/bitops: check worker lcore availability David Marchand
2024-10-11 15:27 ` Morten Brørup
@ 2024-10-13 6:53 ` Mattias Rönnblom
2024-10-14 14:26 ` David Marchand
2024-10-25 7:39 ` [PATCH v2] " David Marchand
2 siblings, 1 reply; 8+ messages in thread
From: Mattias Rönnblom @ 2024-10-13 6:53 UTC (permalink / raw)
To: David Marchand, dev
Cc: Jack Bond-Preston, Morten Brørup, Mattias Rönnblom,
Tyler Retzlaff
On 2024-10-11 17:25, David Marchand wrote:
> Coverity is not able to understand that having 2 lcores means that
> rte_get_next_lcore(-1, 0, 1) can't return RTE_MAX_LCORE.
> Add an assert.
>
> Coverity issue: 445382, 445383, 445384, 445387, 445389, 445391
> Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Note:
> - a better fix would be to check lcore id validity in the EAL launch API,
> but it requires inspecting all functions and it could result in some
> API change, so sending this as a simple fix for now,
>
> ---
> app/test/test_bitops.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/app/test/test_bitops.c b/app/test/test_bitops.c
> index 4200073ae4..4ed54709fb 100644
> --- a/app/test/test_bitops.c
> +++ b/app/test/test_bitops.c
> @@ -159,6 +159,7 @@ test_bit_atomic_parallel_assign ## size(void) \
> return TEST_SKIPPED; \
> } \
> worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> + TEST_ASSERT(worker_lcore_id < RTE_MAX_LCORE, "Failed to find a worker lcore"); \
How about:
static unsigned int
get_worker_lcore(void)
{
unsigned int lcore_id;
lcore_id = rte_get_next_lcore(-1, 1, 0);
/* avoid Coverity false positives */
RTE_VERIFY(lcore_id < RTE_MAX_LCORE);
return lcore_id;
}
In the macros:
worker_lcore_id = get_worker_lcore(-1, 1, 0);
Makes the macros a tiny bit smaller/less redundant and gives an
opportunity for a comment. Also, it's more appropriate to use RTE_VERIFY
I would argue, since rte_get_next_lcore() is not the SUT.
> lmain.bit = rte_rand_max(size); \
> do { \
> lworker.bit = rte_rand_max(size); \
> @@ -218,6 +219,7 @@ test_bit_atomic_parallel_test_and_modify ## size(void) \
> return TEST_SKIPPED; \
> } \
> worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> + TEST_ASSERT(worker_lcore_id < RTE_MAX_LCORE, "Failed to find a worker lcore"); \
> int rc = rte_eal_remote_launch(run_parallel_test_and_modify ## size, &lworker, \
> worker_lcore_id); \
> TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
> @@ -267,6 +269,7 @@ test_bit_atomic_parallel_flip ## size(void) \
> return TEST_SKIPPED; \
> } \
> worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> + TEST_ASSERT(worker_lcore_id < RTE_MAX_LCORE, "Failed to find a worker lcore"); \
> int rc = rte_eal_remote_launch(run_parallel_flip ## size, &lworker, worker_lcore_id); \
> TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
> run_parallel_flip ## size(&lmain); \
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] test/bitops: check worker lcore availability
2024-10-13 6:53 ` Mattias Rönnblom
@ 2024-10-14 14:26 ` David Marchand
0 siblings, 0 replies; 8+ messages in thread
From: David Marchand @ 2024-10-14 14:26 UTC (permalink / raw)
To: Mattias Rönnblom
Cc: dev, Jack Bond-Preston, Morten Brørup,
Mattias Rönnblom, Tyler Retzlaff
On Sun, Oct 13, 2024 at 8:53 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2024-10-11 17:25, David Marchand wrote:
> > Coverity is not able to understand that having 2 lcores means that
> > rte_get_next_lcore(-1, 0, 1) can't return RTE_MAX_LCORE.
> > Add an assert.
> >
> > Coverity issue: 445382, 445383, 445384, 445387, 445389, 445391
> > Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > Note:
> > - a better fix would be to check lcore id validity in the EAL launch API,
> > but it requires inspecting all functions and it could result in some
> > API change, so sending this as a simple fix for now,
> >
> > ---
> > app/test/test_bitops.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/app/test/test_bitops.c b/app/test/test_bitops.c
> > index 4200073ae4..4ed54709fb 100644
> > --- a/app/test/test_bitops.c
> > +++ b/app/test/test_bitops.c
> > @@ -159,6 +159,7 @@ test_bit_atomic_parallel_assign ## size(void) \
> > return TEST_SKIPPED; \
> > } \
> > worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> > + TEST_ASSERT(worker_lcore_id < RTE_MAX_LCORE, "Failed to find a worker lcore"); \
>
> How about:
>
> static unsigned int
> get_worker_lcore(void)
> {
> unsigned int lcore_id;
>
> lcore_id = rte_get_next_lcore(-1, 1, 0);
>
> /* avoid Coverity false positives */
> RTE_VERIFY(lcore_id < RTE_MAX_LCORE);
>
> return lcore_id;
> }
>
> In the macros:
> worker_lcore_id = get_worker_lcore(-1, 1, 0);
>
> Makes the macros a tiny bit smaller/less redundant and gives an
> opportunity for a comment. Also, it's more appropriate to use RTE_VERIFY
> I would argue, since rte_get_next_lcore() is not the SUT.
I agree on the principle.
I will send a new one when possible.
--
David Marchand
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] test/bitops: check worker lcore availability
2024-10-11 15:25 [PATCH] test/bitops: check worker lcore availability David Marchand
2024-10-11 15:27 ` Morten Brørup
2024-10-13 6:53 ` Mattias Rönnblom
@ 2024-10-25 7:39 ` David Marchand
2024-10-25 9:22 ` Morten Brørup
` (2 more replies)
2 siblings, 3 replies; 8+ messages in thread
From: David Marchand @ 2024-10-25 7:39 UTC (permalink / raw)
To: dev
Cc: Jack Bond-Preston, Mattias Rönnblom, Morten Brørup,
Tyler Retzlaff
Coverity is not able to understand that having 2 lcores means that
rte_get_next_lcore(-1, 0, 1) can't return RTE_MAX_LCORE.
Add a check.
Coverity issue: 445382, 445383, 445384, 445387, 445389, 445391
Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- added a helper,
- prefer RTE_VERIFY(),
Note:
- a better fix would be to check lcore id validity in the EAL launch API,
but it requires inspecting all functions and it could result in some
API change, so sending this as a simple fix for now,
---
app/test/test_bitops.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/app/test/test_bitops.c b/app/test/test_bitops.c
index 681e984037..78a7df6bb1 100644
--- a/app/test/test_bitops.c
+++ b/app/test/test_bitops.c
@@ -13,6 +13,17 @@
#include <rte_random.h>
#include "test.h"
+static unsigned int
+get_worker_lcore(void)
+{
+ unsigned int lcore_id = rte_get_next_lcore(-1, 1, 0);
+
+ /* avoid checkers (like Coverity) false positives */
+ RTE_VERIFY(lcore_id < RTE_MAX_LCORE);
+
+ return lcore_id;
+}
+
#define GEN_TEST_BIT_ACCESS(test_name, set_fun, clear_fun, assign_fun, flip_fun, test_fun, size, \
mod) \
static int \
@@ -158,7 +169,7 @@ test_bit_atomic_parallel_assign ## size(void) \
printf("Need multiple cores to run parallel test.\n"); \
return TEST_SKIPPED; \
} \
- worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
+ worker_lcore_id = get_worker_lcore(); \
lmain.bit = rte_rand_max(size); \
do { \
lworker.bit = rte_rand_max(size); \
@@ -217,7 +228,7 @@ test_bit_atomic_parallel_test_and_modify ## size(void) \
printf("Need multiple cores to run parallel test.\n"); \
return TEST_SKIPPED; \
} \
- worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
+ worker_lcore_id = get_worker_lcore(); \
int rc = rte_eal_remote_launch(run_parallel_test_and_modify ## size, &lworker, \
worker_lcore_id); \
TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
@@ -266,7 +277,7 @@ test_bit_atomic_parallel_flip ## size(void) \
printf("Need multiple cores to run parallel test.\n"); \
return TEST_SKIPPED; \
} \
- worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
+ worker_lcore_id = get_worker_lcore(); \
int rc = rte_eal_remote_launch(run_parallel_flip ## size, &lworker, worker_lcore_id); \
TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
run_parallel_flip ## size(&lmain); \
--
2.46.2
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v2] test/bitops: check worker lcore availability
2024-10-25 7:39 ` [PATCH v2] " David Marchand
@ 2024-10-25 9:22 ` Morten Brørup
2024-10-25 9:46 ` Jack Bond-Preston
2024-10-30 8:26 ` David Marchand
2 siblings, 0 replies; 8+ messages in thread
From: Morten Brørup @ 2024-10-25 9:22 UTC (permalink / raw)
To: David Marchand, dev
Cc: Jack Bond-Preston, Mattias Rönnblom, Tyler Retzlaff
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] test/bitops: check worker lcore availability
2024-10-25 7:39 ` [PATCH v2] " David Marchand
2024-10-25 9:22 ` Morten Brørup
@ 2024-10-25 9:46 ` Jack Bond-Preston
2024-10-30 8:26 ` David Marchand
2 siblings, 0 replies; 8+ messages in thread
From: Jack Bond-Preston @ 2024-10-25 9:46 UTC (permalink / raw)
To: David Marchand, dev
Cc: Mattias Rönnblom, Morten Brørup, Tyler Retzlaff
Acked-by: Jack Bond-Preston <jack.bond-preston@foss.arm.com>
On 25/10/2024 08:39, David Marchand wrote:
> Coverity is not able to understand that having 2 lcores means that
> rte_get_next_lcore(-1, 0, 1) can't return RTE_MAX_LCORE.
> Add a check.
>
> Coverity issue: 445382, 445383, 445384, 445387, 445389, 445391
> Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v2:
> - added a helper,
> - prefer RTE_VERIFY(),
>
> Note:
> - a better fix would be to check lcore id validity in the EAL launch API,
> but it requires inspecting all functions and it could result in some
> API change, so sending this as a simple fix for now,
>
> ---
> app/test/test_bitops.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/app/test/test_bitops.c b/app/test/test_bitops.c
> index 681e984037..78a7df6bb1 100644
> --- a/app/test/test_bitops.c
> +++ b/app/test/test_bitops.c
> @@ -13,6 +13,17 @@
> #include <rte_random.h>
> #include "test.h"
>
> +static unsigned int
> +get_worker_lcore(void)
> +{
> + unsigned int lcore_id = rte_get_next_lcore(-1, 1, 0);
> +
> + /* avoid checkers (like Coverity) false positives */
> + RTE_VERIFY(lcore_id < RTE_MAX_LCORE);
> +
> + return lcore_id;
> +}
> +
> #define GEN_TEST_BIT_ACCESS(test_name, set_fun, clear_fun, assign_fun, flip_fun, test_fun, size, \
> mod) \
> static int \
> @@ -158,7 +169,7 @@ test_bit_atomic_parallel_assign ## size(void) \
> printf("Need multiple cores to run parallel test.\n"); \
> return TEST_SKIPPED; \
> } \
> - worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> + worker_lcore_id = get_worker_lcore(); \
> lmain.bit = rte_rand_max(size); \
> do { \
> lworker.bit = rte_rand_max(size); \
> @@ -217,7 +228,7 @@ test_bit_atomic_parallel_test_and_modify ## size(void) \
> printf("Need multiple cores to run parallel test.\n"); \
> return TEST_SKIPPED; \
> } \
> - worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> + worker_lcore_id = get_worker_lcore(); \
> int rc = rte_eal_remote_launch(run_parallel_test_and_modify ## size, &lworker, \
> worker_lcore_id); \
> TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
> @@ -266,7 +277,7 @@ test_bit_atomic_parallel_flip ## size(void) \
> printf("Need multiple cores to run parallel test.\n"); \
> return TEST_SKIPPED; \
> } \
> - worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> + worker_lcore_id = get_worker_lcore(); \
> int rc = rte_eal_remote_launch(run_parallel_flip ## size, &lworker, worker_lcore_id); \
> TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
> run_parallel_flip ## size(&lmain); \
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] test/bitops: check worker lcore availability
2024-10-25 7:39 ` [PATCH v2] " David Marchand
2024-10-25 9:22 ` Morten Brørup
2024-10-25 9:46 ` Jack Bond-Preston
@ 2024-10-30 8:26 ` David Marchand
2 siblings, 0 replies; 8+ messages in thread
From: David Marchand @ 2024-10-30 8:26 UTC (permalink / raw)
To: David Marchand
Cc: dev, Jack Bond-Preston, Mattias Rönnblom,
Morten Brørup, Tyler Retzlaff
On Fri, Oct 25, 2024 at 9:39 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> Coverity is not able to understand that having 2 lcores means that
> rte_get_next_lcore(-1, 0, 1) can't return RTE_MAX_LCORE.
> Add a check.
>
> Coverity issue: 445382, 445383, 445384, 445387, 445389, 445391
> Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Applied, thanks.
--
David Marchand
^ permalink raw reply [flat|nested] 8+ messages in thread