Received-SPF: pass (google.com: domain of linux-kernel+bounces-210163-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <4d3871c1-afa1-4402-ad62-2fdb9d58dc3c@linux.dev>
Date: Tue, 11 Jun 2024 11:36:31 -0400
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH net-next 3/3] net: xilinx: axienet: Add statistics support
To: Andrew Lunn <andrew@lunn.ch>
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>,
 netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 Michal Simek <michal.simek@amd.com>, Jakub Kicinski <kuba@kernel.org>,
 Russell King <linux@armlinux.org.uk>, Paolo Abeni <pabeni@redhat.com>,
 Eric Dumazet <edumazet@google.com>, linux-kernel@vger.kernel.org,
 "David S . Miller" <davem@davemloft.net>
References: <20240610231022.2460953-1-sean.anderson@linux.dev>
 <20240610231022.2460953-4-sean.anderson@linux.dev>
 <40cff9a6-bad3-4f85-8cbc-6d4bc72f9b9f@lunn.ch>
Content-Language: en-US
From: Sean Anderson <sean.anderson@linux.dev>
In-Reply-To: <40cff9a6-bad3-4f85-8cbc-6d4bc72f9b9f@lunn.ch>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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.

--Sean