netdev - Re: [PATCH net-next 1/2] page_pool: Convert page_pool_recycle_stats to u64_stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <307939b7-8f51-437a-b5b2-ac5342630504@huawei.com>
Date: Sat, 22 Feb 2025 16:13:35 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	<linux-rdma@...r.kernel.org>, <netdev@...r.kernel.org>
CC: "David S. Miller" <davem@...emloft.net>, Andrew Lunn
	<andrew+netdev@...n.ch>, Eric Dumazet <edumazet@...gle.com>, Ilias Apalodimas
	<ilias.apalodimas@...aro.org>, Jakub Kicinski <kuba@...nel.org>, Jesper
 Dangaard Brouer <hawk@...nel.org>, Leon Romanovsky <leon@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, Saeed Mahameed <saeedm@...dia.com>, Simon Horman
	<horms@...nel.org>, Tariq Toukan <tariqt@...dia.com>, Thomas Gleixner
	<tglx@...utronix.de>
Subject: Re: [PATCH net-next 1/2] page_pool: Convert page_pool_recycle_stats
 to u64_stats_t.

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;