Re: [PATCH v2 3/3] mempool: use cache for frequently updated statistics

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2022-11-02 10:29, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>> Sent: Wednesday, 2 November 2022 09.01
>>
>> On 2022-10-31 12:26, Morten Brørup wrote:
> 
> [...]
> 
>>> +++ b/lib/mempool/rte_mempool.h
>>> @@ -86,6 +86,21 @@ struct rte_mempool_cache {
>>>    	uint32_t size;	      /**< Size of the cache */
>>>    	uint32_t flushthresh; /**< Threshold before we flush excess
>> elements */
>>>    	uint32_t len;	      /**< Current cache count */
>>> +	uint32_t unused0;
>>> +#ifdef RTE_LIBRTE_MEMPOOL_STATS
>>> +	/*
>>> +	 * Alternative location for the most frequently updated mempool
>> statistics (per-lcore),
>>> +	 * providing faster update access when using a mempool cache.
>>> +	 */
>>> +	struct {
>>> +		uint64_t put_bulk;          /**< Number of puts. */
>>> +		uint64_t put_objs;          /**< Number of objects
>> successfully put. */
>>> +		uint64_t get_success_bulk;  /**< Successful allocation
>> number. */
>>> +		uint64_t get_success_objs;  /**< Objects successfully
>> allocated. */
>>> +	} stats;                        /**< Statistics */
>>> +#else
>>> +	uint64_t unused1[4];
>>
>> Are a particular DPDK version supposed to be ABI compatible with
>> itself,
>> with different configuration options? E.g., with and without
>> RTE_LIBRTE_MEMPOOL_STATS. Is that why you have those 4 unused
>> uint64_ts?
> 
> Valid point: There was no ABI compatibility between with and without RTE_LIBRTE_MEMPOOL_STATS before this patch, so there is no need to add partial ABI compatibility here.
> 
> The #else part of this structure should be removed.
> 
> Does anyone disagree?
> 
>>> +#endif
> 

I have no opinion on that matter, but I have another question: if you 
remove 'unused1', should you also remove the unused0 field?