[v5,2/2] test/service: fix race condition on stopping lcore
Checks
Commit Message
This commit fixes a potential race condition in the tests
where the lcore running a service would increment a counter
that was already reset by the test-suite thread. The resulting
race-condition incremented value could cause CI failures, as
indicated by DPDK's CI.
This patch fixes the race-condition by making use of the
added rte_service_lcore_active() API, which indicates when
a service-core is no longer in the service-core polling loop.
The unit test makes use of the above function to detect when
all statistics increments are done in the service-core thread,
and then the unit test continues finalizing and checking state.
Fixes: f28f3594ded2 ("service: add attribute API")
Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
v4:
- Update test to new _may_be_ style API (Honnappa)
- Add reviewed by from ML
v3:
- Refactor while() to for() to simplify (Harry)
- Use SERVICE_DELAY instead of magic const 1 (Phil)
- Add Phil's reviewed by tag from ML
v2:
Thanks for discussion on v1, this v2 fixup for the CI
including previous feedback on ML.
---
app/test/test_service_cores.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
Comments
On Fri, Jul 24, 2020 at 3:44 PM Harry van Haaren
<harry.van.haaren@intel.com> wrote:
>
> This commit fixes a potential race condition in the tests
> where the lcore running a service would increment a counter
> that was already reset by the test-suite thread. The resulting
> race-condition incremented value could cause CI failures, as
> indicated by DPDK's CI.
>
> This patch fixes the race-condition by making use of the
> added rte_service_lcore_active() API, which indicates when
> a service-core is no longer in the service-core polling loop.
>
> The unit test makes use of the above function to detect when
> all statistics increments are done in the service-core thread,
> and then the unit test continues finalizing and checking state.
>
> Fixes: f28f3594ded2 ("service: add attribute API")
>
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
There is still a compilation issue reported by the Intel CI.
http://mails.dpdk.org/archives/test-report/2020-July/146535.html
Please fix.
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, September 14, 2020 9:37 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev <dev@dpdk.org>; Igor Romanov <igor.romanov@oktetlabs.ru>; Honnappa
> Nagarahalli <honnappa.nagarahalli@arm.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; nd <nd@arm.com>; Aaron Conole
> <aconole@redhat.com>; Lukasz Wojciechowski
> <l.wojciechow@partner.samsung.com>; Phil Yang <phil.yang@arm.com>
> Subject: Re: [PATCH v5 2/2] test/service: fix race condition on stopping lcore
>
> On Fri, Jul 24, 2020 at 3:44 PM Harry van Haaren
> <harry.van.haaren@intel.com> wrote:
> >
> > This commit fixes a potential race condition in the tests
> > where the lcore running a service would increment a counter
> > that was already reset by the test-suite thread. The resulting
> > race-condition incremented value could cause CI failures, as
> > indicated by DPDK's CI.
> >
> > This patch fixes the race-condition by making use of the
> > added rte_service_lcore_active() API, which indicates when
> > a service-core is no longer in the service-core polling loop.
> >
> > The unit test makes use of the above function to detect when
> > all statistics increments are done in the service-core thread,
> > and then the unit test continues finalizing and checking state.
> >
> > Fixes: f28f3594ded2 ("service: add attribute API")
> >
> > Reported-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>
> There is still a compilation issue reported by the Intel CI.
> http://mails.dpdk.org/archives/test-report/2020-July/146535.html
>
> Please fix.
Done, v6 sent; http://patches.dpdk.org/project/dpdk/list/?series=12198
@@ -362,6 +362,9 @@ service_lcore_attr_get(void)
"Service core add did not return zero");
TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(id, slcore_id, 1),
"Enabling valid service and core failed");
+ /* Ensure service is not active before starting */
+ TEST_ASSERT_EQUAL(0, rte_service_lcore_may_be_active(slcore_id),
+ "Not-active service core reported as active");
TEST_ASSERT_EQUAL(0, rte_service_lcore_start(slcore_id),
"Starting service core failed");
@@ -382,7 +385,22 @@ service_lcore_attr_get(void)
lcore_attr_id, &lcore_attr_value),
"Invalid lcore attr didn't return -EINVAL");
- rte_service_lcore_stop(slcore_id);
+ /* Ensure service is active */
+ TEST_ASSERT_EQUAL(1, rte_service_lcore_may_be_active(slcore_id),
+ "Active service core reported as not-active");
+
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(id, slcore_id, 0),
+ "Disabling valid service and core failed");
+ TEST_ASSERT_EQUAL(0, rte_service_lcore_stop(slcore_id),
+ "Failed to stop service lcore");
+
+ /* Wait until service lcore not active, or for 100x SERVICE_DELAY */
+ for (int i = 0; rte_service_lcore_may_be_active(slcore_id) == 1 &&
+ i < 100; i++)
+ rte_delay_ms(SERVICE_DELAY);
+
+ TEST_ASSERT_EQUAL(0, rte_service_lcore_may_be_active(slcore_id),
+ "Service lcore not stopped after waiting.");
TEST_ASSERT_EQUAL(0, rte_service_lcore_attr_reset_all(slcore_id),
"Valid lcore_attr_reset_all() didn't return success");