[patch] add net_device_stats support to ethtool

[patch] add net_device_stats support to ethtool

[Dan Nicolaescu]

Hi,

"ethtool -S" only supports devices that have custom code written to
print the stats. A lot of drivers use "struct net_device_stats", so
adding code to ethtool would make it very easy for such drivers to
add support for "ethtool -S". The drivers would just need to add this: 

	.get_strings	   = ethtool_op_net_device_stats_get_strings,
	.get_stats_count   = ethtool_op_net_device_stats_get_stats_count,
	.get_ethtool_stats = ethtool_op_net_device_get_ethtool_stats,

 to their struct ethtool_ops


(The function names are not the best... Suggestions for better ones
are welcome)

Is there any interest to have this in the kernel? 

The patch contains straightforward implementation for the
.get_strings, .get_stats_count and .get_ethtool_stats ethtool_ops methods.

Thanks
        --Dan

--- ethtool.c~	2004-12-24 13:35:50.000000000 -0800
+++ ethtool.c	2006-12-01 08:55:26.000000000 -0800
@@ -809,6 +809,64 @@
 	return -EOPNOTSUPP;
 }
 
+
+#define NET_DEVICE_NUM_STATS (sizeof(struct net_device_stats) / sizeof(unsigned long))
+
+static struct {
+	const char string[ETH_GSTRING_LEN];
+} ethtool_net_device_stats_keys[NET_DEVICE_NUM_STATS] = {
+	{ "rx_packets"},	 
+	{ "tx_packets"},	 
+	{ "rx_bytes"},	 
+	{ "tx_bytes"},	 
+	{ "rx_errors"},	 
+	{ "tx_errors"},	 
+	{ "rx_dropped"},	 
+	{ "tx_dropped"},	 
+	{ "multicast"},	 
+	{ "collisions"},
+	{ "rx_length_errors"},
+	{ "rx_over_errors"},	 
+	{ "rx_crc_errors"},	 
+	{ "rx_frame_errors"}, 
+	{ "rx_fifo_errors"},	 
+	{ "rx_missed_errors"},
+	{ "tx_aborted_errors"},
+	{ "tx_carrier_errors"},
+	{ "tx_fifo_errors"},
+	{ "tx_heartbeat_errors"},
+	{ "tx_window_errors"},
+	{ "rx_compressed"},
+	{ "tx_compressed"}
+};
+
+int ethtool_op_net_device_stats_get_stats_count(struct net_device *dev)
+{
+	return NET_DEVICE_NUM_STATS;
+}
+
+void ethtool_op_net_device_stats_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
+{
+	switch (stringset) {
+	case ETH_SS_STATS:
+		memcpy(buf, &ethtool_net_device_stats_keys, sizeof(ethtool_net_device_stats_keys));
+		break;
+	default:
+		WARN_ON(1);	/* we need a WARN() */
+		break;
+	}
+}
+
+void ethtool_op_net_device_get_ethtool_stats(struct net_device *dev,
+					      struct ethtool_stats *estats, u64 *tmp_stats)
+{
+	u32 i;
+	u64 *dest = tmp_stats;
+	unsigned long *src = (unsigned long*)dev->get_stats(dev);
+	for (i = 0; i < estats->n_stats; i++)
+	  *dest++ = *src++;
+}
+
 EXPORT_SYMBOL(dev_ethtool);
 EXPORT_SYMBOL(ethtool_op_get_link);
 EXPORT_SYMBOL(ethtool_op_get_sg);
@@ -817,3 +875,6 @@
 EXPORT_SYMBOL(ethtool_op_set_sg);
 EXPORT_SYMBOL(ethtool_op_set_tso);
 EXPORT_SYMBOL(ethtool_op_set_tx_csum);
+EXPORT_SYMBOL(ethtool_op_net_device_stats_get_stats_count);
+EXPORT_SYMBOL(ethtool_op_net_device_stats_get_strings);
+EXPORT_SYMBOL(ethtool_op_net_device_get_ethtool_stats);
--- ethtool.h~	2006-09-05 12:29:45.000000000 -0700
+++ ethtool.h	2006-12-01 08:51:46.000000000 -0800
@@ -260,6 +260,12 @@
 int ethtool_op_set_sg(struct net_device *dev, u32 data);
 u32 ethtool_op_get_tso(struct net_device *dev);
 int ethtool_op_set_tso(struct net_device *dev, u32 data);
