RE: Service core statistics MT safety

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

<snip>
> >>>>>>>>
> >>>>>>>>> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> >>>>>>>>> Sent: Monday, 27 June 2022 13.06
> >>>>>>>>>
> >>>>>>>>> Hi.
> >>>>>>>>>
> >>>>>>>>> Is it safe to enable stats on MT safe services?
> >>>>>>>>>
> >>>>>>>>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-
> >> 313273af
> >>>>>>>>> -
> >>>> 4
> >>>>>>>>> 54445555731-6096fdb16385f88f&q=1&e=27b94605-d1e2-40b6-
> >> af6d-
> >>>> 9ebc54d
> >>>>>>>>>
> >>>>
> >>
> 5db18&u=https%3A%2F%2Fgithub.com%2FDPDK%2Fdpdk%2Fblob%2Fmain%
> >>>> 2Flib
> >>>>>>>>> %2Feal%2Fcommon%2Frte_service.c%23
> >>>>>>>>> L3
> >>>>>>>>> 6
> >>>>>>>>> 6
> >>>>>>>>>
> >>>>>>>>> It seems to me this would have to be an __atomic_add for this
> >>>>>>>>> code to produce deterministic results.
> >>>>>>>>
> >>>>>>>> I agree. The same goes for the 'calls' field.
> >>>>>>> The calling function does the locking.
> >>>>>>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-
> 313273af
> >>>>>>> -
> >>>> 454
> >>>>>>> 445555731-5ce419f8bf9f9b76&q=1&e=27b94605-d1e2-40b6-af6d-
> >>>> 9ebc54d5db1
> >>>>>>>
> >>>>
> >>
> 8&u=https%3A%2F%2Fgithub.com%2FDPDK%2Fdpdk%2Fblob%2Fmain%2Flib
> >>>> %2Feal
> >>>>>>> %2Fcommon%2Frte_service.c%23L3
> >>>>>>> 98
> >>>>>>>
> >>>>>>> For more information you can look at:
> >>>>>>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-
> 313273af
> >>>>>>> -
> >>>> 454
> >>>>>>> 445555731-ba0d1416f08856f0&q=1&e=27b94605-d1e2-40b6-
> af6d-
> >>>> 9ebc54d5db1
> >>>>>>>
> >>>>
> >>
> 8&u=https%3A%2F%2Fgithub.com%2FDPDK%2Fdpdk%2Fblob%2Fmain%2Flib
> >>>> %2Feal
> >>>>>>> %2Finclude%2Frte_service.h%23L
> >>>>>>> 120
> >>>>>>>
> >>>>>>
> >>>>>> What about the
> >>>>>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-
> 313273af-
> >>>> 4544
> >>>>>> 45555731-b64334addc78c264&q=1&e=27b94605-d1e2-40b6-af6d-
> >>>> 9ebc54d5db18&
> >>>>>>
> >>>>
> >>
> u=https%3A%2F%2Fgithub.com%2FDPDK%2Fdpdk%2Fblob%2Fmain%2Flib%2
> >>>> Feal%2F
> >>>>>> common%2Frte_service.c%23L404
> >>>>>> call (for MT safe services)?
> >>>>>>
> >>>>>> There's no lock held there.
> >>>>> Good point.
> >>>>> This is the case where the service running in service cores is MT
> >>>>> safe. However,
> >>>> the stats are incremented outside of the MT Safety mechanism
> >>>> employed by the service. So, yes, this and other updates in the
> >>>> function 'service_runner_do_callback' need to be updated atomically.
> >>>>
> >>>> Maybe a better solution would be to move this to the core_state
> >>>> struct (and eliminate the "calls" field since the same information
> >>>> is already in the core_state struct). The contention on these cache
> >>>> lines will be pretty crazy for services with short run time (say a
> >>>> thousand cycles or less per call), assuming they are mapped to many
> cores.
> >>> That's one option, the structures are internal as well. With this
> >>> option stats
> >> need to be aggregated which will not give an accurate view. But, that
> >> is the nature of the statistics.
> >>>
> >>
> >> Per-core counters is a very common pattern. Used for Linux MIB
> >> counters, for example. I'm not sure I think it's much less accurate.
> >> I mean, you just load in quick succession what's globally visible for
> >> the different per-lcore counters. If you do a relaxed store on an
> >> ARM, how long time does it take until it's seen by someone doing a relaxed
> load on a different core? Roughly.
> > I think my explanation of the problem is not clear.
> >
> > If a service is running on more than one core and the stats are per core, when
> we aggregate, the resulting statistics is not atomic. By making the stats per core,
> we will be taking out that feature which is present currently (even though it is
> implemented incorrectly). As we agree, the proposed change is a common
> pattern and taking away the atomicity of the stats might not be a problem.
> >
> 
> Isn't it just a push model, versus a pull one? Both give just an approximation,
> albeit a very good one, of how many cycles are spent "now" for a particular
> service. Isn't time a local phenomena in a SMP system, and there is no global
> "now"? Maybe you can achieve such with a transaction or handshake of some
> sort, but I don't see how the an __atomic_add would be enough.
If we consider a global time line of events, using atomic operation will provide a single 'now' from the reader's perspective (of course there might be writers waiting to update). Without the atomic operations, there will not be a single 'now' from reader's perspective, there will be multiple read events on the timeline.

> 
> 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.
Is the service running on multiple cores?

> 
> >>
> >>> I am also wondering if these stats are of any use other than for debugging.
> >> Adding a capability to disable stats might help as well.
> >>>
> >>
> >> They could be used as a crude tool to determine service core utilization.
> >> Comparing utilization between different services running on the same core
> >> should be straight-forward, but lcore utilization is harder in absolute terms. If
> >> you just look at "cycles", a completely idle core would look like it's very busy
> >> (basically rdtsc latency added for every loop). I assume you'd have to do
> some
> >> heuristic based on both "calls" and "cycles" to get an estimate.
> >>
> >> I think service core utilization would be very useful, although that would
> require
> >> some changes in the service function signature, so the service can report
> back if
> >> it did some useful work for a particular call.
> >>
> >> That would make for a DPDK 'top'. Just like 'top', it can't impose any serious
> >> performance degradation when used, to be really useful, I think.
> >>
> >> Sure, it should be possible to turn it on and off. I thought that was the case
> >> already?
> > Thanks, yes, this exists already. Though the 'loops' counter is out of the stats
> enable check, looks like it is considered as an attribute for some reason.
> >
> >>
> >>>>
> >>>> Idle service cores will basically do nothing else than stall waiting
> >>>> for these lines, I suspect, hampering the progress of more busy cores.
> >