netdev - Re: [PATCH 1/3] ethtool: Support ETHTOOL

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: <173c5f98-36bc-2e52-1e64-3a5f89008d46@candelatech.com>
Date:   Thu, 19 Apr 2018 08:25:41 -0700
From:   Ben Greear <greearb@...delatech.com>
To:     Johannes Berg <johannes@...solutions.net>, netdev@...r.kernel.org
Cc:     linux-wireless@...r.kernel.org, ath10k@...ts.infradead.org
Subject: Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

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@...delatech.com>
Candela Technologies Inc  http://www.candelatech.com