On 2022-06-27 20:19, Honnappa Nagarahalli wrote: > <snip> >>>> >>>>> From: Mattias Rönnblom [mailto:mattias.ronnb...@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.