+int ethtool_op_net_device_stats_get_stats_count(struct net_device *dev);
+void ethtool_op_net_device_stats_get_strings(struct net_device *dev,
+					     u32 stringset, u8 *buf);
+void ethtool_op_net_device_get_ethtool_stats(struct net_device *dev,
+					     struct ethtool_stats *estats,
+					     u64 *tmp_stats);
 
 /**
  * &ethtool_ops - Alter and report network device settings

[patch] add net_device_stats support to ethtool

[Dan Nicolaescu]

Hi,

I have posted this patch in the past with absolutely no reply.
I would appreciate some sort of feedback of the form 
interested/not interested.  Should I just drop it?

"ethtool -S" only supports devices that have custom code written to
print the stats. 

A lot of drivers use "struct net_device_stats", so adding code to
ethtool would make it very easy for such drivers to add support for
"ethtool -S". The drivers would just need to add this:

	.get_strings	   = ethtool_op_net_device_stats_get_strings,
	.get_stats_count   = ethtool_op_net_device_stats_get_stats_count,
	.get_ethtool_stats = ethtool_op_net_device_get_ethtool_stats,

to their struct ethtool_ops. I found this very useful when debugging a
new driver.

(The function names are not the best, suggestions for better names are
welcome).

The code added is very small, and it gives easy access to stats that the
drivers are computing anyway.

This should save code in case the drivers the use net_device_stats
decide they want ethtool stats support.

The patch only adds, it does not modify any existing line of code.

Thanks
        --Dan

--- ethtool.c~	2004-12-24 13:35:50.000000000 -0800
+++ ethtool.c	2006-12-01 08:55:26.000000000 -0800
@@ -809,6 +809,64 @@
 	return -EOPNOTSUPP;
 }
 
+
+#define NET_DEVICE_NUM_STATS (sizeof(struct net_device_stats) / sizeof(unsigned long))
+
+static struct {
+	const char string[ETH_GSTRING_LEN];
+} ethtool_net_device_stats_keys[NET_DEVICE_NUM_STATS] = {
+	{ "rx_packets"},	 
+	{ "tx_packets"},	 
+	{ "rx_bytes"},	 
+	{ "tx_bytes"},	 
+	{ "rx_errors"},	 
+	{ "tx_errors"},	 
+	{ "rx_dropped"},	 
+	{ "tx_dropped"},	 
+	{ "multicast"},	 
+	{ "collisions"},
+	{ "rx_length_errors"},
+	{ "rx_over_errors"},	 
+	{ "rx_crc_errors"},	 
+	{ "rx_frame_errors"}, 
+	{ "rx_fifo_errors"},	 
+	{ "rx_missed_errors"},
+	{ "tx_aborted_errors"},
+	{ "tx_carrier_errors"},
+	{ "tx_fifo_errors"},
+	{ "tx_heartbeat_errors"},
+	{ "tx_window_errors"},
+	{ "rx_compressed"},
+	{ "tx_compressed"}
+};
+
+int ethtool_op_net_device_stats_get_stats_count(struct net_device *dev)
+{
+	return NET_DEVICE_NUM_STATS;
+}
+
+void ethtool_op_net_device_stats_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
+{
+	switch (stringset) {
+	case ETH_SS_STATS:
+		memcpy(buf, &ethtool_net_device_stats_keys, sizeof(ethtool_net_device_stats_keys));
+		break;
+	default:
+		WARN_ON(1);	/* we need a WARN() */
+		break;
+	}
+}
+
+void ethtool_op_net_device_get_ethtool_stats(struct net_device *dev,
+					      struct ethtool_stats *estats, u64 *tmp_stats)
+{
+	u32 i;
+	u64 *dest = tmp_stats;
+	unsigned long *src = (unsigned long*)dev->get_stats(dev);
+	for (i = 0; i < estats->n_stats; i++)
+	  *dest++ = *src++;
+}
+
 EXPORT_SYMBOL(dev_ethtool);
 EXPORT_SYMBOL(ethtool_op_get_link);
 EXPORT_SYMBOL(ethtool_op_get_sg);
