[dpdk-dev] [PATCH v2 2/2] test/service: fix race condition on stopping lcore

[Van Haaren, Harry]

> -----Original Message-----
> From: Phil Yang <Phil.Yang at arm.com>
> Sent: Tuesday, July 21, 2020 9:39 AM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at dpdk.org
> Cc: david.marchand at redhat.com; igor.romanov at oktetlabs.ru; Honnappa
> Nagarahalli <Honnappa.Nagarahalli at arm.com>; Yigit, Ferruh
> <ferruh.yigit at intel.com>; nd <nd at arm.com>; aconole at redhat.com;
> l.wojciechow at partner.samsung.com; nd <nd at arm.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/2] test/service: fix race condition on
> stopping lcore
> 
> <...>
> 
> > Subject: [dpdk-dev] [PATCH v2 2/2] test/service: fix race condition on
> > stopping lcore
> >
> > 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 at redhat.com>
> > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> 
> Minor nit, otherwise it looks good to me.
> 
> Reviewed-by: Phil Yang <phil.yang at arm.com>

Thanks, will add in v3.

<snip>

> > +	int i = 0;
> > +	while (rte_service_lcore_active(slcore_id) == 1) {
> > +		rte_delay_ms(1);
> 
> Just as it does in other functions, use the macro instead of the magic number
> would be better.
> rte_delay_ms(SERVICE_DELAY);

Sure, will change. I've refactored the while() to a for() too, think it cleans up a little.

<snip>