net/core: don't increment rx_dropped on inactive slaves

The network core tries to keep track of dropped packets, but some packets you wouldn't really call dropped, so much as intentionally ignored, under certain circumstances. One such case is that of bonding and team device slaves that are currently inactive. Their respective rx_handler functions return RX_HANDLER_EXACT (the only places in the kernel that return that), which ends up tracking into the network core's __netif_receive_skb_core() function's drop path, with no pt_prev set. On a noisy network, this can result in a very rapidly incrementing rx_dropped counter, not only on the inactive slave(s), but also on the master device, such as the following:

In this scenario, p5p1, p5p2 and p7p1 are all inactive slaves in an active-backup bond0, and you can see that all three have high drop counts, with the master bond0 showing a tally of all three.

I know that this was previously discussed some here:

   http://www.spinics.net/lists/netdev/msg226341.html

It seems additional counters never came to fruition, but honestly, for this particular case, I'm not even sure they're warranted, I'd be inclined to say just silently drop these packets without incrementing a counter. At least, that's probably what would make someone who has complained loudly about this issue happy, as they have monitoring tools that are squaking loudly at any increments to rx_dropped.

CC "David S. Miller" <[email protected]>

CC Eric Dumazet <[email protected]>

CC Jiri Pirko <[email protected]>

CC Daniel Borkmann <[email protected]>

CC Tom Herbert <[email protected]>

CC Jay Vosburgh <[email protected]>

CC Veaceslav Falico <[email protected]>

CC Andy Gospodarek <[email protected]>

CC [email protected]

Signed-off-by Jarod Wilson <[email protected]>

---
net/core/dev.c | 3 +++

1 file changed, 3 insertions(+)

net/core/dev.c +2 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else
 			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		if (deliver_exact)
+			goto inactive; /* bond or team inactive slave */

drop:

		atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
		kfree_skb(skb);
		/* Jamal, now you will not able to escape explaining
		 * me how you were going to use this. :-)

Signature · 1 line

1.8.3.1

Quoted 9 lines · Jarod Wilson

The network core tries to keep track of dropped packets, but some packets you wouldn't really call dropped, so much as intentionally ignored, under certain circumstances. One such case is that of bonding and team device slaves that are currently inactive. Their respective rx_handler functions return RX_HANDLER_EXACT (the only places in the kernel that return that), which ends up tracking into the network core's __netif_receive_skb_core() function's drop path, with no pt_prev set. On a noisy network, this can result in a very rapidly incrementing rx_dropped counter, not only on the inactive slave(s), but also on the master device, such as the following:

[...]

Quoted 6 lines

In this scenario, p5p1, p5p2 and p7p1 are all inactive slaves in an active-backup bond0, and you can see that all three have high drop counts, with the master bond0 showing a tally of all three.

I know that this was previously discussed some here:

Quoted 8 lines

   http://www.spinics.net/lists/netdev/msg226341.html

It seems additional counters never came to fruition, but honestly, for this particular case, I'm not even sure they're warranted, I'd be inclined to say just silently drop these packets without incrementing a counter. At least, that's probably what would make someone who has complained loudly about this issue happy, as they have monitoring tools that are squaking loudly at any increments to rx_dropped.

I don't think the kernel should silently drop packets; there
should be a counter somewhere. If a packet is being thrown away deliberately, it should not just vanish into the screaming void of space. Someday someone will try and track down where that packet is being dropped.

I've had that same conversation with customers who insist on
accounting for every packet drop (from the "any drop is an error" mindset), so I understand the issue.

Thinking about the prior discussion, the rx_drop_inactive is
still a good idea, but I'd actually today get good use from a "rx_drop_unforwardable" (or an equivalent but shorter name) counter that counts every time a packet is dropped due to is_skb_forwardable() returning false. __dev_forward_skb does this (and hits rx_dropped), as does the bridge (and does not count it).

	-J

Quoted 25 lines

CC "David S. Miller" <[email protected]>

CC Eric Dumazet <[email protected]>

CC Jiri Pirko <[email protected]>

CC Daniel Borkmann <[email protected]>

CC Tom Herbert <[email protected]>

CC Jay Vosburgh <[email protected]>

CC Veaceslav Falico <[email protected]>

CC Andy Gospodarek <[email protected]>

CC [email protected]

Signed-off-by Jarod Wilson <[email protected]>

---
net/core/dev.c | 3 +++

1 file changed, 3 insertions(+)

net/core/dev.c +2 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else
 			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		if (deliver_exact)
+			goto inactive; /* bond or team inactive slave */