@@ -817,3 +875,6 @@
 EXPORT_SYMBOL(ethtool_op_set_sg);
 EXPORT_SYMBOL(ethtool_op_set_tso);
 EXPORT_SYMBOL(ethtool_op_set_tx_csum);
+EXPORT_SYMBOL(ethtool_op_net_device_stats_get_stats_count);
+EXPORT_SYMBOL(ethtool_op_net_device_stats_get_strings);
+EXPORT_SYMBOL(ethtool_op_net_device_get_ethtool_stats);
--- ethtool.h~	2006-09-05 12:29:45.000000000 -0700
+++ ethtool.h	2006-12-01 08:51:46.000000000 -0800
@@ -260,6 +260,12 @@
 int ethtool_op_set_sg(struct net_device *dev, u32 data);
 u32 ethtool_op_get_tso(struct net_device *dev);
 int ethtool_op_set_tso(struct net_device *dev, u32 data);
+int ethtool_op_net_device_stats_get_stats_count(struct net_device *dev);
+void ethtool_op_net_device_stats_get_strings(struct net_device *dev,
+					     u32 stringset, u8 *buf);
+void ethtool_op_net_device_get_ethtool_stats(struct net_device *dev,
+					     struct ethtool_stats *estats,
+					     u64 *tmp_stats);
 
 /**
  * &ethtool_ops - Alter and report network device settings

Re: [patch] add net_device_stats support to ethtool

[Ben Greear]

Dan Nicolaescu wrote:
> Hi,
>
> I have posted this patch in the past with absolutely no reply.
> I would appreciate some sort of feedback of the form 
> interested/not interested.  Should I just drop it?
>
>   
I like it, but why not offer this for all devices since they all have 
these stats.
Could add new handlers called something like .get_strings_generic, or
just add this to the higher-level ethtool handling before it looks for 
handlers.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [patch] add net_device_stats support to ethtool

[Dan Nicolaescu]

Ben Greear <greearb@candelatech.com> writes:

  > Dan Nicolaescu wrote:
  > > Hi,
  > >
  > > I have posted this patch in the past with absolutely no reply.
  > > I would appreciate some sort of feedback of the form interested/not
  > > interested.  Should I just drop it?
  > >
  > >   
  > I like it, but why not offer this for all devices since they all have
  > these stats.
  >
  > Could add new handlers called something like .get_strings_generic, or
  > just add this to the higher-level ethtool handling before it looks for
  > handlers.

If I get your point, then the difference would be that drivers would add
to the initialization of the ethtool structure something like:

         .get_strings_generic = 1;
          
instead of what I originally proposed:

	.get_strings	   = ethtool_op_net_device_stats_get_strings,
	.get_stats_count   = ethtool_op_net_device_stats_get_stats_count,
	.get_ethtool_stats = ethtool_op_net_device_get_ethtool_stats,

Sure that could work, but it would require a few lines of changes in
ethtool.

I can submit a patch that does things that way, if that is considered
better.  But I would like to hear that this code is wanted before
putting any effort in it. I has been ignored for so long... 

Thanks

        --dan

Re: [patch] add net_device_stats support to ethtool

[Ben Greear]

Dan Nicolaescu wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>   > Dan Nicolaescu wrote:
>   > > Hi,
>   > >
>   > > I have posted this patch in the past with absolutely no reply.
>   > > I would appreciate some sort of feedback of the form interested/not
>   > > interested.  Should I just drop it?
>   > >
>   > >   
>   > I like it, but why not offer this for all devices since they all have
>   > these stats.
>   >
>   > Could add new handlers called something like .get_strings_generic, or
>   > just add this to the higher-level ethtool handling before it looks for
>   > handlers.
>
> If I get your point, then the difference would be that drivers would add
> to the initialization of the ethtool structure something like:
>   
I meant something more like this (this will not apply..I hand-edited it 
to remove
some extraneous crap from my patch...and I'm sure it's white-space damaged).

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c5e0593..095d1eb 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c

+/* Handle some generic ethtool commands here */
+static int ethtool_get_netdev_stats(struct net_device *dev, void 
*useraddr) {
+   
+    struct ethtool_ndstats* nds = (struct ethtool_ndstats*)(useraddr);
+     
+    struct net_device_stats *stats = dev->get_stats(dev);
+    if (stats) {
+        if (copy_to_user(nds->data, stats, sizeof(*stats))) {
+            return -EFAULT;
+        }
+    }
+    else {
+        return -EOPNOTSUPP;
+    }
+    return 0;
+}
+
+
 /* The main entry point in this file.  Called from net/core/dev.c */
 
 int dev_ethtool(struct ifreq *ifr)
