Service core statistics MT safety

[Van Haaren, Harry]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>
> Sent: Thursday, July 7, 2022 6:26 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; Morten Brørup
> <mb at smartsharesystems.com>; Mattias Rönnblom <hofors at lysator.liu.se>;
> mattias.ronnblom <mattias.ronnblom at ericsson.com>; dev at dpdk.org
> Cc: nd <nd at arm.com>; nd <nd at arm.com>
> Subject: RE: Service core statistics MT safety
> 
> <snip>

<snip>

> > Yes, understood and agree. Per-core counters are preferred, as relaxed atomic
> > ordered stores are enough to ensure non-tearing loads. Summation before
> > reporting back from the stats-requesting thread.
> > For backporting, per-core counters is a significant change. Is using atomics to fix
> > the miss-behaviour a better idea?
> Agree, the change will be simple.
> 
> This will result in some perf impact which can be mitigated by disabling the stats
> where possible.

Yes, agree, disabling stats is a workaround if they're not required yep.
Correctness > performance. We can optimize to per-CPU stats in a future release.


> > My question below is still open, is the below enough to fix the *functional* part
> > of MT services?
> >
> > Code today uses normal ADD/INCQ (and hence the MT increments bug of
> > tear/clobber exists)
> >    0x000055555649189d <+141>:   add    %rax,0x50(%rbx)
> >    0x00005555564918a1 <+145>:   incq   0x48(%rbx)
> >
> > For x86, my disassembly for RELAXED and SEQ_CST orderings are the same,
> > using LOCK prefix ("proper atomics")
> >    0x000055555649189d <+141>:   lock add %rax,0x50(%rbx)
> >    0x00005555564918a2 <+146>:   lock incq 0x48(%rbx)
> >
> > My understanding is that since 2x threads can write the same address, SEQ_CST
> > is required.
> > Can somebody more familiar with C11 confirm that?
> We need to use RELAXED. You can think of it as, 'do we need a particular order for
> memory operations within a single thread?' In this case, we do not need a particular
> order for incrementing stats in the single thread context.

Thanks for confirming; makes sense.

2 patches sent, you're on CC.