drop:

Quoted 8 lines

		atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
		kfree_skb(skb);
		/* Jamal, now you will not able to escape explaining
		 * me how you were going to use this. :-)

Signature · 1 line

1.8.3.1

---
-Jay Vosburgh, [email protected]

The network core tries to keep track of dropped packets, but some packets you wouldn't really call dropped, so much as intentionally ignored, under certain circumstances. One such case is that of bonding and team device slaves that are currently inactive. Their respective rx_handler functions return RX_HANDLER_EXACT (the only places in the kernel that return that), which ends up tracking into the network core's __netif_receive_skb_core() function's drop path, with no pt_prev set. On a noisy network, this can result in a very rapidly incrementing rx_dropped counter, not only on the inactive slave(s), but also on the master device, such as the following:

Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packets errs drop fifo colls carrier compressed

 p7p1: 14783346  140430    0 140428    0     0          0      2040      680       8    0    0    0     0       0          0
 p7p2: 14805198  140648    0    0    0     0          0      2034        0       0    0    0    0     0       0          0
bond0: 53365248  532798    0 421160    0     0          0    115151     2040      24    0    0    0     0       0          0
   lo:    5420      54    0    0    0     0          0         0     5420      54    0    0    0     0       0          0
 p5p1: 19292195  196197    0 140368    0     0          0     56564      680       8    0    0    0     0       0          0
 p5p2: 19289707  196171    0 140364    0     0          0     56547      680       8    0    0    0     0       0          0
  em3: 20996626  158214    0    0    0     0          0       383        0       0    0    0    0     0       0          0
  em2: 14065122  138462    0    0    0     0          0       310        0       0    0    0    0     0       0          0
  em1: 14063162  138440    0    0    0     0          0       308        0       0    0    0    0     0       0          0
  em4: 21050830  158729    0    0    0     0          0       385    71662     469    0    0    0     0       0          0

ib0: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0

In this scenario, p5p1, p5p2 and p7p1 are all inactive slaves in an active-backup bond0, and you can see that all three have high drop counts, with the master bond0 showing a tally of all three.

I know that this was previously discussed some here:

   http://www.spinics.net/lists/netdev/msg226341.html

It seems additional counters never came to fruition, but honestly, for this particular case, I'm not even sure they're warranted, I'd be inclined to say just silently drop these packets without incrementing a counter. At least, that's probably what would make someone who has complained loudly about this issue happy, as they have monitoring tools that are squaking loudly at any increments to rx_dropped.

CC "David S. Miller" <[email protected]>

CC Eric Dumazet <[email protected]>

CC Jiri Pirko <[email protected]>

CC Daniel Borkmann <[email protected]>

CC Tom Herbert <[email protected]>

CC Jay Vosburgh <[email protected]>

CC Veaceslav Falico <[email protected]>

CC Andy Gospodarek <[email protected]>

CC [email protected]

Signed-off-by Jarod Wilson <[email protected]>

The network core tries to keep track of dropped packets, but some packets you wouldn't really call dropped, so much as intentionally ignored, under certain circumstances. One such case is that of bonding and team device slaves that are currently inactive. Their respective rx_handler functions return RX_HANDLER_EXACT (the only places in the kernel that return that), which ends up tracking into the network core's __netif_receive_skb_core() function's drop path, with no pt_prev set. On a noisy network, this can result in a very rapidly incrementing rx_dropped counter, not only on the inactive slave(s), but also on the master device, such as the following:

Inter-|   Receive                                                |  Transmit
 face |bytes    packets errs drop fifo frame compressed multicast|bytes    packets errs drop fifo colls carrier compressed
  p7p1: 14783346  140430    0 140428    0     0          0      2040      680       8    0    0    0     0       0          0
  p7p2: 14805198  140648    0    0    0     0          0      2034        0       0    0    0    0     0       0          0
 bond0: 53365248  532798    0 421160    0     0          0    115151     2040      24    0    0    0     0       0          0
    lo:    5420      54    0    0    0     0          0         0     5420      54    0    0    0     0       0          0
  p5p1: 19292195  196197    0 140368    0     0          0     56564      680       8    0    0    0     0       0          0
  p5p2: 19289707  196171    0 140364    0     0          0     56547      680       8    0    0    0     0       0          0
   em3: 20996626  158214    0    0    0     0          0       383        0       0    0    0    0     0       0          0
   em2: 14065122  138462    0    0    0     0          0       310        0       0    0    0    0     0       0          0
   em1: 14063162  138440    0    0    0     0          0       308        0       0    0    0    0     0       0          0
   em4: 21050830  158729    0    0    0     0          0       385    71662     469    0    0    0     0       0          0

ib0: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0

In this scenario, p5p1, p5p2 and p7p1 are all inactive slaves in an active-backup bond0, and you can see that all three have high drop counts, with the master bond0 showing a tally of all three.

I know that this was previously discussed some here:

    http://www.spinics.net/lists/netdev/msg226341.html

It seems additional counters never came to fruition, but honestly, for this particular case, I'm not even sure they're warranted, I'd be inclined to say just silently drop these packets without incrementing a counter. At least, that's probably what would make someone who has complained loudly about this issue happy, as they have monitoring tools that are squaking loudly at any increments to rx_dropped.

CC "David S. Miller" <[email protected]>

CC Eric Dumazet <[email protected]>

CC Jiri Pirko <[email protected]>

CC Daniel Borkmann <[email protected]>

CC Tom Herbert <[email protected]>

CC Jay Vosburgh <[email protected]>

CC Veaceslav Falico <[email protected]>

CC Andy Gospodarek <[email protected]>

CC [email protected]

Signed-off-by Jarod Wilson <[email protected]>

---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

net/core/dev.c +3 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else
 			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		if (deliver_exact)
+			goto inactive; /* bond or team inactive slave */
 drop:
 		atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
 		kfree_skb(skb);
 		/* Jamal, now you will not able to escape explaining
 		 * me how you were going to use this. :-)

Signature · 1 line

1.8.3.1

---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

net/core/dev.c +0 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else

Quoted 10 lines · Jarod Wilson

---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

net/core/dev.c +0 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else

net/core/dev.c ncls: · line 4163

Quoted 10 lines

net/core/dev.c +3 -0

 			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		if (deliver_exact)
+			goto inactive; /* bond or team inactive slave */
 drop:
 		atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
 		kfree_skb(skb);
 		/* Jamal, now you will not able to escape explaining
 		 * me how you were going to use this. :-)

Note that if you still have a kfree_skb() instead of consume_skb(), some tools will still give you a wrong signal (packet dropped ...).

But then maybe the signal is telling some truth.

We receive a packet, and decide to drop it because no one was willing to handle it.

Maybe someone wants to know a particular slave receives 10,000 such frames per second and hurts performance with useless work.

We should at least increment some counter and maybe dump it with "ethtool -S" or something.

# ethtool -S bond0
no stats available

From: Jarod Wilson <[email protected]>
Date: Tue, 26 Jan 2016 16:14:53 -0500

Quoted 2 lines

# ethtool -S bond0
no stats available

ethtool -S is for device specific stats.

Some drivers use this facility to provide per-RX-queue and per-TX-queue versions of the existing core netdev stats.

Quoted 28 lines · Eric Dumazet

Quoted 10 lines · Jarod Wilson

---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

net/core/dev.c +0 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
            else

net/core/dev.c ncls: · line 4163

Quoted 10 lines

net/core/dev.c +3 -0

                    ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
    } else {
+           if (deliver_exact)
+                   goto inactive; /* bond or team inactive slave */
 drop:
            atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
            kfree_skb(skb);
            /* Jamal, now you will not able to escape explaining
             * me how you were going to use this. :-)

Note that if you still have a kfree_skb() instead of consume_skb(), some tools will still give you a wrong signal (packet dropped ...).

But then maybe the signal is telling some truth.

Quoted 8 lines

We receive a packet, and decide to drop it because no one was willing to handle it.

Maybe someone wants to know a particular slave receives 10,000 such frames per second and hurts performance with useless work.

We should at least increment some counter and maybe dump it with "ethtool -S" or something.

I've been digging into ethtool -S a little bit, and am somewhat at a loss as to how I would wire into this. From what I've been able to figure out, it's entirely device-specific-ish counters spit out. On my sfc cards, I get rx_noskb_drops and rx_nodesc_drop_cnt output from ethtool -S, but for the core network stack, these are actually added up and shoved into rx_dropped, and no other network driver has those two individual counters.

By itself, rx_dropped isn't output directly anywhere from ethtool, so far as I can see. And ethtool -S bondX shows absolutely nothing. *Should* ethtool -S be dumping all the network core stats? I have to say I was more than a little surprised at this:

Quoted 45 lines · Jarod Wilson

Quoted 28 lines · Eric Dumazet

Quoted 10 lines · Jarod Wilson

---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

net/core/dev.c +0 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
            else

net/core/dev.c ncls: · line 4163

Quoted 10 lines

net/core/dev.c +3 -0

                    ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
    } else {
+           if (deliver_exact)
+                   goto inactive; /* bond or team inactive slave */
 drop:
            atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
            kfree_skb(skb);
            /* Jamal, now you will not able to escape explaining
             * me how you were going to use this. :-)

Note that if you still have a kfree_skb() instead of consume_skb(), some tools will still give you a wrong signal (packet dropped ...).

But then maybe the signal is telling some truth.

Quoted 8 lines

We receive a packet, and decide to drop it because no one was willing to handle it.

Maybe someone wants to know a particular slave receives 10,000 such frames per second and hurts performance with useless work.

We should at least increment some counter and maybe dump it with "ethtool -S" or something.

I've been digging into ethtool -S a little bit, and am somewhat at a loss as to how I would wire into this. From what I've been able to figure out, it's entirely device-specific-ish counters spit out. On my sfc cards, I get rx_noskb_drops and rx_nodesc_drop_cnt output from ethtool -S, but for the core network stack, these are actually added up and shoved into rx_dropped, and no other network driver has those two individual counters.

Quoted 4 lines

By itself, rx_dropped isn't output directly anywhere from ethtool, so far as I can see. And ethtool -S bondX shows absolutely nothing. *Should* ethtool -S be dumping all the network core stats? I have to say I was more than a little surprised at this:

# ip -s -s link sh dev eth0
15: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc
pfifo_fast state DOWN mode DEFAULT group default qlen 1000
    link/ether 3c:97:0e:be:91:7b brd ff:ff:ff:ff:ff:ff
    RX: bytes  packets  errors  dropped overrun mcast
    0          0        0       0       0       0
    RX errors: length  crc     frame   fifo    missed
               0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    0          0        0       0       0       0
    TX errors: aborted fifo    window  heartbeat
               0        0       0       0

So start with the following patch :

include/uapi/linux/if_link.h +0 -0

index a30b78090594..7c762cf1e4d5 100644

net/core/dev.c +3 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else
 			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		if (deliver_exact)
+			goto inactive; /* bond or team inactive slave */
 drop:
 		atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
 		kfree_skb(skb);
 		/* Jamal, now you will not able to escape explaining
 		 * me how you were going to use this. :-)

Signature · 1 line

1.8.3.1

From: Jarod Wilson <[email protected]>
Date: Fri, 22 Jan 2016 14:11:22 -0500

Quoted 19 lines

net/core/dev.c +3 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else
 			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		if (deliver_exact)
+			goto inactive; /* bond or team inactive slave */
 drop:
 		atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
 		kfree_skb(skb);
 		/* Jamal, now you will not able to escape explaining
 		 * me how you were going to use this. :-)

Signature · 1 line

1.8.3.1

I agree that rx_dropped is not the correct stat to bump here, but I'm totally against the event disappearing completely into thin air.

You have to replace the rx_dropped bump with _something_.

The only reason this hasn't been "fixed" yet is that everyone is too damn lazy to implement that "something".

Quoted 24 lines · David Miller

From: Jarod Wilson <[email protected]>
Date: Fri, 22 Jan 2016 14:11:22 -0500

Quoted 6 lines

net/core/dev.c +0 -0

index 8cba3d8..1354c7b 100644
@@ -4153,8 +4153,11 @@ ncls:
 		else

net/core/dev.c ncls: · line 4163

Quoted 10 lines

net/core/dev.c +3 -0

 			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		if (deliver_exact)
+			goto inactive; /* bond or team inactive slave */
 drop:
 		atomic_long_inc(&skb->dev->rx_dropped);
