Re: [PATCH net-next v2 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

On Wed, 2 Dec 2020 02:28:51 +0200 Vladimir Oltean wrote:

On Fri, Nov 27, 2020 at 10:36:42PM +0100, Andrew Lunn wrote:

quoted

quoted

Either way, can we conclude that ndo_get_stats64 is not a replacement
for ethtool -S, since the latter is blocking and, if implemented correctly,
can return the counters at the time of the call (therefore making sure
that anything that happened before the syscall has been accounted into
the retrieved values), and the former isn't?  

ethtool -S is the best source of consistent, up to date statistics we
have. It seems silly not to include everything the hardware offers
there.  

To add to this, it would seem odd to me if we took the decision to not
expose MAC-level counters any longer in ethtool. Say the MAC has a counter
named rx_dropped. If we are only exposing this counter in ndo_get_stats64,
then we could hit the scenario where this counter keeps incrementing,
but it is the network stack who increments it, and not the MAC.

dev_get_stats() currently does:
	storage->rx_dropped += (unsigned long)atomic_long_read(&dev->rx_dropped);
	storage->tx_dropped += (unsigned long)atomic_long_read(&dev->tx_dropped);
	storage->rx_nohandler += (unsigned long)atomic_long_read(&dev->rx_nohandler);

thereby clobbering the MAC-provided counter. We would not know if it is
a MAC-level drop or not.

Fine granularity HW stats are fine, but the aggregate must be reported
in standard stats first.

The correct stat for MAC drops (AFAIU) is rx_missed.

This should act as a generic "device had to drop valid packets"
indication and ethtool -S should serve for manual debugging to find 
out which stage of pipeline / reason caused the drop.