netdev - Re: [net 02/11] net/mlx5e: Fix multicast counter not up-to-date in "ip -s"

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]

Date:   Tue, 7 Jul 2020 09:37:07 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Saeed Mahameed <saeedm@...lanox.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...lanox.com>,
        Ron Diskin <rondi@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [net 02/11] net/mlx5e: Fix multicast counter not up-to-date in
 "ip -s"

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.