From: Ben Greear <greearb@candelatech.com> To: Johannes Berg <johannes@sipsolutions.net>, netdev@vger.kernel.org Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org Subject: Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command. Date: Thu, 19 Apr 2018 08:25:41 -0700 [thread overview] Message-ID: <173c5f98-36bc-2e52-1e64-3a5f89008d46@candelatech.com> (raw) In-Reply-To: <1524119910.3024.12.camel@sipsolutions.net> 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@candelatech.com> Candela Technologies Inc http://www.candelatech.com
WARNING: multiple messages have this Message-ID (diff)
From: Ben Greear <greearb@candelatech.com> To: Johannes Berg <johannes@sipsolutions.net>, netdev@vger.kernel.org Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org Subject: Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command. Date: Thu, 19 Apr 2018 08:25:41 -0700 [thread overview] Message-ID: <173c5f98-36bc-2e52-1e64-3a5f89008d46@candelatech.com> (raw) In-Reply-To: <1524119910.3024.12.camel@sipsolutions.net> 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@candelatech.com> Candela Technologies Inc http://www.candelatech.com _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2018-04-19 15:25 UTC|newest] Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-18 1:49 [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command greearb-my8/4N5VtI7c+919tysfdA 2018-04-18 1:49 ` greearb 2018-04-18 1:49 ` greearb 2018-04-18 1:49 ` [PATCH 2/3] mac80211: Add support for ethtool gstats2 API greearb 2018-04-18 1:49 ` greearb 2018-04-18 1:49 ` [PATCH 3/3] ath10k: Support " greearb 2018-04-18 1:49 ` greearb 2018-04-19 11:19 ` kbuild test robot 2018-04-19 11:19 ` kbuild test robot 2018-04-18 21:07 ` [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command Florian Fainelli 2018-04-18 21:07 ` Florian Fainelli 2018-04-18 21:26 ` Johannes Berg 2018-04-18 21:26 ` Johannes Berg 2018-04-18 21:51 ` Ben Greear 2018-04-18 21:51 ` Ben Greear 2018-04-19 6:38 ` Johannes Berg 2018-04-19 6:38 ` Johannes Berg 2018-04-19 15:25 ` Ben Greear [this message] 2018-04-19 15:25 ` Ben Greear [not found] ` <173c5f98-36bc-2e52-1e64-3a5f89008d46-my8/4N5VtI7c+919tysfdA@public.gmane.org> 2018-04-19 15:26 ` Johannes Berg 2018-04-19 15:26 ` Johannes Berg 2018-04-19 15:26 ` Johannes Berg [not found] ` <1524151617.3024.25.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> 2018-04-22 18:54 ` David Miller 2018-04-22 18:54 ` David Miller 2018-04-22 18:54 ` David Miller [not found] ` <20180422.145420.1197041027922699603.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> 2018-04-22 21:15 ` Roopa Prabhu 2018-04-22 21:15 ` Roopa Prabhu 2018-04-22 21:15 ` Roopa Prabhu 2018-04-23 15:41 ` Ben Greear 2018-04-23 15:41 ` Ben Greear 2018-04-23 15:38 ` Ben Greear 2018-04-23 15:38 ` Ben Greear 2018-04-23 15:38 ` Ben Greear
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=173c5f98-36bc-2e52-1e64-3a5f89008d46@candelatech.com \ --to=greearb@candelatech.com \ --cc=ath10k@lists.infradead.org \ --cc=johannes@sipsolutions.net \ --cc=linux-wireless@vger.kernel.org \ --cc=netdev@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.