[PATCH 2/2] service: fix potential stats race-condition on MT services

[Honnappa Nagarahalli]

> > <snip previous discussions>
> >
> > > > diff --git a/lib/eal/common/rte_service.c
> > > > b/lib/eal/common/rte_service.c index ef31b1f63c..f045e74ef3 100644
> > > > --- a/lib/eal/common/rte_service.c
> > > > +++ b/lib/eal/common/rte_service.c
> > > > @@ -363,9 +363,15 @@ service_runner_do_callback(struct
> > > > rte_service_spec_impl *s,
> > > >  		uint64_t start = rte_rdtsc();
> > > >  		s->spec.callback(userdata);
> > > >  		uint64_t end = rte_rdtsc();
> > > > -		s->cycles_spent += end - start;
> > > > +		uint64_t cycles = end - start;
> > > >  		cs->calls_per_service[service_idx]++;
> > > > -		s->calls++;
> > > > +		if (service_mt_safe(s)) {
> > > > +			__atomic_fetch_add(&s->cycles_spent, cycles,
> > > > __ATOMIC_RELAXED);
> > > > +			__atomic_fetch_add(&s->calls, 1,
> > > > __ATOMIC_RELAXED);
> > > > +		} else {
> > > > +			s->cycles_spent += cycles;
> > > > +			s->calls++;
> > > This is still a problem from a reader perspective. It is possible
> > > that the writes could be split while a reader is reading the stats. These
> need to be atomic adds.
> >
> > Thanks for pointing out; I do "think" in x86 in terms of load/store
> > tearing; and on x86 naturally aligned load/stores will not tear. Apologies for
> missing the ARM angle here.
Arm architecture has similar things as well. I believe compiler does not provide any guarantees that it will only generate non-tearing instructions. Refer to a discussion in the past [1] [2] where it was thought that the compiler would generate a non-tearing store (this is a slightly different scenario).

[1] http://inbox.dpdk.org/dev/d5d563ab-0411-3faf-39ec-4994f2bc9f6f@intel.com/
[2] Refer to commit '316095eb41ed22da808b5826404c398917c83c89'

> >
> > I'm not sure how to best encode the difference between tearing & "locked
> instructions"
> > to make things multi-writer safe. But they're not the same thing, and
> > I'd prefer not pay the penalty for LOCK instructions (multi-writer) only to
> satisfy the non-tearing requirements.
> >
> > Is there an rte_atomic-* type that is guaranteed as non-tearing?
Nothing that I know of.

> >
> > In that case, changing the type of the calls/cycles_spent variables to such a
> type to ensure "non-tearing"
> > single-reader, single-writer behaviour is enough, instead of forcing
> __atomic_fetch_add() everywhere?
> 
> Regular read, increment and then atomic store should work without locks
> where alignment is correct on most architectures, and doing the store
> atomically should prevent any tearing.
Agree, we could do this, will provide more flexibility for the micro-architecture to work with. Good to understand the perf benefits vs complexity and the branch cost.