Service core statistics MT safety

[Mattias Rönnblom]

On 2022-06-27 20:19, Honnappa Nagarahalli wrote:
> <snip>
>>>>
>>>>> From: Mattias Rönnblom [mailto:mattias.ronnblom at 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-454445555731-6096fdb16385f88f&q=1&e=27b94605-d1e2-40b6-af6d-9ebc54d5db18&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-454445555731-5ce419f8bf9f9b76&q=1&e=27b94605-d1e2-40b6-af6d-9ebc54d5db18&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-454445555731-ba0d1416f08856f0&q=1&e=27b94605-d1e2-40b6-af6d-9ebc54d5db18&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-454445555731-b64334addc78c264&q=1&e=27b94605-d1e2-40b6-af6d-9ebc54d5db18&u=https%3A%2F%2Fgithub.com%2FDPDK%2Fdpdk%2Fblob%2Fmain%2Flib%2Feal%2Fcommon%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.

Idle service cores will basically do nothing else than stall waiting for 
these lines, I suspect, hampering the progress of more busy cores.