[PATCH] netdev: re-expose net_device_stats to user headers

[PATCH] netdev: re-expose net_device_stats to user headers

[Stephen Hemminger]

This commit broke iproute2 build:

commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b
Author: Ben Hutchings <bhutchings@solarflare.com>
Date:   Tue Jun 8 07:19:54 2010 +0000

    net: Enable 64-bit net device statistics on 32-bit architectures
    
    Use struct rtnl_link_stats64 as the statistics structure.

Iproute2 uses a the kernel exported headers, and the structure
net_device_stats which is part of the netlink IFLA_STATS message
was no longer exposed.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
This patch needs to be applied to net-2.6 (2.6.36)
    

--- a/include/linux/netdevice.h	2010-08-23 08:22:17.801906612 -0700
+++ b/include/linux/netdevice.h	2010-08-23 08:25:23.502078638 -0700
@@ -165,6 +165,8 @@ static inline bool dev_xmit_complete(int
 #define MAX_HEADER (LL_MAX_HEADER + 48)
 #endif
 
+#endif  /*  __KERNEL__  */
+
 /*
  *	Old network device statistics. Fields are native words
  *	(unsigned long) so they can be read and written atomically.
@@ -196,9 +198,6 @@ struct net_device_stats {
 	unsigned long	tx_compressed;
 };
 
-#endif  /*  __KERNEL__  */
-
-
 /* Media selection options. */
 enum {
         IF_PORT_UNKNOWN = 0,

Re: [PATCH] netdev: re-expose net_device_stats to user headers

[Ben Hutchings]

On Mon, 2010-08-23 at 08:32 -0700, Stephen Hemminger wrote:
> This commit broke iproute2 build:
> 
> commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b
> Author: Ben Hutchings <bhutchings@solarflare.com>
> Date:   Tue Jun 8 07:19:54 2010 +0000
> 
>     net: Enable 64-bit net device statistics on 32-bit architectures
>     
>     Use struct rtnl_link_stats64 as the statistics structure.
> 
> Iproute2 uses a the kernel exported headers, and the structure
> net_device_stats which is part of the netlink IFLA_STATS message
> was no longer exposed.
[...]

This is not true; the kernel uses struct rtnl_link_stats for IFLA_STATS.
AFAICS the only reference to struct net_device_stats in iproute2 is:

misc/ifstat.c:51:#define MAXS (sizeof(struct net_device_stats)/sizeof(unsigned long))

This should be changed to refer to the structure that is actually used.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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] netdev: re-expose net_device_stats to user headers

[Stephen Hemminger]

On Mon, 23 Aug 2010 16:43:23 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Mon, 2010-08-23 at 08:32 -0700, Stephen Hemminger wrote:
> > This commit broke iproute2 build:
> > 
> > commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b
> > Author: Ben Hutchings <bhutchings@solarflare.com>
> > Date:   Tue Jun 8 07:19:54 2010 +0000
> > 
> >     net: Enable 64-bit net device statistics on 32-bit architectures
> >     
> >     Use struct rtnl_link_stats64 as the statistics structure.
> > 
> > Iproute2 uses a the kernel exported headers, and the structure
> > net_device_stats which is part of the netlink IFLA_STATS message
> > was no longer exposed.
> [...]
> 
> This is not true; the kernel uses struct rtnl_link_stats for IFLA_STATS.
> AFAICS the only reference to struct net_device_stats in iproute2 is:
> 
> misc/ifstat.c:51:#define MAXS (sizeof(struct net_device_stats)/sizeof(unsigned long))
> 
> This should be changed to refer to the structure that is actually used.

Ok.

[PATCH] iproute2: add 64bit support to ifstat

[Eric Dumazet]

Le lundi 23 août 2010 à 09:17 -0700, Stephen Hemminger a écrit :
> On Mon, 23 Aug 2010 16:43:23 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Mon, 2010-08-23 at 08:32 -0700, Stephen Hemminger wrote:
> > > This commit broke iproute2 build:
> > > 
> > > commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b
> > > Author: Ben Hutchings <bhutchings@solarflare.com>
> > > Date:   Tue Jun 8 07:19:54 2010 +0000
> > > 
> > >     net: Enable 64-bit net device statistics on 32-bit architectures
> > >     
> > >     Use struct rtnl_link_stats64 as the statistics structure.
> > > 
> > > Iproute2 uses a the kernel exported headers, and the structure
> > > net_device_stats which is part of the netlink IFLA_STATS message
> > > was no longer exposed.
> > [...]
> > 
> > This is not true; the kernel uses struct rtnl_link_stats for IFLA_STATS.
> > AFAICS the only reference to struct net_device_stats in iproute2 is:
> > 
> > misc/ifstat.c:51:#define MAXS (sizeof(struct net_device_stats)/sizeof(unsigned long))
> > 
> > This should be changed to refer to the structure that is actually used.
> 
> Ok.

Stephen,

Here is a patch to ifstat on top of your last one.

I guess nobody tried it on a 64bit platform...

Thanks

iproute2: add 64bit support to ifstat

ifstat assumes IFLA_STATS fields are "unsigned long"

ifstat can use 64bit stats if available (IFLA_STATS64)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 5cf2e01..c649ee2 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -49,7 +49,7 @@ int npatterns;
 char info_source[128];
 int source_mismatch;
 
-#define MAXS (sizeof(struct rtnl_link_stats)/sizeof(unsigned long))
+#define MAXS (sizeof(struct rtnl_link_stats)/sizeof(__u32))
 
 struct ifstat_ent
 {
@@ -58,7 +58,7 @@ struct ifstat_ent
 	int			ifindex;
 	unsigned long long	val[MAXS];
 	double			rate[MAXS];
-	unsigned long		ival[MAXS];
+	__u32			ival[MAXS];
 };
 
 struct ifstat_ent *kern_db;
@@ -108,8 +108,12 @@ static int get_nlmsg(const struct sockaddr_nl *who,
 	n->name = strdup(RTA_DATA(tb[IFLA_IFNAME]));
 	memcpy(&n->ival, RTA_DATA(tb[IFLA_STATS]), sizeof(n->ival));
 	memset(&n->rate, 0, sizeof(n->rate));
-	for (i=0; i<MAXS; i++)
-		n->val[i] = n->ival[i];
+	if (tb[IFLA_STATS64])
+		memcpy(&n->val, RTA_DATA(tb[IFLA_STATS64]), sizeof(n->val));
+	else
+		for (i = 0; i < MAXS; i++)
+			n->val[i] = n->ival[i];
+
 	n->next = kern_db;
 	kern_db = n;
 	return 0;

Re: [PATCH] iproute2: add 64bit support to ifstat

[Stephen Hemminger]

On Mon, 23 Aug 2010 18:54:24 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le lundi 23 août 2010 à 09:17 -0700, Stephen Hemminger a écrit :
> > On Mon, 23 Aug 2010 16:43:23 +0100
> > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > 
> > > On Mon, 2010-08-23 at 08:32 -0700, Stephen Hemminger wrote:
> > > > This commit broke iproute2 build:
> > > > 
> > > > commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b
> > > > Author: Ben Hutchings <bhutchings@solarflare.com>
> > > > Date:   Tue Jun 8 07:19:54 2010 +0000
> > > > 
> > > >     net: Enable 64-bit net device statistics on 32-bit architectures
> > > >     
> > > >     Use struct rtnl_link_stats64 as the statistics structure.
> > > > 
> > > > Iproute2 uses a the kernel exported headers, and the structure
> > > > net_device_stats which is part of the netlink IFLA_STATS message
> > > > was no longer exposed.
> > > [...]
> > > 
> > > This is not true; the kernel uses struct rtnl_link_stats for IFLA_STATS.
> > > AFAICS the only reference to struct net_device_stats in iproute2 is:
> > > 
> > > misc/ifstat.c:51:#define MAXS (sizeof(struct net_device_stats)/sizeof(unsigned long))
> > > 
> > > This should be changed to refer to the structure that is actually used.
> > 
> > Ok.
> 
> Stephen,
> 
> Here is a patch to ifstat on top of your last one.
> 
> I guess nobody tried it on a 64bit platform...
> 
> Thanks
> 
> iproute2: add 64bit support to ifstat
> 
> ifstat assumes IFLA_STATS fields are "unsigned long"
> 
> ifstat can use 64bit stats if available (IFLA_STATS64)
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> 
> diff --git a/misc/ifstat.c b/misc/ifstat.c
> index 5cf2e01..c649ee2 100644
> --- a/misc/ifstat.c
> +++ b/misc/ifstat.c
> @@ -49,7 +49,7 @@ int npatterns;
>  char info_source[128];
>  int source_mismatch;
>  
> -#define MAXS (sizeof(struct rtnl_link_stats)/sizeof(unsigned long))
> +#define MAXS (sizeof(struct rtnl_link_stats)/sizeof(__u32))
>  
>  struct ifstat_ent
>  {
> @@ -58,7 +58,7 @@ struct ifstat_ent
>  	int			ifindex;
>  	unsigned long long	val[MAXS];
>  	double			rate[MAXS];
> -	unsigned long		ival[MAXS];
> +	__u32			ival[MAXS];
>  };
>  
>  struct ifstat_ent *kern_db;
> @@ -108,8 +108,12 @@ static int get_nlmsg(const struct sockaddr_nl *who,
>  	n->name = strdup(RTA_DATA(tb[IFLA_IFNAME]));
>  	memcpy(&n->ival, RTA_DATA(tb[IFLA_STATS]), sizeof(n->ival));
>  	memset(&n->rate, 0, sizeof(n->rate));
> -	for (i=0; i<MAXS; i++)
> -		n->val[i] = n->ival[i];
> +	if (tb[IFLA_STATS64])
> +		memcpy(&n->val, RTA_DATA(tb[IFLA_STATS64]), sizeof(n->val));
> +	else
> +		for (i = 0; i < MAXS; i++)
> +			n->val[i] = n->ival[i];
> +
>  	n->next = kern_db;
>  	kern_db = n;
>  	return 0;
> 
> 

I think this breaks the wraparound detection code in this command.


-- 

Re: [PATCH] iproute2: add 64bit support to ifstat

[Eric Dumazet]

Le lundi 23 août 2010 à 10:33 -0700, Stephen Hemminger a écrit :

> I think this breaks the wraparound detection code in this command.
> 
> 

OK lets fix the bug only, before adding 64bit counters capabilities.

Thanks

[PATCH] iproute2: add 64bit arches support to ifstat

ifstat assumes IFLA_STATS fields are "unsigned long", but they are
__u32. This fix is needed to let ifstat run on 64bit arches.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/misc/ifstat.c b/misc/ifstat.c
index 5cf2e01..5b229e7 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -49,7 +49,7 @@ int npatterns;
 char info_source[128];
 int source_mismatch;
 
-#define MAXS (sizeof(struct rtnl_link_stats)/sizeof(unsigned long))
+#define MAXS (sizeof(struct rtnl_link_stats)/sizeof(__u32))
 
 struct ifstat_ent
 {
@@ -58,7 +58,7 @@ struct ifstat_ent
 	int			ifindex;
 	unsigned long long	val[MAXS];
 	double			rate[MAXS];
-	unsigned long		ival[MAXS];
+	__u32			ival[MAXS];
 };
 
 struct ifstat_ent *kern_db;
@@ -187,7 +187,7 @@ void load_raw_table(FILE *fp)
 			*next++ = 0;
 			if (sscanf(p, "%llu", n->val+i) != 1)
 				abort();
-			n->ival[i] = (unsigned long)n->val[i];
+			n->ival[i] = (__u32)n->val[i];
 			p = next;
 			if (!(next = strchr(p, ' ')))
 				abort();
@@ -563,8 +563,6 @@ static void usage(void)
 "   -s, --noupdate	don;t update history\n"
 "   -t, --interval=SECS	report average over the last SECS\n"
 "   -V, --version	output version information\n"
-"   -z, --zeros		show entries with zero activity\n"
-"   -e, --errors	show errors\n"
 "   -z, --zeros		show entries with zero activity\n");
 
 	exit(-1);
@@ -581,8 +579,6 @@ static const struct option longopts[] = {
 	{ "interval", 1, 0, 't' },
 	{ "version", 0, 0, 'V' },
 	{ "zeros", 0, 0, 'z' },
-	{ "errors", 0, 0, 'e' },
-	{ "zeros", 0, 0, 'z' },
 	{ 0 }
 };
 

Re: [PATCH] iproute2: add 64bit support to ifstat

[Stephen Hemminger]

> @@ -563,8 +563,6 @@ static void usage(void)
>  "   -s, --noupdate	don;t update history\n"
>  "   -t, --interval=SECS	report average over the last SECS\n"
>  "   -V, --version	output version information\n"
> -"   -z, --zeros		show entries with zero activity\n"
> -"   -e, --errors	show errors\n"
>  "   -z, --zeros		show entries with zero activity\n");
>  
>  	exit(-1);
> @@ -581,8 +579,6 @@ static const struct option longopts[] = {
>  	{ "interval", 1, 0, 't' },
>  	{ "version", 0, 0, 'V' },
>  	{ "zeros", 0, 0, 'z' },
> -	{ "errors", 0, 0, 'e' },
> -	{ "zeros", 0, 0, 'z' },
>  	{ 0 }
>  };
>  
> 
> 

Did you mean to break --errors?

Re: [PATCH] iproute2: add 64bit support to ifstat

[Eric Dumazet]

Le lundi 23 août 2010 à 13:06 -0700, Stephen Hemminger a écrit :
> > @@ -563,8 +563,6 @@ static void usage(void)
> >  "   -s, --noupdate	don;t update history\n"
> >  "   -t, --interval=SECS	report average over the last SECS\n"
> >  "   -V, --version	output version information\n"
> > -"   -z, --zeros		show entries with zero activity\n"
> > -"   -e, --errors	show errors\n"
> >  "   -z, --zeros		show entries with zero activity\n");
> >  
> >  	exit(-1);
> > @@ -581,8 +579,6 @@ static const struct option longopts[] = {
> >  	{ "interval", 1, 0, 't' },
> >  	{ "version", 0, 0, 'V' },
> >  	{ "zeros", 0, 0, 'z' },
> > -	{ "errors", 0, 0, 'e' },
> > -	{ "zeros", 0, 0, 'z' },
> >  	{ 0 }
> >  };
> >  
> > 
> > 
> 
> Did you mean to break --errors?

