| Message ID | 1524016176-3881-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
On 04/17/2018 06:49 PM, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > This is similar to ETHTOOL_GSTATS, but it allows you to specify > flags. These flags can be used by the driver to decrease the > amount of stats refreshed. In particular, this helps with ath10k > since getting the firmware stats can be slow. You can configure the statistics refresh rate through the ethtool coalescing parameter stats_block_coalesce_usecs, not sure if this is exactly what you had in mind, but if it works, then you might as well want to use it. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > include/linux/ethtool.h | 12 ++++++++++++ > include/uapi/linux/ethtool.h | 10 ++++++++++ > net/core/ethtool.c | 40 +++++++++++++++++++++++++++++++++++----- > 3 files changed, 57 insertions(+), 5 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index ebe4181..a4aa11f 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -243,6 +243,15 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32, > * @get_ethtool_stats: Return extended statistics about the device. > * This is only useful if the device maintains statistics not > * included in &struct rtnl_link_stats64. > + * @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. > * @begin: Function to be called before any other operation. Returns a > * negative error code or zero. > * @complete: Function to be called after any other operation except > @@ -355,6 +364,9 @@ struct ethtool_ops { > int (*set_phys_id)(struct net_device *, enum ethtool_phys_id_state); > void (*get_ethtool_stats)(struct net_device *, > struct ethtool_stats *, u64 *); > + void (*get_ethtool_stats2)(struct net_device *dev, > + struct ethtool_stats *gstats, u64 *data, > + u32 flags); > int (*begin)(struct net_device *); > void (*complete)(struct net_device *); > u32 (*get_priv_flags)(struct net_device *); > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h > index 4ca65b5..1c74f3e 100644 > --- a/include/uapi/linux/ethtool.h > +++ b/include/uapi/linux/ethtool.h > @@ -1396,11 +1396,21 @@ enum ethtool_fec_config_bits { > #define ETHTOOL_PHY_STUNABLE 0x0000004f /* Set PHY tunable configuration */ > #define ETHTOOL_GFECPARAM 0x00000050 /* Get FEC settings */ > #define ETHTOOL_SFECPARAM 0x00000051 /* Set FEC settings */ > +#define ETHTOOL_GSTATS2 0x00000052 /* get NIC-specific statistics > + * with ability to specify flags. > + * See ETHTOOL_GS2* below. > + */ > > /* compatibility with older code */ > #define SPARC_ETH_GSET ETHTOOL_GSET > #define SPARC_ETH_SSET ETHTOOL_SSET > > +/* GSTATS2 flags */ > +#define ETHTOOL_GS2_SKIP_NONE (0) /* default is to update all stats */ > +#define ETHTOOL_GS2_SKIP_FW (1<<0) /* Skip reading stats that probe firmware, > + * and thus are slow/expensive. > + */ > + > /* Link mode bit indices */ > enum ethtool_link_mode_bit_indices { > ETHTOOL_LINK_MODE_10baseT_Half_BIT = 0, > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 03416e6..6ec3413 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1952,16 +1952,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr) > return rc; > } > > -static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) > +static int _ethtool_get_stats(struct net_device *dev, void __user *useraddr, > + u32 flags) > { > struct ethtool_stats stats; > const struct ethtool_ops *ops = dev->ethtool_ops; > u64 *data; > int ret, n_stats; > > - if (!ops->get_ethtool_stats || !ops->get_sset_count) > - return -EOPNOTSUPP; > - > n_stats = ops->get_sset_count(dev, ETH_SS_STATS); > if (n_stats < 0) > return n_stats; > @@ -1976,7 +1974,10 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) > if (n_stats && !data) > return -ENOMEM; > > - ops->get_ethtool_stats(dev, &stats, data); > + if (flags != ETHTOOL_GS2_SKIP_NONE) > + ops->get_ethtool_stats2(dev, &stats, data, flags); > + else > + ops->get_ethtool_stats(dev, &stats, data); > > ret = -EFAULT; > if (copy_to_user(useraddr, &stats, sizeof(stats))) > @@ -1991,6 +1992,31 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) > return ret; > } > > +static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) > +{ > + const struct ethtool_ops *ops = dev->ethtool_ops; > + > + if (!ops->get_ethtool_stats || !ops->get_sset_count) > + return -EOPNOTSUPP; > + return _ethtool_get_stats(dev, useraddr, ETHTOOL_GS2_SKIP_NONE); > +} > + > +static int ethtool_get_stats2(struct net_device *dev, void __user *useraddr) > +{ > + struct ethtool_stats stats; > + const struct ethtool_ops *ops = dev->ethtool_ops; > + u32 flags = 0; > + > + if (!ops->get_ethtool_stats2 || !ops->get_sset_count) > + return -EOPNOTSUPP; > + > + if (copy_from_user(&stats, useraddr, sizeof(stats))) > + return -EFAULT; > + > + flags = stats.n_stats; > + return _ethtool_get_stats(dev, useraddr, flags); > +} > + > static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr) > { > struct ethtool_stats stats; > @@ -2632,6 +2658,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > case ETHTOOL_GSSET_INFO: > case ETHTOOL_GSTRINGS: > case ETHTOOL_GSTATS: > + case ETHTOOL_GSTATS2: > case ETHTOOL_GPHYSTATS: > case ETHTOOL_GTSO: > case ETHTOOL_GPERMADDR: > @@ -2742,6 +2769,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > case ETHTOOL_GSTATS: > rc = ethtool_get_stats(dev, useraddr); > break; > + case ETHTOOL_GSTATS2: > + rc = ethtool_get_stats2(dev, useraddr); > + break; > case ETHTOOL_GPERMADDR: > rc = ethtool_get_perm_addr(dev, useraddr); > break; >
On Tue, 2018-04-17 at 18:49 -0700, greearb@candelatech.com wrote: > > + * @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. It'd be pretty hard to know which flags are firmware stats? Anyway, there's no way I'm going to take this patch, so you need to float it on netdev first (best CC us here) and get it applied there before we can do anything on the wifi side. johannes
On 04/18/2018 02:26 PM, Johannes Berg wrote: > On Tue, 2018-04-17 at 18:49 -0700, greearb@candelatech.com wrote: >> >> + * @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. > > 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. In my case, I have lots of virtual stations (or APs), and I want stats for them as well as for the 'radio', so I would probe the first vdev with flags of 'skip-none' to get all stats, including radio (firmware) stats. And then the rest I would just probe the non-firmware stats. To be honest, I was slightly amused that anyone expressed interest in this patch originally, but maybe other people have similar use case and/or drivers with slow-to-acquire stats. > Anyway, there's no way I'm going to take this patch, so you need to > float it on netdev first (best CC us here) and get it applied there > before we can do anything on the wifi side. I posted the patches to netdev, ath10k and linux-wireless. If I had only posted them individually to different lists I figure I'd be hearing about how the netdev patch is useless because it has no driver support, etc. Thanks, Ben
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. 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. > I posted the patches to netdev, ath10k and linux-wireless. If I had only > posted them individually to different lists I figure I'd be hearing about how > the netdev patch is useless because it has no driver support, etc. Sure. I missed netdev, perhaps because it was in To, or more likely because I was too sleepy. Sorry for the noise. johannes
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
On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote: > > 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. That's a good point. > Maybe the flag could be called: ETHTOOL_GS2_NO_REFRESH_FW ? Sounds more to the point to me, yeah. > > > > 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 :) I guess you'll have to ask davem. :) johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: Thu, 19 Apr 2018 17:26:57 +0200 > On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote: >> >> 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 :) > > I guess you'll have to ask davem. :) Well, first of all, I really don't like this. The first reason is that every time I see interface foo become foo2, foo3 is never far behind it. If foo was not extensible enough such that we needed foo2, we beter design the new thing with explicitly better extensibility in mind. Furthermore, what you want here is a specific filter. Someone else will want to filter on another criteria, and the next person will want yet another. This needs to be properly generalized. And frankly if we had moved to ethtool netlink/devlink by now, we could just add a netlink attribute for filtering and not even be having this conversation.
On Sun, Apr 22, 2018 at 11:54 AM, David Miller <davem@davemloft.net> wrote: > From: Johannes Berg <johannes@sipsolutions.net> > Date: Thu, 19 Apr 2018 17:26:57 +0200 > >> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote: >>> >>> 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 :) >> >> I guess you'll have to ask davem. :) > > Well, first of all, I really don't like this. > > The first reason is that every time I see interface foo become foo2, > foo3 is never far behind it. > > If foo was not extensible enough such that we needed foo2, we beter > design the new thing with explicitly better extensibility in mind. > > Furthermore, what you want here is a specific filter. Someone else > will want to filter on another criteria, and the next person will > want yet another. > > This needs to be properly generalized. > > And frankly if we had moved to ethtool netlink/devlink by now, we > could just add a netlink attribute for filtering and not even be > having this conversation. +1. Also, the RTM_GETSTATS api was added to improve stats query efficiency (with filters). we should look at it to see if this fits there. Keeping all stats queries in one place will help.
On 04/22/2018 11:54 AM, David Miller wrote: > From: Johannes Berg <johannes@sipsolutions.net> > Date: Thu, 19 Apr 2018 17:26:57 +0200 > >> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote: >>> >>> 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 :) >> >> I guess you'll have to ask davem. :) > > Well, first of all, I really don't like this. > > The first reason is that every time I see interface foo become foo2, > foo3 is never far behind it. > > If foo was not extensible enough such that we needed foo2, we beter > design the new thing with explicitly better extensibility in mind. > > Furthermore, what you want here is a specific filter. Someone else > will want to filter on another criteria, and the next person will > want yet another. > > This needs to be properly generalized. > > And frankly if we had moved to ethtool netlink/devlink by now, we > could just add a netlink attribute for filtering and not even be > having this conversation. Well, since there are un-defined flags, it would be simple enough to extend the API further in the future (flag (1<<31) could mean expect more input members, etc. And, adding up to 30 more flags to filter on different things won't change the API and should be backwards compatible. But, if you don't want it, that is OK by me, I agree it is a fairly obscure feature. It would have saved me time if you had said you didn't want it at the first RFC patch though... Thanks, Ben
On 04/22/2018 02:15 PM, Roopa Prabhu wrote: > On Sun, Apr 22, 2018 at 11:54 AM, David Miller <davem@davemloft.net> wrote: >> From: Johannes Berg <johannes@sipsolutions.net> >> Date: Thu, 19 Apr 2018 17:26:57 +0200 >> >>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote: >>>> >>>> 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 :) >>> >>> I guess you'll have to ask davem. :) >> >> Well, first of all, I really don't like this. >> >> The first reason is that every time I see interface foo become foo2, >> foo3 is never far behind it. >> >> If foo was not extensible enough such that we needed foo2, we beter >> design the new thing with explicitly better extensibility in mind. >> >> Furthermore, what you want here is a specific filter. Someone else >> will want to filter on another criteria, and the next person will >> want yet another. >> >> This needs to be properly generalized. >> >> And frankly if we had moved to ethtool netlink/devlink by now, we >> could just add a netlink attribute for filtering and not even be >> having this conversation. > > > +1. > > Also, the RTM_GETSTATS api was added to improve stats query efficiency > (with filters). > we should look at it to see if this fits there. Keeping all stats > queries in one place will help. I like the ethtool API, so I'll be sticking with that for now. Thanks, Ben
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index ebe4181..a4aa11f 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -243,6 +243,15 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32, * @get_ethtool_stats: Return extended statistics about the device. * This is only useful if the device maintains statistics not * included in &struct rtnl_link_stats64. + * @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. * @begin: Function to be called before any other operation. Returns a * negative error code or zero. * @complete: Function to be called after any other operation except @@ -355,6 +364,9 @@ struct ethtool_ops { int (*set_phys_id)(struct net_device *, enum ethtool_phys_id_state); void (*get_ethtool_stats)(struct net_device *, struct ethtool_stats *, u64 *); + void (*get_ethtool_stats2)(struct net_device *dev, + struct ethtool_stats *gstats, u64 *data, + u32 flags); int (*begin)(struct net_device *); void (*complete)(struct net_device *); u32 (*get_priv_flags)(struct net_device *); diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 4ca65b5..1c74f3e 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -1396,11 +1396,21 @@ enum ethtool_fec_config_bits { #define ETHTOOL_PHY_STUNABLE 0x0000004f /* Set PHY tunable configuration */ #define ETHTOOL_GFECPARAM 0x00000050 /* Get FEC settings */ #define ETHTOOL_SFECPARAM 0x00000051 /* Set FEC settings */ +#define ETHTOOL_GSTATS2 0x00000052 /* get NIC-specific statistics + * with ability to specify flags. + * See ETHTOOL_GS2* below. + */ /* compatibility with older code */ #define SPARC_ETH_GSET ETHTOOL_GSET #define SPARC_ETH_SSET ETHTOOL_SSET +/* GSTATS2 flags */ +#define ETHTOOL_GS2_SKIP_NONE (0) /* default is to update all stats */ +#define ETHTOOL_GS2_SKIP_FW (1<<0) /* Skip reading stats that probe firmware, + * and thus are slow/expensive. + */ + /* Link mode bit indices */ enum ethtool_link_mode_bit_indices { ETHTOOL_LINK_MODE_10baseT_Half_BIT = 0, diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 03416e6..6ec3413 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1952,16 +1952,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr) return rc; } -static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) +static int _ethtool_get_stats(struct net_device *dev, void __user *useraddr, + u32 flags) { struct ethtool_stats stats; const struct ethtool_ops *ops = dev->ethtool_ops; u64 *data; int ret, n_stats; - if (!ops->get_ethtool_stats || !ops->get_sset_count) - return -EOPNOTSUPP; - n_stats = ops->get_sset_count(dev, ETH_SS_STATS); if (n_stats < 0) return n_stats; @@ -1976,7 +1974,10 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) if (n_stats && !data) return -ENOMEM; - ops->get_ethtool_stats(dev, &stats, data); + if (flags != ETHTOOL_GS2_SKIP_NONE) + ops->get_ethtool_stats2(dev, &stats, data, flags); + else + ops->get_ethtool_stats(dev, &stats, data); ret = -EFAULT; if (copy_to_user(useraddr, &stats, sizeof(stats))) @@ -1991,6 +1992,31 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) return ret; } +static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) +{ + const struct ethtool_ops *ops = dev->ethtool_ops; + + if (!ops->get_ethtool_stats || !ops->get_sset_count) + return -EOPNOTSUPP; + return _ethtool_get_stats(dev, useraddr, ETHTOOL_GS2_SKIP_NONE); +} + +static int ethtool_get_stats2(struct net_device *dev, void __user *useraddr) +{ + struct ethtool_stats stats; + const struct ethtool_ops *ops = dev->ethtool_ops; + u32 flags = 0; + + if (!ops->get_ethtool_stats2 || !ops->get_sset_count) + return -EOPNOTSUPP; + + if (copy_from_user(&stats, useraddr, sizeof(stats))) + return -EFAULT; + + flags = stats.n_stats; + return _ethtool_get_stats(dev, useraddr, flags); +} + static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr) { struct ethtool_stats stats; @@ -2632,6 +2658,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) case ETHTOOL_GSSET_INFO: case ETHTOOL_GSTRINGS: case ETHTOOL_GSTATS: + case ETHTOOL_GSTATS2: case ETHTOOL_GPHYSTATS: case ETHTOOL_GTSO: case ETHTOOL_GPERMADDR: @@ -2742,6 +2769,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) case ETHTOOL_GSTATS: rc = ethtool_get_stats(dev, useraddr); break; + case ETHTOOL_GSTATS2: + rc = ethtool_get_stats2(dev, useraddr); + break; case ETHTOOL_GPERMADDR: rc = ethtool_get_perm_addr(dev, useraddr); break;