[PATCH] test/bitops: check worker lcore availability

[PATCH] test/bitops: check worker lcore availability

[David Marchand]

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"); \
 	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); \
-- 
2.46.2

RE: [PATCH] test/bitops: check worker lcore availability

[Morten Brørup]

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

Re: [PATCH] test/bitops: check worker lcore availability

[Mattias Rönnblom]

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); \

Re: [PATCH] test/bitops: check worker lcore availability

[David Marchand]

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