On Tue, 7 Jul 2020 03:29:18 +0000 Saeed Mahameed wrote: > On Mon, 2020-07-06 at 19:07 -0700, Jakub Kicinski wrote: > > On Tue, 7 Jul 2020 01:51:21 +0000 Saeed Mahameed wrote: > > > > Also looks like you report the total number of mcast packets in > > > > ethtool > > > > -S, which should be identical to ip -s? If so please remove > > > > that. > > > > > > why ? it is ok to report the same counter both in ehttool and > > > netdev > > > stats. > > > > I don't think it is, Stephen and I have been trying to catch this in > > review for a while now. It's an entirely unnecessary code > > duplication. > > Code duplication shouldn't be a factor. For example, in mlx5 we have a > generic mechanism to define and report stats, for the netdev stats to > be reported in ethtoool all we need to do is define the representing > string and store those counters in the SW stats struct.
And sum them up in a loop. One day we'll have a better API for standard stats and will will we still argue then that ethtool -S should duplicate _everything_. Drivers should put more focus on providing useful information in standard statistics in the first place, then think about exposing more details as needed. > > We should steer towards proper APIs first if those exist. > > > > Using ethtool -S stats gets very messy very quickly in production. > > I agree on ethtool getting messy very quickly, but i disagree on not > reporting netdev stats, I don't think the 4 netdev stats are the reason > for messy ethtool. > > Ethtool -S is meant for verbosity and debug, and it is full of > driver/HW specific counters, how are you planing to audit all of those > ? I don't understand how verbosity, debug, and being full of HW specific counters has any relevance for this patch - which is reporting a pure SW driver stat. > We had an idea in the past to allow users to choose what stats groups > to report to ethtool, we can follow up on this if this is something > other drivers might be interested in. > > example: > > ethtool -S eth0 --groups XDP,SW,PER_QUEUE,PCI,PORT,DRIVER_SPECIFIC > Where non DRIVER_SPECIFC groups are standardize stats objects.. Attributes are useful but the primary problem is the fact that we, driver developers, seem to be funneling all our creative passion into coming up with new names for statistics. Look for example how many names we have for etherStatsPkts256to511Octets And nobody(!) thought it's a good idea to just name the counter what it's called in the RFC. Policing a free form string interface in review is just unworkable. Anyway.. I'm sidetracking.