linux-kernel - Re: [PATCH net-next v4 2/2] 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: <b7f66966-f97a-4890-b452-2a8a5e20b953@linux.dev>
Date: Tue, 20 Aug 2024 16:02:45 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>,
 Andrew Lunn <andrew@...n.ch>, "netdev@...r.kernel.org"
 <netdev@...r.kernel.org>
Cc: "Simek, Michal" <michal.simek@....com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Russell King <linux@...linux.org.uk>, "David S . Miller"
 <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 Simon Horman <horms@...nel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "Katakam, Harini" <harini.katakam@....com>
Subject: Re: [PATCH net-next v4 2/2] net: xilinx: axienet: Add statistics
 support

On 8/20/24 15:04, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@...ux.dev>
>> Sent: Tuesday, August 20, 2024 11:24 PM
>> To: Andrew Lunn <andrew@...n.ch>; Pandey, Radhey Shyam
>> <radhey.shyam.pandey@....com>; netdev@...r.kernel.org
>> Cc: Simek, Michal <michal.simek@....com>; linux-kernel@...r.kernel.org;
>> Russell King <linux@...linux.org.uk>; David S . Miller
>> <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
>> <pabeni@...hat.com>; Eric Dumazet <edumazet@...gle.com>; Simon
>> Horman <horms@...nel.org>; linux-arm-kernel@...ts.infradead.org; Sean
>> Anderson <sean.anderson@...ux.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@...ux.dev>

--Sean