| Message ID | 1356830366-991-1-git-send-email-fbl@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 29 Dec 2012 23:19:26 -0200 Flavio Leitner <fbl@redhat.com> wrote: > This patch adds few ethtool operations to team driver. > > Signed-off-by: Flavio Leitner <fbl@redhat.com> What is the motivation for this? Is there an application that depends on ethtool (versus netlink, or /proc)? Sorry, I see no point in providing ethtool statistics for generic data that is already reported by existing netlink and other infrastructure. The purpose of ethtool statistics is to report device specific that is not available through the normal generic statistics. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stephen Hemminger <shemminger@vyatta.com> Date: Sat, 29 Dec 2012 17:29:45 -0800 > On Sat, 29 Dec 2012 23:19:26 -0200 > Flavio Leitner <fbl@redhat.com> wrote: > >> This patch adds few ethtool operations to team driver. >> >> Signed-off-by: Flavio Leitner <fbl@redhat.com> > > What is the motivation for this? Is there an application that depends > on ethtool (versus netlink, or /proc)? > > Sorry, I see no point in providing ethtool statistics for generic data that is already > reported by existing netlink and other infrastructure. The purpose of ethtool > statistics is to report device specific that is not available through the normal > generic statistics. Agreed, ethtool stats should _ONLY_ report device specific statistics. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 29, 2012 at 05:29:45PM -0800, Stephen Hemminger wrote: > On Sat, 29 Dec 2012 23:19:26 -0200 > Flavio Leitner <fbl@redhat.com> wrote: > > > This patch adds few ethtool operations to team driver. > > > > Signed-off-by: Flavio Leitner <fbl@redhat.com> > > What is the motivation for this? Is there an application that depends > on ethtool (versus netlink, or /proc)? Speaking as a support engineer, it's a lot easier to grab ethtool -S and see everything than grab two or more outputs. > Sorry, I see no point in providing ethtool statistics for generic data that is already > reported by existing netlink and other infrastructure. The purpose of ethtool > statistics is to report device specific that is not available through the normal > generic statistics. Right, but those statistics can be device specific as well. The tg3 and bnx2, for instance, do the same reporting [rx|tx]_bytes|octets. I see no harm, and it is helpful.
From: Flavio Leitner <fbl@redhat.com> Date: Sat, 29 Dec 2012 23:44:03 -0200 > Right, but those statistics can be device specific as well. The tg3 and bnx2, for > instance, do the same reporting [rx|tx]_bytes|octets. Because those ARE PER QUEUE in multiqueue configurations, and thus device specific. There is no reason to report the bare single-queue generic netdevice stats via ethtool. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 29, 2012 at 06:09:30PM -0800, David Miller wrote: > From: Flavio Leitner <fbl@redhat.com> > Date: Sat, 29 Dec 2012 23:44:03 -0200 > > > Right, but those statistics can be device specific as well. The tg3 and bnx2, for > > instance, do the same reporting [rx|tx]_bytes|octets. > > Because those ARE PER QUEUE in multiqueue configurations, and thus > device specific. > > There is no reason to report the bare single-queue generic > netdevice stats via ethtool. Alright, I will post another version without the statistics. please, drop this one. thanks,
On Sat, 2012-12-29 at 23:44 -0200, Flavio Leitner wrote: > Speaking as a support engineer, it's a lot easier to grab ethtool -S and > see everything than grab two or more outputs. > I agree its very convenient. I have a patch to add GRO statistics at the core layer, in the ethtool -S stats. I was about to ask netdev guys what they think of this idea ? net-gro: Add GRO counters to ethtool -S In order to get an idea of how effective is GRO aggregation on a machine, we need appropriate counters. Preferably use "ethtool -S" to display them on a per device basis, or even per RX queue. In this implementation, I chose to not change NIC drivers. Core network stack adds the gro counters at the end of the counters each NIC driver provides for ethtool -S There are 5 counters per RX queue : gro_complete: number of time the NAPI handler did not consume its budget (This force a flush of all GRO packets in the GRO queue) gro_overflows: number of time a segment could not be stored in GRO queue because current number or messages is too high gro_nogro: number of time a segment was not stored in GRO queue. (Because its not a TCP packet, or it includes a SYN/FIN/RST/PSH flag) gro_msgs: number of GRO messages (might contain 1 to 17 segments) gro_segs: number of GRO segments Example: On receiver machine, with 8 RX queues : ethtool -S eth4 | tail -n 10 gro_complete[7]: 56635 gro_overflows[7]: 0 gro_nogro[7]: 212 gro_msgs[7]: 129410 gro_segs[7]: 1434925 gro_complete: 699479 gro_overflows: 0 gro_nogro: 2455 gro_msgs: 1626470 gro_segs: 17876794 In this example, we can compute average number of segments per GRO message : 17876794/17876794 = 10.99 Or more precisely : 17876794/(17876794+2455) = 10.97 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2012-12-29 at 18:31 -0800, Eric Dumazet wrote: > On Sat, 2012-12-29 at 23:44 -0200, Flavio Leitner wrote: > > > Speaking as a support engineer, it's a lot easier to grab ethtool -S and > > see everything than grab two or more outputs. > > > > I agree its very convenient. > > I have a patch to add GRO statistics at the core layer, in the ethtool > -S stats. > > I was about to ask netdev guys what they think of this idea ? [...] I would like to make these statistics available *but* I would prefer to see a plan for properly integrating the basic statistics (net_device_stats/rtnl_link_stats64), additional core statistics (such as these) and driver-specific statistics. It's annoying that users have to use different tools to get these, and adding core statistics to ethtool (currently driver-specific) is just going to be more confusing. Ben.
On Sat, 29 Dec 2012 18:31:56 -0800 Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Sat, 2012-12-29 at 23:44 -0200, Flavio Leitner wrote: > > > Speaking as a support engineer, it's a lot easier to grab ethtool -S and > > see everything than grab two or more outputs. > > > > I agree its very convenient. > > I have a patch to add GRO statistics at the core layer, in the ethtool > -S stats. > > I was about to ask netdev guys what they think of this idea ? > > > net-gro: Add GRO counters to ethtool -S > > In order to get an idea of how effective is GRO aggregation on a machine, > we need appropriate counters. Preferably use "ethtool -S" to display them > on a per device basis, or even per RX queue. > > In this implementation, I chose to not change NIC drivers. > > Core network stack adds the gro counters at the end of the counters > each NIC driver provides for ethtool -S > > There are 5 counters per RX queue : > > gro_complete: number of time the NAPI handler did not consume its budget > (This force a flush of all GRO packets in the GRO queue) > gro_overflows: number of time a segment could not be stored in GRO queue > because current number or messages is too high > gro_nogro: number of time a segment was not stored in GRO queue. > (Because its not a TCP packet, or it includes a > SYN/FIN/RST/PSH flag) > gro_msgs: number of GRO messages (might contain 1 to 17 segments) > gro_segs: number of GRO segments > > Example: > > On receiver machine, with 8 RX queues : > > ethtool -S eth4 | tail -n 10 > gro_complete[7]: 56635 > gro_overflows[7]: 0 > gro_nogro[7]: 212 > gro_msgs[7]: 129410 > gro_segs[7]: 1434925 > gro_complete: 699479 > gro_overflows: 0 > gro_nogro: 2455 > gro_msgs: 1626470 > gro_segs: 17876794 > > In this example, we can compute average number of segments per GRO message : > > 17876794/17876794 = 10.99 > > Or more precisely : 17876794/(17876794+2455) = 10.97 > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Ethtool is awkward because it is only available through ioctl, no netlink or /proc. If you use ethtool for GRO, please make it a separate ioctl not an add-on to existing device statistics. ethtool --gro ?? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c index ad86660..f711039 100644 --- a/drivers/net/team/team.c +++ b/drivers/net/team/team.c @@ -28,6 +28,7 @@ #include <net/genetlink.h> #include <net/netlink.h> #include <net/sch_generic.h> +#include <generated/utsrelease.h> #include <linux/if_team.h> #define DRV_NAME "team" @@ -1731,6 +1732,75 @@ static const struct net_device_ops team_netdev_ops = { .ndo_fix_features = team_fix_features, }; +/*********************** + * ethtool interface + ***********************/ + +static const char ethtool_stats_keys[][ETH_GSTRING_LEN] = { + "rx_packets", + "rx_bytes", + "rx_dropped", + "tx_packets", + "tx_bytes", + "tx_dropped", + "multicast", +}; + +#define TEAM_NUM_STATS ARRAY_SIZE(ethtool_stats_keys) + +static int team_get_sset_count(struct net_device *netdev, int sset) +{ + switch (sset) { + case ETH_SS_STATS: + return TEAM_NUM_STATS; + default: + return -EOPNOTSUPP; + } +} + +static void team_get_strings(struct net_device *netdev, u32 stringset, u8 *data) +{ + switch (stringset) { + case ETH_SS_STATS: + memcpy(data, *ethtool_stats_keys, sizeof(ethtool_stats_keys)); + break; + } +} + +static void team_get_ethtool_stats(struct net_device *netdev, + struct ethtool_stats *stats, + u64 *data) +{ + struct rtnl_link_stats64 net_stats; + int i; + + memset(&net_stats, 0, sizeof(struct rtnl_link_stats64)); + team_get_stats64(netdev, &net_stats); + i = 0; + /* ordering based on ethtool_stats_keys */ + data[i++] = net_stats.rx_packets; + data[i++] = net_stats.rx_bytes; + data[i++] = net_stats.rx_dropped; + data[i++] = net_stats.tx_packets; + data[i++] = net_stats.tx_bytes; + data[i++] = net_stats.tx_dropped; + data[i++] = net_stats.multicast; +} + +static void team_ethtool_get_drvinfo(struct net_device *dev, + struct ethtool_drvinfo *drvinfo) +{ + strncpy(drvinfo->driver, DRV_NAME, 32); + strncpy(drvinfo->version, UTS_RELEASE, 32); +} + +static const struct ethtool_ops team_ethtool_ops = { + .get_drvinfo = team_ethtool_get_drvinfo, + .get_link = ethtool_op_get_link, + .get_strings = team_get_strings, + .get_ethtool_stats = team_get_ethtool_stats, + .get_sset_count = team_get_sset_count, +}; /*********************** * rt netlink interface @@ -1780,6 +1850,7 @@ static void team_setup(struct net_device *dev) ether_setup(dev); dev->netdev_ops = &team_netdev_ops; + dev->ethtool_ops = &team_ethtool_ops; dev->destructor = team_destructor; dev->tx_queue_len = 0; dev->flags |= IFF_MULTICAST;
This patch adds few ethtool operations to team driver. Signed-off-by: Flavio Leitner <fbl@redhat.com> --- drivers/net/team/team.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+)