On 2025/2/21 19:52, Sebastian Andrzej Siewior wrote:
>
> static const char pp_stats[][ETH_GSTRING_LEN] = {
> @@ -82,6 +88,7 @@ static const char pp_stats[][ETH_GSTRING_LEN] = {
> bool page_pool_get_stats(const struct page_pool *pool,
> struct page_pool_stats *stats)
> {
> + unsigned int start;
> int cpu = 0;
>
> if (!stats)
> @@ -99,11 +106,19 @@ bool page_pool_get_stats(const struct page_pool *pool,
> const struct page_pool_recycle_stats *pcpu =
> per_cpu_ptr(pool->recycle_stats, cpu);
>
> - stats->recycle_stats.cached += pcpu->cached;
> - stats->recycle_stats.cache_full += pcpu->cache_full;
> - stats->recycle_stats.ring += pcpu->ring;
> - stats->recycle_stats.ring_full += pcpu->ring_full;
> - stats->recycle_stats.released_refcnt += pcpu->released_refcnt;
> + do {
> + start = u64_stats_fetch_begin(&pcpu->syncp);
> + u64_stats_add(&stats->recycle_stats.cached,
> + u64_stats_read(&pcpu->cached));
> + u64_stats_add(&stats->recycle_stats.cache_full,
> + u64_stats_read(&pcpu->cache_full));
> + u64_stats_add(&stats->recycle_stats.ring,
> + u64_stats_read(&pcpu->ring));
> + u64_stats_add(&stats->recycle_stats.ring_full,
> + u64_stats_read(&pcpu->ring_full));
> + u64_stats_add(&stats->recycle_stats.released_refcnt,
> + u64_stats_read(&pcpu->released_refcnt));
It seems the above u64_stats_add() may be called more than one time
if the below u64_stats_fetch_retry() returns true, which might mean
the stats is added more than it is needed.
It seems more correct to me that pool->alloc_stats is read into a
local varible in the while loop and then do the addition outside
the while loop?
> + } while (u64_stats_fetch_retry(&pcpu->syncp, start));
> }
>
> return true;
