Re: [PATCH v4 3/3] mempool: use cache for frequently updated stats

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 11/4/22 15:03, Morten Brørup wrote:
> When built with stats enabled (RTE_LIBRTE_MEMPOOL_STATS defined), the
> performance of mempools with caches is improved as follows.
> 
> When accessing objects in the mempool, either the put_bulk and put_objs or
> the get_success_bulk and get_success_objs statistics counters are likely
> to be incremented.
> 
> By adding an alternative set of these counters to the mempool cache
> structure, accessing the dedicated statistics structure is avoided in the
> likely cases where these counters are incremented.
> 
> The trick here is that the cache line holding the mempool cache structure
> is accessed anyway, in order to access the 'len' or 'flushthresh' fields.
> Updating some statistics counters in the same cache line has lower
> performance cost than accessing the statistics counters in the dedicated
> statistics structure, which resides in another cache line.
> 
> mempool_perf_autotest with this patch shows the following improvements in
> rate_persec.
> 
> The cost of enabling mempool stats (without debug) after this patch:
> -6.8 % and -6.7 %, respectively without and with cache.
> 
> v4:
> * Fix checkpatch warnings:
>    A couple of typos in the patch description.
>    The macro to add to a mempool cache stat variable should not use
>    do {} while (0). Personally, I would tend to disagree with this, but
>    whatever keeps the CI happy.
> v3:
> * Don't update the description of the RTE_MEMPOOL_STAT_ADD macro.
>    This change belongs in the first patch of the series.
> v2:
> * Move the statistics counters into a stats structure.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[snip]

> +/**
> + * @internal When stats is enabled, store some statistics.
> + *
> + * @param cache
> + *   Pointer to the memory pool cache.
> + * @param name
> + *   Name of the statistics field to increment in the memory pool cache.
> + * @param n
> + *   Number to add to the statistics.
> + */
> +#ifdef RTE_LIBRTE_MEMPOOL_STATS
> +#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) (cache)->stats.name += n

I'd enclose it in parenthesis.

> +#else
> +#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) do {} while (0)
> +#endif
> +