RE: Service core statistics MT safety

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> Sent: Friday, 1 July 2022 20.38
> 
> <snip>
> >
> > <big snip of previous discussions>
> >
> > > > At the time of the read operation (in the global counter
> solution),
> > > > there may well be cycles consumed or calls having been made, but
> not
> > > > yet posted. The window between call having been made, and global
> > > > counter having been incremented (and thus made globally visible)
> is small,
> > but non-zero.
> > > Agree. The read value is the atomic state of the system at a given
> > > instance (when the read was executed), though that instance
> happened few
> > cycles back.
> > > (Just to be clear, I am fine with per-core counters)
> >
> > Option 1: "Per core counters"
> >
> > > Agree we need atomic operations. I am not sure if
> __atomic_fetch_add
> > > or __atomic_store_n would have a large difference.
> __atomic_fetch_add
> > > would result in less number of instructions. I am fine with either.
> >
> > Option 2: "Use atomics for counter increments".
> >
> > > > >> I was fortunate to get some data from a real-world
> application,
> > > > >> and enabling service core stats resulted in a 7% degradation
> of
> > > > >> overall system capacity. I'm guessing atomic instructions
> would
> > > > >> not make things
> > > > better.
> >
> > Agree, performance of atomics is likely to reduce performance.. but
> correctness
> > is worth more than performance.
> >
> > <snip>
> >
> > In my mind, any LTS/backports get the simplest/highest-confidence
> bugfix: using
> > atomics.
> > The atomics are behind the "service stats" feature enable, so impact
> is only
> > when those are enabled.
> >
> > If there is still a performance hit, and there are *no* MT services
> registered, we
> > could check a static-global flag, and if there are no MT services use
> the normal
> > adds. Thoughts on such a solution to reduce atomic perf impact only
> to apps
> > with MT-services?
> The choice is not between atomic stats and non-atomic stats. The stats
> need to be incremented atomically in both cases(MT or no MT services)
> as the reader could be another thread. We need to ensure that the
> stores and loads are not split. Hence, we need atomic operations. We
> cannot avoid any performance hit here.
> 
> I think the choice is between per-core stats vs global stats. IMO, we
> should go with per-core stats to be aligned with the norm.
+1

And the per-core counters can be summed when read for statistics, so it still looks like one counter per service. They are only updated per-core to prevent cache trashing.

> 
> >
> > The code changes themselves are OK.. I can send a patch with fix if
> there's
> > agreement on the approach?
> >
> >
> > diff --git a/lib/eal/common/rte_service.c
> b/lib/eal/common/rte_service.c index
> > ef31b1f63c..a07c8fc2d7 100644
> > --- a/lib/eal/common/rte_service.c
> > +++ b/lib/eal/common/rte_service.c
> > @@ -363,9 +363,9 @@ 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;
> > +               __atomic_fetch_add(&s->cycles_spent, (end-start),
> > __ATOMIC_RELAXED);
> > +               __atomic_fetch_add(&s->calls, 1, __ATOMIC_RELAXED);
> >                 cs->calls_per_service[service_idx]++;
> > -               s->calls++;
> >         } else
> >                 s->spec.callback(userdata);  }