+inactive:
 		kfree_skb(skb);
 		/* Jamal, now you will not able to escape explaining
 		 * me how you were going to use this. :-)

I agree that rx_dropped is not the correct stat to bump here, but I'm totally against the event disappearing completely into thin air.

Quoted 4 lines

You have to replace the rx_dropped bump with _something_.

The only reason this hasn't been "fixed" yet is that everyone is too damn lazy to implement that "something".

Would you want to see all things that shouldn't increment rx_dropped come in one shot, along with the four or so other counters, as discussed in the prior thread, or can they be done piecemeal? To date, I'm really only familiar with this particular case, and could probably get something together this week. To address the rest, I'd have to poke around a bit more and see what there is to see and do.

include/linux/netdevice.h struct net_device_stats { · line 185

Quoted 11 lines · Jarod Wilson

include/linux/netdevice.h +1 -0

index 289c231..7973ab5 100644
@@ -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.

Quoted 17 lines · Eric Dumazet

include/linux/netdevice.h struct net_device_stats { · line 185

Quoted 11 lines · Jarod Wilson

include/linux/netdevice.h +1 -0

index 289c231..7973ab5 100644
@@ -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.

Quoted 1 line

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?

Quoted 17 lines · Eric Dumazet

include/linux/netdevice.h struct net_device_stats { · line 185

Quoted 11 lines · Jarod Wilson

include/linux/netdevice.h +1 -0

index 289c231..7973ab5 100644
@@ -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.

Quoted 1 line

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. :)

Quoted 9 lines · Jarod Wilson

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();    \
                    ^

Quoted 4 lines

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:

net/core/dev.c +3 -2

index 82334c6..2ca3eab 100644
@@ -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.

Quoted 3 lines · Eric Dumazet

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.

Quoted 6 lines · Eric Dumazet

Quoted 3 lines · Eric Dumazet

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

Quoted 2 lines

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

Outside of that, does this approach look sane? Should I bother with touching /proc/net/dev output or not?

The netdev_stats_to_stats64 function copies the deprecated net_device_stats format stats into rtnl_link_stats64 for legacy support purposes, but with the BUILD_BUG_ON as it was, it wasn't possible to extend rtnl_link_stats64 without also extending net_device_stats. Relax the BUILD_BUG_ON to only require that rtnl_link_stats64 is larger, and zero out all the stat counters that aren't present in net_device_stats.

CC Eric Dumazet <[email protected]>

CC [email protected]

Signed-off-by Jarod Wilson <[email protected]>

---
Re-re-sending, hopefully getting the patch to the right list this time.

 net/core/dev.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

net/core/dev.c +5 -3

index 8cba3d8..575a7df 100644
@@ -7253,25 +7253,30 @@ void netdev_run_todo(void)
 	}
 }
 
-/* Convert net_device_stats to rtnl_link_stats64.  They have the same
- * fields in the same order, with only the type differing.
+/* Convert net_device_stats to rtnl_link_stats64. rtnl_link_stats64 has
+ * all the same fields in the same order as net_device_stats, with only
+ * the type differing, but rtnl_link_stats64 may have additional fields
+ * at the end for newer counters.
  */
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 			     const struct net_device_stats *netdev_stats)
 {
 #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;

Quoted 43 lines · Jarod Wilson

The netdev_stats_to_stats64 function copies the deprecated net_device_stats format stats into rtnl_link_stats64 for legacy support purposes, but with the BUILD_BUG_ON as it was, it wasn't possible to extend rtnl_link_stats64 without also extending net_device_stats. Relax the BUILD_BUG_ON to only require that rtnl_link_stats64 is larger, and zero out all the stat counters that aren't present in net_device_stats.

CC Eric Dumazet <[email protected]>

CC [email protected]

Signed-off-by Jarod Wilson <[email protected]>

---
Re-re-sending, hopefully getting the patch to the right list this time.

 net/core/dev.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

net/core/dev.c +5 -3

index 8cba3d8..575a7df 100644
@@ -7253,25 +7253,30 @@ void netdev_run_todo(void)
 	}
 }
 