I dont think so.

These options are already handled, unless I am wrong.

Do you want a separate patch for this trivial fix ?

Re: [PATCH] iproute2: add 64bit support to ifstat

[Stephen Hemminger]

On Mon, 23 Aug 2010 22:28:31 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le lundi 23 août 2010 à 13:06 -0700, Stephen Hemminger a écrit :
> > > @@ -563,8 +563,6 @@ static void usage(void)
> > >  "   -s, --noupdate	don;t update history\n"
> > >  "   -t, --interval=SECS	report average over the last SECS\n"
> > >  "   -V, --version	output version information\n"
> > > -"   -z, --zeros		show entries with zero activity\n"
> > > -"   -e, --errors	show errors\n"
> > >  "   -z, --zeros		show entries with zero activity\n");
> > >  
> > >  	exit(-1);
> > > @@ -581,8 +579,6 @@ static const struct option longopts[] = {
> > >  	{ "interval", 1, 0, 't' },
> > >  	{ "version", 0, 0, 'V' },
> > >  	{ "zeros", 0, 0, 'z' },
> > > -	{ "errors", 0, 0, 'e' },
> > > -	{ "zeros", 0, 0, 'z' },
> > >  	{ 0 }
> > >  };
> > >  
> > > 
> > > 
> > 
> > Did you mean to break --errors?
> 
> I dont think so.
> 
> These options are already handled, unless I am wrong.
> 
> Do you want a separate patch for this trivial fix ?

I'll tweak your previous patch
--