netdev - Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

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: <20200912071612.cq7adzzxxgpcauux@skbuf>
Date:   Sat, 12 Sep 2020 10:16:12 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, mkubecek@...e.cz,
        michael.chan@...adcom.com, tariqt@...dia.com, saeedm@...dia.com,
        alexander.duyck@...il.com, andrew@...n.ch
Subject: Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

On Fri, Sep 11, 2020 at 05:42:46PM -0700, Jakub Kicinski wrote:
> > The ethtool -S framework was nice because you could append to the
> > counters of the master interface while not losing them.
> > As for "ethtool -A", those parameters are fixed as part of the
> > fixed-link device tree node corresponding to the CPU port.
>
> I think I'm missing the problem you're trying to describe.
> Are you making a general comment / argument on ethtool stats?

No, it appears to me that you're trying to standardize some things out
of ethtool -S. I just want to make sure that, while doing so, you are
aware of some of the more subtle uses of that interface.

> Pause stats are symmetrical - as can be seen in your quote
> what's RX for the CPU is TX for the switch, and vice versa.

If things work, yes. If they don't, no.

> Since ethtool -A $cpu_mac controls whether CPU netdev generates
> and accepts pause frames, correspondingly the direction and meaning
> of pause statistics on that interface is well defined.

Well, with a fixed-link, there's not a lot you can change with "ethtool
--pause" (the link is managed with phylink). Up until now, I have mostly
only heard of links between a DSA master and the CPU port that are not
fixed-link. In theory (and in limited practice), phylink can even drive
a PHY on the CPU port, so in that case, it may make sense for DSA to try
to automatically apply the mirror pause frame configuration of the
master. However, that wasn't supported before, either, and was not what
I was talking about.

> You can still append your custom CPU port stats to ethtool -S or
> debugfs or whatnot,

I don't understand, so you're saying that DSA can keep pause stats
reporting in "ethtool -S", but the rest of devices should move to
"ethtool -a"? You know that a typical switching chip will report the
same statistics counters on all ports, including the ones that do have a
net_device, right?  So DSA gets a waiver from implementing
.get_pause_stats()?

> but those are only useful for validating that the configuration of the
> CPU port is not completely broken. Otherwise the counters are
> symmetrical. A day-to-day user of the device doesn't need to see both
> of them.

A day-to-day user shouldn't need to look at ethtool -S or any other
statistics for that matter, either. If they need to look at flow control
on the CPU port they'd better get the full story rather than half of it.

Sorry for the non-constructive answer. Like Florian said, it would be
nice to have some built-in mechanism for this new ndo that DSA could use
to keep annotating its own stats.

Thanks,
-Vladimir