-/* Convert net_device_stats to rtnl_link_stats64.  They have the same
- * fields in the same order, with only the type differing.
+/* Convert net_device_stats to rtnl_link_stats64. rtnl_link_stats64 has
+ * all the same fields in the same order as net_device_stats, with only
+ * the type differing, but rtnl_link_stats64 may have additional fields
+ * at the end for newer counters.
  */
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 			     const struct net_device_stats *netdev_stats)
 {
 #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;
 

net/core/dev.c void netdev_run_todo(void) · line 7279

Quoted 9 lines

net/core/dev.c +4 -1

-	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];
 #endif
+	/* zero out counters that only exist in rtnl_link_stats64 */
+	memset((char *)stats64 + sizeof(*netdev_stats), 0,
+	       sizeof(*stats64) - sizeof(*netdev_stats));

Are you sure it works on 32bit arches ?

Quoted 44 lines · Eric Dumazet

Quoted 43 lines · Jarod Wilson

The netdev_stats_to_stats64 function copies the deprecated net_device_stats format stats into rtnl_link_stats64 for legacy support purposes, but with the BUILD_BUG_ON as it was, it wasn't possible to extend rtnl_link_stats64 without also extending net_device_stats. Relax the BUILD_BUG_ON to only require that rtnl_link_stats64 is larger, and zero out all the stat counters that aren't present in net_device_stats.

