RE: [PATCH 2/2] service: fix potential stats race-condition on MT services

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

> From: Harry van Haaren [mailto:harry.van.haaren@intel.com]
> Sent: Friday, 8 July 2022 14.57
> 
> This commit fixes a potential racey-add that could occur if
> multiple service-lcores were executing the same MT-safe service
> at the same time, with service statistics collection enabled.
> 
> Because multiple threads can run and execute the service, the
> stats values can have multiple writer threads, resulting in the
> requirement of using atomic addition for correctness.
> 
> Note that when a MT unsafe service is executed, a spinlock is
> held, so the stats increments are protected. This fact is used
> to avoid executing atomic add instructions when not required.
> 
> This patch causes a 1.25x increase in cycle-cost for polling a
> MT safe service when statistics are enabled. No change was seen
> for MT unsafe services, or when statistics are disabled.
> 
> Reported-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Suggested-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> ---

[...]

> +		if (service_mt_safe(s)) {
> +			__atomic_fetch_add(&s->cycles_spent, cycles,
> __ATOMIC_RELAXED);
> +			__atomic_fetch_add(&s->calls, 1, __ATOMIC_RELAXED);
> +		} else {
> +			s->cycles_spent += cycles;
> +			s->calls++;
> +		}

Have you considered the performance cost of the __atomic_fetch_add(__ATOMIC_RELAXED) versus the performance cost of the branch to compare if the service is MT safe? It might be cheaper to just always use the atomic addition. I don't know, just mentioning that the compare-and-branch also has a cost.

I'm not familiar with the DPDK services library, so perhaps MT safe and MT unsafe services are never mixed, in which case the branch will always take the same path, so branch prediction will eliminate the cost of branching.