Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320321 > unrolled thread

Re: [PATCH net 0/4] net: add rx_unhandled stat counter

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net 0/4] net: add rx_unhandled stat counter Jarod Wilson <jarod@redhat.com> - 2016-01-28 07:10 +0100
    Re: [PATCH net 0/4] net: add rx_unhandled stat counter Jarod Wilson <jarod@redhat.com> - 2016-01-28 07:20 +0100
      Re: [PATCH net 0/4] net: add rx_unhandled stat counter Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-28 14:10 +0100
        Re: [PATCH net 0/4] net: add rx_unhandled stat counter Jarod Wilson <jarod@redhat.com> - 2016-01-28 15:40 +0100
          Re: [PATCH net 0/4] net: add rx_unhandled stat counter Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-28 15:50 +0100
            Re: [PATCH net 0/4] net: add rx_unhandled stat counter Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-28 15:50 +0100
              Re: [PATCH net 0/4] net: add rx_unhandled stat counter Jarod Wilson <jarod@redhat.com> - 2016-01-28 16:20 +0100
            Re: [PATCH net 0/4] net: add rx_unhandled stat counter Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-28 15:50 +0100
    Re: [PATCH net 0/4] net: add rx_unhandled stat counter Jarod Wilson <jarod@redhat.com> - 2016-01-28 07:20 +0100
    Re: [PATCH net 0/4] net: add rx_unhandled stat counter Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-28 14:10 +0100

#1320321 — Re: [PATCH net 0/4] net: add rx_unhandled stat counter

On Wed, Jan 27, 2016 at 01:09:47PM -0800, Eric Dumazet wrote:
> On Wed, 2016-01-27 at 15:21 -0500, Jarod Wilson wrote:
> 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 289c231..7973ab5 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -180,6 +180,7 @@ struct net_device_stats {
> >  	unsigned long	tx_window_errors;
> >  	unsigned long	rx_compressed;
> >  	unsigned long	tx_compressed;
> > +	unsigned long	rx_unhandled;
> >  };
> >  
> 
> This structure is deprecated, please do not add new fields in it,
> as it will increase netlink answers for no good reason.
> 
> rtnl_link_stats64 is what really matters these days.

I'll respin the set without that, along with s/unhandled/nohandler/, which
I somehow got screwed up in my head and realized a split second after
hitting send. Outside of that, does this approach look sane? Should I
bother with touching /proc/net/dev output or not?

Thanks much,

-- 
Jarod Wilson
jarod@redhat.com

[toc] | [next] | [standalone]

#1320324

On Thu, Jan 28, 2016 at 01:02:15AM -0500, Jarod Wilson wrote:
> On Wed, Jan 27, 2016 at 01:09:47PM -0800, Eric Dumazet wrote:
> > On Wed, 2016-01-27 at 15:21 -0500, Jarod Wilson wrote:
> > 
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index 289c231..7973ab5 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -180,6 +180,7 @@ struct net_device_stats {
> > >  	unsigned long	tx_window_errors;
> > >  	unsigned long	rx_compressed;
> > >  	unsigned long	tx_compressed;
> > > +	unsigned long	rx_unhandled;
> > >  };
> > >  
> > 
> > This structure is deprecated, please do not add new fields in it,
> > as it will increase netlink answers for no good reason.
> > 
> > rtnl_link_stats64 is what really matters these days.
> 
> I'll respin the set without that

Or not. Now I remember why I added that in the first place:

In file included from ./arch/x86/include/asm/uaccess.h:7:0,
                 from net/core/dev.c:75:
net/core/dev.c: In function 'netdev_stats_to_stats64':
include/linux/compiler.h:484:20: error: call to '__compiletime_assert_7263' declared with attribute error: BUILD_BUG_ON failed: sizeof(*stats64) != sizeof(*netdev_stats)
    prefix ## suffix();    \
                    ^

Things are actually hard-wired to require that addition at the moment, or
you get the above build failure. Not sure if it's safe to remove that
BUILD_BUG_ON() yet, haven't looked closely, it's past my bed time. :)

-- 
Jarod Wilson
jarod@redhat.com

[toc] | [prev] | [next] | [standalone]

#1320647

On Thu, 2016-01-28 at 01:18 -0500, Jarod Wilson wrote:

