On 04/18/2018 11:38 PM, Johannes Berg wrote:
On Wed, 2018-04-18 at 14:51 -0700, Ben Greear wrote:
It'd be pretty hard to know which flags are firmware stats?
Yes, it is, but ethtool stats are difficult to understand in a generic
manner anyway, so someone using them is already likely aware of low-level
details of the driver(s) they are using.
Right. Come to think of it though,
+ * @get_ethtool_stats2: Return extended statistics about the device.
+ * This is only useful if the device maintains statistics not
+ * included in &struct rtnl_link_stats64.
+ * Takes a flags argument: 0 means all (same as get_ethtool_stats),
+ * 0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
+ * Other flags are reserved for now.
+ * Same number of stats will be returned, but some of them might
+ * not be as accurate/refreshed. This is to allow not querying
+ * firmware or other expensive-to-read stats, for instance.
"skip" vs. "don't refresh" is a bit ambiguous - I'd argue better to
either really skip and not return the non-refreshed ones (also helps
with the identifying), or rename the flag.
In order to efficiently parse lots of stats over and over again, I probe
the stat names once on startup, map them to the variable I am trying to use
(since different drivers may have different names for the same basic stat),
and then I store the stat index.
On subsequent stat reads, I just grab stats and go right to the index to
store the stat.
If the stats indexes change, that will complicate my logic quite a bit.
Maybe the flag could be called: ETHTOOL_GS2_NO_REFRESH_FW ?
Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
write the spatch and just add the flags argument to "get_ethtool_stats"
instead of adding a separate method - internally to the kernel it's not
that hard to change.
Maybe this could be in followup patches? It's going to touch a lot of files,
and might be hell to get merged all at once, and I've never used spatch, so
just maybe someone else will volunteer that part :)
Thanks,
Ben
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
