Re: [PATCH net-next v4 2/2] net: xilinx: axienet: Add statistics support

[Sean Anderson <sean.anderson@linux.dev>]

On 8/20/24 15:04, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Tuesday, August 20, 2024 11:24 PM
>> To: Andrew Lunn <andrew@lunn.ch>; Pandey, Radhey Shyam
>> <radhey.shyam.pandey@amd.com>; netdev@vger.kernel.org
>> Cc: Simek, Michal <michal.simek@amd.com>; linux-kernel@vger.kernel.org;
>> Russell King <linux@armlinux.org.uk>; David S . Miller
>> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
>> <pabeni@redhat.com>; Eric Dumazet <edumazet@google.com>; Simon
>> Horman <horms@kernel.org>; linux-arm-kernel@lists.infradead.org; Sean
>> Anderson <sean.anderson@linux.dev>
>> Subject: [PATCH net-next v4 2/2] net: xilinx: axienet: Add statistics support
>> 
>> Add support for reading the statistics counters, if they are enabled.
>> The counters may be 64-bit, but we can't detect this statically as
>> there's no ability bit for it and the counters are read-only. Therefore,
>> we assume the counters are 32-bits by default. To ensure we don't miss
> 
> Any reason why we can't have DT property to detect if stats counter
> are configured as 32-bit /64bit? The IP export CONFIG.Statistics_Width
> and device tree generator can read this IP block property and populate 
> stats width property.

Mainly simplicity:

- We need the functions to work with 32-bit counters anyway
- We can always treat 64-bit counters are 32-bit counters
- The reset issue below necessitates keeping track of a "base"
  anyway.

And for my devicetrees (generated with 2022.2) all I get is 

xlnx,stats = <0x1>;

regardless of whether I select 32- or 64-bit counters. So this wouldn't
be something we could reuse from existing devictrees.

>> an overflow, we read all counters at 13-second intervals. This should be
>> often enough to ensure the bytes counters don't wrap at 2.5 Gbit/s.
>> 
>> Another complication is that the counters may be reset when the device
>> is reset (depending on configuration). To ensure the counters persist
>> across link up/down (including suspend/resume), we maintain our own
>> versions along with the last counter value we saw. Because we might wait
> 
> Is that a standard convention to retain/persist counter values across 
> link up/down?

IEEE 802.3 section 30.2.1 says

| All counters defined in this specification are assumed to be
| wrap-around counters. Wrap-around counters are those that
| automatically go from their maximum value (or final value) to zero and
| continue to operate. These unsigned counters do not provide for any
| explicit means to return them to their minimum (zero), i.e., reset.

And get_eth_mac_stats implements these counters for Linux. So I would
say that resetting the counters on link up/down would be non-conformant.

Other drivers also preserve stats across link up/down. For example,
MACB/GEM doesn't reset it stats either. And keeping the stats is also
more friendly for users and monitoring tools.

---

If you happen to have an ear with the RTL designers, I would say that
saturating, clear-on-read counters would be much easier to work with in
software.

--Sean

>> up to 100 ms for the reset to complete, we use a mutex to protect
>> writing hw_stats. We can't sleep in ndo_get_stats64, so we use a seqlock
>> to protect readers.
>> 
>> We don't bother disabling the refresh work when we detect 64-bit
>> counters. This is because the reset issue requires us to read
>> hw_stat_base and reset_in_progress anyway, which would still require the
>> seqcount. And I don't think skipping the task is worth the extra
>> bookkeeping.
>> 
>> We can't use the byte counters for either get_stats64 or
>> get_eth_mac_stats. This is because the byte counters include everything
>> in the frame (destination address to FCS, inclusive). But
>> rtnl_link_stats64 wants bytes excluding the FCS, and
>> ethtool_eth_mac_stats wants to exclude the L2 overhead (addresses and
>> length/type). It might be possible to calculate the byte values Linux
>> expects based on the frame counters, but I think it is simpler to use
>> the existing software counters.
>> 
>> get_ethtool_stats is implemented for nonstandard statistics. This
>> includes the aforementioned byte counters, VLAN and PFC frame
>> counters, and user-defined (e.g. with custom RTL) counters.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

--Sean