> Or not. Now I remember why I added that in the first place:
> 
> In file included from ./arch/x86/include/asm/uaccess.h:7:0,
>                  from net/core/dev.c:75:
> net/core/dev.c: In function 'netdev_stats_to_stats64':
> include/linux/compiler.h:484:20: error: call to '__compiletime_assert_7263' declared with attribute error: BUILD_BUG_ON failed: sizeof(*stats64) != sizeof(*netdev_stats)
>     prefix ## suffix();    \
>                     ^
> 
> Things are actually hard-wired to require that addition at the moment, or
> you get the above build failure. Not sure if it's safe to remove that
> BUILD_BUG_ON() yet, haven't looked closely, it's past my bed time. :)
> 

This was done for the transition from "unsigned long" to "u64", which is
a nop on 64bit arches.

But as we do not need to be compatible, since no linux kernel in the
past had this new field in struct net_device_stats,

and we do not need to add this new field as it is only accessed from
core networking stack [1], you need to adapt this helper.

[1] And maybe some virtual devices like bonding/team 

[toc] | [prev] | [next] | [standalone]

#1320741

On Thu, Jan 28, 2016 at 05:00:02AM -0800, Eric Dumazet wrote:
> On Thu, 2016-01-28 at 01:18 -0500, Jarod Wilson wrote:
> 
> > Or not. Now I remember why I added that in the first place:
> > 
> > In file included from ./arch/x86/include/asm/uaccess.h:7:0,
> >                  from net/core/dev.c:75:
> > net/core/dev.c: In function 'netdev_stats_to_stats64':
> > include/linux/compiler.h:484:20: error: call to '__compiletime_assert_7263' declared with attribute error: BUILD_BUG_ON failed: sizeof(*stats64) != sizeof(*netdev_stats)
> >     prefix ## suffix();    \
> >                     ^
> > 
> > Things are actually hard-wired to require that addition at the moment, or
> > you get the above build failure. Not sure if it's safe to remove that
> > BUILD_BUG_ON() yet, haven't looked closely, it's past my bed time. :)
> > 
> 
> This was done for the transition from "unsigned long" to "u64", which is
> a nop on 64bit arches.
> 
> But as we do not need to be compatible, since no linux kernel in the
> past had this new field in struct net_device_stats,
> 
> and we do not need to add this new field as it is only accessed from
> core networking stack [1], you need to adapt this helper.

Something like this then:

