[PATCH net-next] team: add ethtool support

[PATCH net-next] team: add ethtool support

[Flavio Leitner]

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(+)

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;
-- 
1.8.0.1

Re: [PATCH net-next] team: add ethtool support

[Stephen Hemminger]

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.

Re: [PATCH net-next] team: add ethtool support

[David Miller]

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.

Re: [PATCH net-next] team: add ethtool support

[Flavio Leitner]

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. 

-- 
fbl

Re: [PATCH net-next] team: add ethtool support

[David Miller]

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.

Re: [PATCH net-next] team: add ethtool support

[Flavio Leitner]

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,
-- 
fbl

Re: [PATCH net-next] team: add ethtool support

[Eric Dumazet]

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

Re: [PATCH net-next] team: add ethtool support

[Ben Hutchings]

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.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] team: add ethtool support

[Stephen Hemminger]

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 ??