Re: [PATCH 1/6] service: reduce statistics overhead for parallel services

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2022-10-03 15:33, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>> Sent: Tuesday, September 6, 2022 5:14 PM
>> To: Van; Haaren; Van Haaren, Harry <harry.van.haaren@intel.com>
>> Cc: dev@dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
>> Morten Brørup <mb@smartsharesystems.com>; nd <nd@arm.com>;
>> mattias.ronnblom <mattias.ronnblom@ericsson.com>
>> Subject: [PATCH 1/6] service: reduce statistics overhead for parallel services
>>
>> Move the statistics from the service data structure to the per-lcore
>> struct. This eliminates contention for the counter cache lines, which
>> decreases the producer-side statistics overhead for services deployed
>> across many lcores.
> 
> Agree with the approach, good stuff. One comment below, but no changes
> necessary.
> 
>> Prior to this patch, enabling statistics for a service with a
>> per-service function call latency of 1000 clock cycles deployed across
>> 16 cores on a Intel Xeon 6230N @ 2,3 GHz would incur a cost of ~10000
>> core clock cycles per service call. After this patch, the statistics
>> overhead is reduce to 22 clock cycles per call.
>>
>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> 
> Tested by applying the two patch fixes first (https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-733cb32d7965fb15&q=1&e=2a5d80ad-c4ec-40ca-b66d-1b546435288d&u=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Flist%2F%3Fseries%3D23959)
> and this patchset from Mattias.
> 
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> <snip>
> 
>>   static uint32_t rte_service_count;
>> @@ -138,13 +130,16 @@ rte_service_finalize(void)
>>   	rte_service_library_initialized = 0;
>>   }
>>
>> -/* returns 1 if service is registered and has not been unregistered
>> - * Returns 0 if service never registered, or has been unregistered
>> - */
>> -static inline int
>> +static inline bool
>> +service_registered(uint32_t id)
>> +{
>> +	return rte_services[id].internal_flags & SERVICE_F_REGISTERED;
>> +}
> 
> Before we had a !! to flip any value (e.g. binary 100 to just binary 1).
> SERVICE_F_REGISTERED is binary 1,and internal_flags is u8, so its fine as is.
> 
> <snip>
> 

Since the function now returns a bool, it does not matter what size the 
internal_flags field has or which of its bits SERVICE_F_REGISTERED 
refers to.