diff --git a/net/core/dev.c b/net/core/dev.c
index 82334c6..2ca3eab 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7262,15 +7262,16 @@ void netdev_run_todo(void)
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 			     const struct net_device_stats *netdev_stats)
 {
+	memset(stats64, 0, sizeof(*stats64));
 #if BITS_PER_LONG == 64
-	BUILD_BUG_ON(sizeof(*stats64) != sizeof(*netdev_stats));
+	BUILD_BUG_ON(sizeof(*stats64) < sizeof(*netdev_stats));
 	memcpy(stats64, netdev_stats, sizeof(*stats64));
 #else
 	size_t i, n = sizeof(*stats64) / sizeof(u64);
 	const unsigned long *src = (const unsigned long *)netdev_stats;
 	u64 *dst = (u64 *)stats64;
 
-	BUILD_BUG_ON(sizeof(*netdev_stats) / sizeof(unsigned long) !=
+	BUILD_BUG_ON(sizeof(*netdev_stats) / sizeof(unsigned long) >
 		     sizeof(*stats64) / sizeof(u64));
 	for (i = 0; i < n; i++)
 		dst[i] = src[i];

Compiles locally w/o that net_device_stats addition, seems sane to me.

-- 
Jarod Wilson
jarod@redhat.com

[toc] | [prev] | [next] | [standalone]

#1320743

On Thu, 2016-01-28 at 09:38 -0500, Jarod Wilson wrote:

> Something like this then:
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 82334c6..2ca3eab 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7262,15 +7262,16 @@ void netdev_run_todo(void)
>  void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
>  			     const struct net_device_stats *netdev_stats)
>  {
> +	memset(stats64, 0, sizeof(*stats64));
>  #if BITS_PER_LONG == 64
> -	BUILD_BUG_ON(sizeof(*stats64) != sizeof(*netdev_stats));
> +	BUILD_BUG_ON(sizeof(*stats64) < sizeof(*netdev_stats));
>  	memcpy(stats64, netdev_stats, sizeof(*stats64));
>  #else
>  	size_t i, n = sizeof(*stats64) / sizeof(u64);
>  	const unsigned long *src = (const unsigned long *)netdev_stats;
>  	u64 *dst = (u64 *)stats64;
>  
> -	BUILD_BUG_ON(sizeof(*netdev_stats) / sizeof(unsigned long) !=
> +	BUILD_BUG_ON(sizeof(*netdev_stats) / sizeof(unsigned long) >
>  		     sizeof(*stats64) / sizeof(u64));
>  	for (i = 0; i < n; i++)
>  		dst[i] = src[i];
> 
> Compiles locally w/o that net_device_stats addition, seems sane to me.
> 

Sure, you also can set stats64->rx_unhandled to 0 here, just to be 100%
safe.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1320744

On Thu, 2016-01-28 at 06:44 -0800, Eric Dumazet wrote:
> On Thu, 2016-01-28 at 06:42 -0800, Eric Dumazet wrote:
> 
> > 
> > Sure, you also can set stats64->rx_unhandled to 0 here, just to be 100%
> > safe.
> 
> And not add the memset(stats64, 0, sizeof(*stats64)), since we have the
> guarantee to properly init whole stats64 structure.

Or a more tricky

memset((char *)stats64 + sizeof(struct net_device_stats),
       0,
       sizeof(*stats64) - sizeof(struct net_device_stats));

[toc] | [prev] | [next] | [standalone]

#1320780

On Thu, Jan 28, 2016 at 06:46:50AM -0800, Eric Dumazet wrote:
> On Thu, 2016-01-28 at 06:44 -0800, Eric Dumazet wrote:
> > On Thu, 2016-01-28 at 06:42 -0800, Eric Dumazet wrote:
> > 
> > > 
> > > Sure, you also can set stats64->rx_unhandled to 0 here, just to be 100%
> > > safe.
> > 
> > And not add the memset(stats64, 0, sizeof(*stats64)), since we have the
> > guarantee to properly init whole stats64 structure.
> 
> Or a more tricky
> 
> memset((char *)stats64 + sizeof(struct net_device_stats),
>        0,
>        sizeof(*stats64) - sizeof(struct net_device_stats));

I think I like this best, since it won't require someone to notice they
have to add an explicit initialization if they add a new counter, as well
as saving us from memset'ing the entire struct, the majority of which
we'll initialize moments later. Just needs some documentation updates,
which I've done locally. Will rebuild, re-test, then hopefully get an
updated patchset out the door.

-- 
Jarod Wilson
jarod@redhat.com

[toc] | [prev] | [next] | [standalone]

#1320748

On Thu, 2016-01-28 at 06:42 -0800, Eric Dumazet wrote:

> 
> Sure, you also can set stats64->rx_unhandled to 0 here, just to be 100%
> safe.

And not add the memset(stats64, 0, sizeof(*stats64)), since we have the
guarantee to properly init whole stats64 structure.

[toc] | [prev] | [next] | [standalone]

#1320325

On Thu, Jan 28, 2016 at 01:02:15AM -0500, Jarod Wilson wrote:
> On Wed, Jan 27, 2016 at 01:09:47PM -0800, Eric Dumazet wrote:
> > On Wed, 2016-01-27 at 15:21 -0500, Jarod Wilson wrote:
> > 
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index 289c231..7973ab5 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -180,6 +180,7 @@ struct net_device_stats {
> > >  	unsigned long	tx_window_errors;
> > >  	unsigned long	rx_compressed;
> > >  	unsigned long	tx_compressed;
> > > +	unsigned long	rx_unhandled;
> > >  };
> > >  
> > 
> > This structure is deprecated, please do not add new fields in it,
> > as it will increase netlink answers for no good reason.
> > 
> > rtnl_link_stats64 is what really matters these days.
> 
> I'll respin the set without that, along with s/unhandled/nohandler/, which
> I somehow got screwed up in my head and realized a split second after
> hitting send. Outside of that, does this approach look sane? Should I
> bother with touching /proc/net/dev output or not?

Also, please excuse the poor excuse for a cover-letter that had a
duplicate of patch 1 in it. I'll fix that the next pass too.

/me hangs head in shame...

-- 
Jarod Wilson
jarod@redhat.com

[toc] | [prev] | [next] | [standalone]

#1320644

On Thu, 2016-01-28 at 01:02 -0500, Jarod Wilson wrote:
> Outside of that, does this approach look sane? Should I
> bother with touching /proc/net/dev output or not?

Please do not touch /proc/net/dev 

This is legacy stuff and really should not be touched anymore.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web