CC Eric Dumazet <[email protected]>

CC [email protected]

Signed-off-by Jarod Wilson <[email protected]>

---
Re-re-sending, hopefully getting the patch to the right list this time.

 net/core/dev.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

net/core/dev.c +5 -3

index 8cba3d8..575a7df 100644
@@ -7253,25 +7253,30 @@ void netdev_run_todo(void)
 	}
 }
 
-/* Convert net_device_stats to rtnl_link_stats64.  They have the same
- * fields in the same order, with only the type differing.
+/* Convert net_device_stats to rtnl_link_stats64. rtnl_link_stats64 has
+ * all the same fields in the same order as net_device_stats, with only
+ * the type differing, but rtnl_link_stats64 may have additional fields
+ * at the end for newer counters.
  */
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 			     const struct net_device_stats *netdev_stats)
 {
 #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;
 

Quoted 11 lines

net/core/dev.c void netdev_run_todo(void) · line 7279

Quoted 9 lines

net/core/dev.c +4 -1

-	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];
 #endif
+	/* zero out counters that only exist in rtnl_link_stats64 */
+	memset((char *)stats64 + sizeof(*netdev_stats), 0,
+	       sizeof(*stats64) - sizeof(*netdev_stats));

Are you sure it works on 32bit arches ?

Ew, no, it won't work correctly on 32-bit. The for loop is going to copy data into dst from beyond the end of netdev_stats, and the range looks like it won't be right either, only half of the added stats64 space will get zeroed out. Okay, I'll fix that up correctly.

