Re: [dpdk-dev] [PATCH v2 1/6] service: fix race condition for MT unsafe service

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

Hi Harry,
	Thanks for getting back on this.

<snip>

> > Subject: [PATCH v2 1/6] service: fix race condition for MT unsafe
> > service
> >
> > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >
> > The MT unsafe service might get configured to run on another core
> > while the service is running currently. This might result in the MT
> > unsafe service running on multiple cores simultaneously. Use
> > 'execute_lock' always when the service is MT unsafe.
> >
> > Fixes: e9139a32f6e8 ("service: add function to run on app lcore")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > ---
> 
> Thanks for spinning a new revision - based on ML discussion previously, it
> seems like the "use service-run-count" to avoid this race would be a complex
> solution.
> 
> Suggesting the following;
> 1) Take the approach as per this patch, to always take the atomic, fixing the
> race condition.
Ok

> 2) Add an API to service-cores, which allows "committing" of mappings.
> Committing the mapping would imply that the mappings will not be changed
> in future. With runtime-remapping being removed from the equation, the
> existing branch-over-atomic optimization is valid again.
Ok. Just to make sure I understand this:
a) on the data plane, if commit API is called (probably a new state variable) and num_mapped_cores is set to 1, there is no need to take the lock.
b) possible implementation of the commit API would check if num_mapped_cores for the service is set to 1 and set a variable to indicate that the lock is not required.

What do you think about asking the application to set  the service capability to MT_SAFE if it knows that the service will run on a single core? This would require us to change the documentation and does not require additional code.

> 
> So this would offer applications two situations
> A) No application change: possible performance regression due to atomic
> always taken.
> B) Call "commit" API, and regain the performance as per previous DPDK
> versions.
> 
> Thoughts/opinions on the above?  I've flagged the rest of the patchset for
> review ASAP. Regards, -Harry
> 
> >  lib/librte_eal/common/rte_service.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/rte_service.c
> > b/lib/librte_eal/common/rte_service.c
> > index 70d17a5..b8c465e 100644
> > --- a/lib/librte_eal/common/rte_service.c
> > +++ b/lib/librte_eal/common/rte_service.c
> > @@ -50,6 +50,10 @@ struct rte_service_spec_impl {
> >  	uint8_t internal_flags;
> >
> >  	/* per service statistics */
> > +	/* Indicates how many cores the service is mapped to run on.
> > +	 * It does not indicate the number of cores the service is running
> > +	 * on currently.
> > +	 */
> >  	rte_atomic32_t num_mapped_cores;
> >  	uint64_t calls;
> >  	uint64_t cycles_spent;
> > @@ -370,12 +374,7 @@ service_run(uint32_t i, struct core_state *cs,
> > uint64_t service_mask,
> >
> >  	cs->service_active_on_lcore[i] = 1;
> >
> > -	/* check do we need cmpset, if MT safe or <= 1 core
> > -	 * mapped, atomic ops are not required.
> > -	 */
> > -	const int use_atomics = (service_mt_safe(s) == 0) &&
> > -				(rte_atomic32_read(&s-
> >num_mapped_cores) > 1);
> > -	if (use_atomics) {
> > +	if (service_mt_safe(s) == 0) {
> >  		if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1))
> >  			return -EBUSY;
> >
> > --
> > 2.7.4