@@ -796,9 +884,6 @@ int dev_ethtool(struct ifreq *ifr)
     if (!dev || !netif_device_present(dev))
         return -ENODEV;
 
-    if (!dev->ethtool_ops)
-        return -EOPNOTSUPP;
-
     if (copy_from_user(&ethcmd, useraddr, sizeof (ethcmd)))
         return -EFAULT;
 
@@ -823,12 +908,25 @@ int dev_ethtool(struct ifreq *ifr)
             return -EPERM;
     }
 
-    if (dev->ethtool_ops->begin)
+    if (dev->ethtool_ops && dev->ethtool_ops->begin)
         if ((rc = dev->ethtool_ops->begin(dev)) < 0)
             return rc;
 
     old_features = dev->features;
 
+    /* Handle some generic operations that do not require specific
+     * ethtool handlers.
+     */
+    switch (ethcmd) {
+    case ETHTOOL_GNDSTATS:
+        return ethtool_get_netdev_stats(dev, useraddr);
+    default:
+        break;
+    }
+
+    if (!dev->ethtool_ops)
+        return -EOPNOTSUPP;
+
     switch (ethcmd) {
     case ETHTOOL_GSET:
         rc = ethtool_get_settings(dev, useraddr);


-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [patch] add net_device_stats support to ethtool

[Dan Nicolaescu]

Ben Greear <greearb@candelatech.com> writes:

  > Dan Nicolaescu wrote:
  > > Ben Greear <greearb@candelatech.com> writes:
  > >
  > >   > Dan Nicolaescu wrote:
  > >   > > Hi,
  > >   > >
  > >   > > I have posted this patch in the past with absolutely no reply.
  > >   > > I would appreciate some sort of feedback of the form interested/not
  > >   > > interested.  Should I just drop it?
  > >   > >
  > >   > >     > I like it, but why not offer this for all devices since
  > > they all have
  > >   > these stats.
  > >   >
  > >   > Could add new handlers called something like .get_strings_generic, or
  > >   > just add this to the higher-level ethtool handling before it looks for
  > >   > handlers.
  > >
  > > If I get your point, then the difference would be that drivers would add
  > > to the initialization of the ethtool structure something like:
  > >   
  > I meant something more like this (this will not apply..I hand-edited
  > it to remove
  > some extraneous crap from my patch...and I'm sure it's white-space damaged).

OK.
Hopefully someone can pick up these patches to be merged...

Re: [patch] add net_device_stats support to ethtool

[Jeff Garzik]

Dan Nicolaescu wrote:
> Hi,
> 
> I have posted this patch in the past with absolutely no reply.
> I would appreciate some sort of feedback of the form 
> interested/not interested.  Should I just drop it?
> 
> "ethtool -S" only supports devices that have custom code written to
> print the stats. 

No -- more specifically, ethtool is for NIC-specific statistics that do 
not appear elsewhere.

net_device_stats already appear elsewhere, so it's redundant to add it 
to ethtool.  Just duplicates the same functionality elsewhere.

	Jeff

Re: [patch] add net_device_stats support to ethtool

[Dan Nicolaescu]

Jeff Garzik <jeff@garzik.org> writes:

  > Dan Nicolaescu wrote:
  > > Hi,
  > >
  > > I have posted this patch in the past with absolutely no reply.
  > > I would appreciate some sort of feedback of the form interested/not
  > > interested.  Should I just drop it?
  > >
  > > "ethtool -S" only supports devices that have custom code written to
  > > print the stats. 
  > 
  > No -- more specifically, ethtool is for NIC-specific statistics that
  > do not appear elsewhere.
  > 
  > net_device_stats already appear elsewhere, so it's redundant to add it
  > to ethtool.  Just duplicates the same functionality elsewhere.

Then please add to -S entry of the ethtool man page something like:
"Only statistics for devices that do not use the generic stat
infrastructure are printed".
It is rather confusing for users that "ethtool -S" prints stats for some
devices, and nothing for others, and the documentations says nothing
about this.