linux-kernel - Re: [PATCH net-next 3/3] net: xilinx: axienet: Add statistics support

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6f4916a0-f949-4289-9839-d89af574600d@linux.dev>
Date: Tue, 18 Jun 2024 13:03:47 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Andrew Lunn <andrew@...n.ch>
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@....com>,
 netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 Michal Simek <michal.simek@....com>, Jakub Kicinski <kuba@...nel.org>,
 Russell King <linux@...linux.org.uk>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, linux-kernel@...r.kernel.org,
 "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next 3/3] net: xilinx: axienet: Add statistics support

Hi Andrew,

On 6/11/24 11:36, Sean Anderson wrote:
> On 6/10/24 20:26, Andrew Lunn wrote:
>>> +static u64 axienet_stat(struct axienet_local *lp, enum temac_stat stat)
>>> +{
>>> +	return u64_stats_read(&lp->hw_stats[stat]);
>>> +}
>>> @@ -1695,6 +1760,35 @@ axienet_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
>>>  		stats->tx_packets = u64_stats_read(&lp->tx_packets);
>>>  		stats->tx_bytes = u64_stats_read(&lp->tx_bytes);
>>>  	} while (u64_stats_fetch_retry(&lp->tx_stat_sync, start));
>>> +
>>> +	if (!(lp->features & XAE_FEATURE_STATS))
>>> +		return;
>>> +
>>> +	do {
>>> +		start = u64_stats_fetch_begin(&lp->hw_stat_sync);
>>> +		stats->rx_length_errors =
>>> +			axienet_stat(lp, STAT_RX_LENGTH_ERRORS);
>> 
>> I'm i reading this correctly. You are returning the counters from the
>> last refresh period. What is that? 2.5Gbps would wrapper around a 32
>> byte counter in 13 seconds. I hope these statistics are not 13 seconds
>> out of date?
> 
> By default we use a 1 Hz refresh period. You can of course configure this
> up to 13 seconds, but we refuse to raise it further since we risk missing
> a wrap-around. It's configurable by userspace so they can determine how
> out-of-date they like their stats (vs how often they want to wake up the
> CPU).
> 
>> Since axienet_stats_update() also uses the lp->hw_stat_sync, i don't
>> see why you cannot read the hardware counter value and update to the
>> latest value.
> 
> We would need to synchronize against updates to hw_last_counter. Imagine
> a scenario like
> 
> CPU 1					CPU 2
> __axienet_device_reset()
> 	axienet_stats_update()
> 					axienet_stat()
> 						u64_stats_read()
> 						axienet_ior()
> 	/* device reset */
> 	hw_last_counter = 0
> 						stats->foo = ... - hw_last_counter[...]
> 
> and now we have a glitch in the counter values, since we effectively are
> double-counting the current counter value. Alternatively, we could read
> the counter after reset but before hw_last_counter was updated and get a
> glitch due to underflow.

Does this make sense to you? If it does, I'll send v2 with just the mutex
change and the variable rename pointed out by Simon.

--Sean