On 9/14/2020 8:53 AM, Jakub Kicinski wrote:
On Fri, 11 Sep 2020 19:54:11 -0700 Florian Fainelli wrote:
I think I'm missing the problem you're trying to describe.
Are you making a general comment / argument on ethtool stats?
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.
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.
You can still append your custom CPU port stats to ethtool -S or
debugfs or whatnot, 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.
It would be a lot easier to append the stats if there was not an
additional ndo introduce to fetch the pause statistics because DSA
overlay ndo on a function by function basis. Alternatively we should
consider ethtool netlink operations over a devlink port at some point so
we can get rid of the ugly ndo and ethtool_ops overlay that DSA does.
I'm trying to target the 99.9% of users here, so in all honesty I'm
concerned that if we try to cater to strange corner cases too much
the entire interface will suffer. Hence I decided not to go with
devlink, but extend the API people already know and use. It's the most
logical and obvious place to me.
Can we consider using get_ethtool_stats and ETH_SS_PAUSE_STATS as a
stringset identifier? That way there is a single point within driver to
fetch stats.
Can you say more? There are no strings reported in this patch set.
What I am suggesting is that we have a central and unique method for
drivers to be called for all ethtool statisitcs to be obtained, and not
create another ethtool operation specifically for pause stats.
Today we have get_ethtool_stats and a stringset argument that tells you
which type of statistic to return. I am not suggesting that we return
strings or that it should be necessary for fetching pause stats.
--
Florian