Quoted 4 lines

Ew, no, it won't work correctly on 32-bit. The for loop is going to copy data into dst from beyond the end of netdev_stats, and the range looks like it won't be right either, only half of the added stats64 space will get zeroed out. Okay, I'll fix that up correctly.

Completely untested as of yet, but I think something like the following looks correct. I'll give it a spin as soon as I can.

From: Jarod Wilson <[email protected]>
Date: Sat, 30 Jan 2016 15:53:05 -0500

Quoted 6 lines · Jarod Wilson

Quoted 4 lines

Ew, no, it won't work correctly on 32-bit. The for loop is going to copy data into dst from beyond the end of netdev_stats, and the range looks like it won't be right either, only half of the added stats64 space will get zeroed out. Okay, I'll fix that up correctly.

Quoted 2 lines

Completely untested as of yet, but I think something like the following looks correct. I'll give it a spin as soon as I can.

Jarod, please respin your entire series as a v3 once you sort this out.

The network core tries to keep track of dropped packets, but some packets you wouldn't really call dropped, so much as intentionally ignored, under certain circumstances. One such case is that of bonding and team device slaves that are currently inactive. Their respective rx_handler functions return RX_HANDLER_EXACT (the only places in the kernel that return that), which ends up tracking into the network core's __netif_receive_skb_core() function's drop path, with no pt_prev set. On a noisy network, this can result in a very rapidly incrementing rx_dropped counter, not only on the inactive slave(s), but also on the master device, such as the following:

From: Jarod Wilson <[email protected]>
Date: Thu, 28 Jan 2016 10:49:44 -0500

Quoted 9 lines

The network core tries to keep track of dropped packets, but some packets you wouldn't really call dropped, so much as intentionally ignored, under certain circumstances. One such case is that of bonding and team device slaves that are currently inactive. Their respective rx_handler functions return RX_HANDLER_EXACT (the only places in the kernel that return that), which ends up tracking into the network core's __netif_receive_skb_core() function's drop path, with no pt_prev set. On a noisy network, this can result in a very rapidly incrementing rx_dropped counter, not only on the inactive slave(s), but also on the master device, such as the following:

...

Both my inbox and patchwork only show patch 2, 3, and 4. Where is #1?

include/uapi/linux/if_link.h +2 -0

@@ -35,6 +35,8 @@ struct rtnl_link_stats {
 	/* for cslip etc */
 	__u32	rx_compressed;
 	__u32	tx_compressed;
+
+	__u32	rx_nohandler;		/* dropped, no handler found	*/
 };
 
 /* The main device statistics structure */

Quoted 12 lines · On Mon, 1 Feb 2016 18:51:05 -0500 Jarod Wilson <[email protected]> wrote:

include/uapi/linux/if_link.h +2 -0

@@ -35,6 +35,8 @@ struct rtnl_link_stats {
 	/* for cslip etc */
 	__u32	rx_compressed;
 	__u32	tx_compressed;
+
+	__u32	rx_nohandler;		/* dropped, no handler found	*/
 };
 
 /* The main device statistics structure */

include/uapi/linux/if_link.h struct rtnl_link_stats64 { · line 75

Quoted 6 lines

include/uapi/linux/if_link.h +2 -0

@@ -68,6 +70,8 @@ struct rtnl_link_stats64 {
 	/* for cslip etc */
 	__u64	rx_compressed;
 	__u64	tx_compressed;
+
+	__u64	rx_nohandler;		/* dropped, no handler found	*/
 };

Why was this userspace ABI change allowed?
The stats structure is exposed to user space via netlink and changing the size of responses will break iproute2 commands.

The code will be expecting one size and the response will vary and break existing code. Yes, the code should check the size of the response, but it doesn't and I am sure iproute2 is not the only code that does this.

Quoted 3 lines

Why was this userspace ABI change allowed?
The stats structure is exposed to user space via netlink and changing the size of responses will break iproute2 commands.

Quoted 5 lines

The code will be expecting one size and the response will vary and break existing code. Yes, the code should check the size of the response, but it doesn't and I am sure iproute2 is not the only code that does this.

Jarod, please look into this.

Quoted 3 lines · David Miller

Quoted 3 lines

Why was this userspace ABI change allowed?
The stats structure is exposed to user space via netlink and changing the size of responses will break iproute2 commands.

I do not think it breaks anything.

iproute2 always assumed kernel was sending at least 23 u64, and does not check at all if the kernel sends more. (or less, so iproute2 can print garbage if kernel is malicious)

an iproute2 patch will be needed to automatically detect if new kernels are sending more data and print it accordingly.

Quoted 6 lines

Quoted 4 lines

The code will be expecting one size and the response will vary and break existing code. Yes, the code should check the size of the response, but it doesn't and I am sure iproute2 is not the only code that does this.

Jarod, please look into this.

Running latest net-next, and old iproute2 is just fine.

Quoted 6 lines · Eric Dumazet

Quoted 3 lines · David Miller

Quoted 3 lines

Why was this userspace ABI change allowed?
The stats structure is exposed to user space via netlink and changing the size of responses will break iproute2 commands.

Quoted 8 lines

I do not think it breaks anything.

iproute2 always assumed kernel was sending at least 23 u64, and does not check at all if the kernel sends more. (or less, so iproute2 can print garbage if kernel is malicious)

an iproute2 patch will be needed to automatically detect if new kernels are sending more data and print it accordingly.

My TODO list did include poking at iproute2 to expose the new info, I can take a closer look for possible issues as well, but...

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

The code will be expecting one size and the response will vary and break existing code. Yes, the code should check the size of the response, but it doesn't and I am sure iproute2 is not the only code that does this.

Jarod, please look into this.

Running latest net-next, and old iproute2 is just fine.

...I haven't run into anything that didn't work with current iproute2 either while testing out functionality of these patches. If there's something in particular that seems most suspect that I perhaps simply haven't tried, I can give that a go as well.

In any case, I'm definitely due to take a look at iproute2 as it relates to this patchset.

The iproute2 command can be fixed, but adding dependency on size of response gets gross fast. Imagine when 4 more fields get added, this doesn't scale well.

Also, the definition of userspace ABI is that structures can't change. There are many other utilities that are not visible that may get broken. Traditionally Linux has guaranteed that programs will continue to work no matter how they were coded.

Whole point of TLV is that it allows us to add new fields at the end of the structures.

Look at iproute2, you were the one adding in 2004 code to cope with various tcp_info sizes.

So 12 years later, you cannot say it does not work anymore.

From: Eric Dumazet <[email protected]>
Date: Mon, 08 Feb 2016 14:57:40 -0800

Quoted 2 lines

Whole point of TLV is that it allows us to add new fields at the end of the structures.

...

Quoted 4 lines

Look at iproute2, you were the one adding in 2004 code to cope with various tcp_info sizes.

So 12 years later, you cannot say it does not work anymore.

+1

Support for the new rx_nohandler statistic.
This code is designed to handle the case where the kernel reported statistic structure is smaller than the larger structure in later releases (and vice versa).

Quoted 3 lines · Stephen Hemminger

Support for the new rx_nohandler statistic.
This code is designed to handle the case where the kernel reported statistic structure is smaller than the larger structure in later releases (and vice versa).

This seems to work here, for the most part. However, if you are running a kernel with the new counter, and the counter happens to contain 0, aren't we going to not print anything?

I've got a tweaked version here locally that gets a touch messy, where I get a count of members from RTA_DATA(IFLA_STATS{,64} / sizeof(__u{32,64}), pass that into the print functions, and key off that length for whether or not to print the extra members, so they'll show up even when 0, if they're supported. This does rely on strict ordering of the struct members, no reordering, no removals, etc., but I think everyone is already in favor of that. Looks like the same sort of length checks could be used for rx_compressed and tx_compressed as well, as I think they fall victim to the same issue of not printing if those counters are legitimately 0. Yes, it's a little uglier, and more brittle, but more accurate output.

Quoted 5 lines · On Tue, 9 Feb 2016 18:51:35 -0500 Jarod Wilson <[email protected]> wrote:

Quoted 3 lines · Stephen Hemminger

Support for the new rx_nohandler statistic.
This code is designed to handle the case where the kernel reported statistic structure is smaller than the larger structure in later releases (and vice versa).

Quoted 3 lines

This seems to work here, for the most part. However, if you are running a kernel with the new counter, and the counter happens to contain 0, aren't we going to not print anything?

That is the desirable outcome, since if run on older system the output format will not change from current format.

Quoted 8 lines · Stephen Hemminger

Quoted 5 lines · On Tue, 9 Feb 2016 18:51:35 -0500 Jarod Wilson <[email protected]> wrote:

Quoted 3 lines · Stephen Hemminger

Support for the new rx_nohandler statistic.
This code is designed to handle the case where the kernel reported statistic structure is smaller than the larger structure in later releases (and vice versa).

Quoted 6 lines

Quoted 3 lines

This seems to work here, for the most part. However, if you are running a kernel with the new counter, and the counter happens to contain 0, aren't we going to not print anything?

That is the desirable outcome, since if run on older system the output format will not change from current format.

The problem here is that a change in output might break some user scripts using sed/whatever games.

So it might be better to output a zero field, so that such breakages are detected early, even if no packet was dropped at the time the new kernel was tested.

Having a binary that adds the new field only in some cases hides the change. It looks fine for us humans, but not for programs processing the output.

Quoted 19 lines · Eric Dumazet

Quoted 8 lines · Stephen Hemminger

Quoted 5 lines · On Tue, 9 Feb 2016 18:51:35 -0500 Jarod Wilson <[email protected]> wrote:

Quoted 3 lines · Stephen Hemminger

Support for the new rx_nohandler statistic.
This code is designed to handle the case where the kernel reported statistic structure is smaller than the larger structure in later releases (and vice versa).

Quoted 6 lines

Quoted 3 lines

This seems to work here, for the most part. However, if you are running a kernel with the new counter, and the counter happens to contain 0, aren't we going to not print anything?

That is the desirable outcome, since if run on older system the output format will not change from current format.

The problem here is that a change in output might break some user scripts using sed/whatever games.

Quoted 7 lines

So it might be better to output a zero field, so that such breakages are detected early, even if no packet was dropped at the time the new kernel was tested.

Having a binary that adds the new field only in some cases hides the change. It looks fine for us humans, but not for programs processing the output.

On my test setup, my bond's active interface currently has 0, while the backup interface has a few thousand, so I can alternate back and forth checking the interfaces, and one doesn't print the counter while the other does, which is what seemed odd to me and prompted the added ugliness. But most setups (anything outside of bond/team currently) should never have this counter incremented, we do have prior art with the compressed fields, and scripts really probably ought to be scraping stats out of sysfs rather than using ip, so I can sort of understand not wanting the added ugliness. I do tend to prefer consistency though.

It seems additional counters never came to fruition, so this is a first attempt at creating one of them, so that we stop calling these drops, which for users monitoring rx_dropped, causes great alarm, and renders the counter much less useful for them.

This adds a sysfs statistics node and makes the counter available via netlink.

Additionally, I'm not certain if this set qualifies for net, or if it should be put aside and resubmitted for net-next after 4.5 is put to bed, but I do have users who consider this an important bugfix.

This has been tested quite a bit on x86_64, and now lightly on i686 as well, to verify functionality of updates to netdev_stats_to_stats64() on 32-bit arches.