bonding inactive slaves vs rx_dropped

bonding inactive slaves vs rx_dropped

[David Miller]

People are starting to notice that rx_dropped now increments on every
packet received on an bond's inactive slave.

I'm actually fine with rx_dropped incrementing in this situation.

The problem I want to address is that rx_dropped is encompassing
several unrelated situations and thus has become less useful for
diagnosis.

I think we should add some new RX stats such that we can get at
least a small amount of granularity for rx_dropped.

This way team, bond, etc. can increment a new netdev_stats->rx_foo in
this situation, and then someone doing diagnosis can see that
rx_dropped and rx_foo are incrementing at similar rates.

Re: bonding inactive slaves vs rx_dropped

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>People are starting to notice that rx_dropped now increments on every
>packet received on an bond's inactive slave.
>
>I'm actually fine with rx_dropped incrementing in this situation.
>
>The problem I want to address is that rx_dropped is encompassing
>several unrelated situations and thus has become less useful for
>diagnosis.
>
>I think we should add some new RX stats such that we can get at
>least a small amount of granularity for rx_dropped.
>
>This way team, bond, etc. can increment a new netdev_stats->rx_foo in
>this situation, and then someone doing diagnosis can see that
>rx_dropped and rx_foo are incrementing at similar rates.

	This drop isn't really happening in bonding, though.  From
looking at the code, it comes about because, for the inactive slave, the
rx_handler call returns EXACT, and there aren't any exact match ptype
bindings, so __netif_receive_skb throws it away.  This isn't always the
case; sometimes there is an exact match, for things like iSCSI or FCoE
that are really determined to get the packet.

	On a non-bonded interface, the same drop path (except for the
rx_handler) is taken for incoming packets for an unsupported protocol.

	We could probably add an, oh, rx_dropped_inactive, or some
variation on that theme, that is incremented at the end of
__netif_receive_skb if deliver_exact is set, e.g., something like:

diff --git a/net/core/dev.c b/net/core/dev.c
index a87bc74..4cd7c1f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3559,6 +3559,8 @@ ncls:
 	} else {
 drop:
 		atomic_long_inc(&skb->dev->rx_dropped);
+		if (deliver_exact)
+			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 think this would only hit frames that have been soft-rejected
(rx_handler says EXACT) by bonding or team, but were not subsequently
delivered to an exact match listener.

	There's the separate questions of whether there should be more
counters (e.g., drops in dev_skb_forward or enqueue_to_backlog), and how
to deliver the counter(s) to user space.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: bonding inactive slaves vs rx_dropped

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Thu, 14 Feb 2013 13:51:01 -0800

> David Miller <davem@davemloft.net> wrote:
> 
>>People are starting to notice that rx_dropped now increments on every
>>packet received on an bond's inactive slave.
>>
>>I'm actually fine with rx_dropped incrementing in this situation.
>>
>>The problem I want to address is that rx_dropped is encompassing
>>several unrelated situations and thus has become less useful for
>>diagnosis.
>>
>>I think we should add some new RX stats such that we can get at
>>least a small amount of granularity for rx_dropped.
>>
>>This way team, bond, etc. can increment a new netdev_stats->rx_foo in
>>this situation, and then someone doing diagnosis can see that
>>rx_dropped and rx_foo are incrementing at similar rates.
> 
> 	This drop isn't really happening in bonding, though.  From
> looking at the code, it comes about because, for the inactive slave, the
> rx_handler call returns EXACT, and there aren't any exact match ptype
> bindings, so __netif_receive_skb throws it away.  This isn't always the
> case; sometimes there is an exact match, for things like iSCSI or FCoE
> that are really determined to get the packet.

This isn't even the whole story, it won't return 'exact' if the packet
from the inactive slave is broadcast or multicast.

My general rule is that every special case increments a special
'absurdity' statistic counter for the code :-)

> 	We could probably add an, oh, rx_dropped_inactive, or some
> variation on that theme, that is incremented at the end of
> __netif_receive_skb if deliver_exact is set, e.g., something like:

Yes, that looks fine to me.

> 	There's the separate questions of whether there should be more
> counters (e.g., drops in dev_skb_forward or enqueue_to_backlog), and how
> to deliver the counter(s) to user space.

Since there is some pain in adding counters, I think we should try to
find a nice (very small) set of cases to cover all at once.

Re: bonding inactive slaves vs rx_dropped

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: Jay Vosburgh <fubar@us.ibm.com>
>Date: Thu, 14 Feb 2013 13:51:01 -0800
>
>> David Miller <davem@davemloft.net> wrote:
>> 
>>>People are starting to notice that rx_dropped now increments on every
>>>packet received on an bond's inactive slave.
>>>
>>>I'm actually fine with rx_dropped incrementing in this situation.
>>>
>>>The problem I want to address is that rx_dropped is encompassing
>>>several unrelated situations and thus has become less useful for
>>>diagnosis.
>>>
>>>I think we should add some new RX stats such that we can get at
>>>least a small amount of granularity for rx_dropped.
>>>
>>>This way team, bond, etc. can increment a new netdev_stats->rx_foo in
>>>this situation, and then someone doing diagnosis can see that
>>>rx_dropped and rx_foo are incrementing at similar rates.
>> 
>> 	This drop isn't really happening in bonding, though.  From
>> looking at the code, it comes about because, for the inactive slave, the
>> rx_handler call returns EXACT, and there aren't any exact match ptype
>> bindings, so __netif_receive_skb throws it away.  This isn't always the
>> case; sometimes there is an exact match, for things like iSCSI or FCoE
>> that are really determined to get the packet.
>
>This isn't even the whole story, it won't return 'exact' if the packet
>from the inactive slave is broadcast or multicast.

	It should; it returns EXACT if the packet is on an inactive
slave, with one exception: for a balance-alb mode inactive slave,
broadcast and multicast return EXACT, but unicast (technically, not
broadcast and not multicast) returns ANOTHER after assigning skb->dev to
the bond.

	This permits unicast traffic to pass on the inactive slave(s) in
alb mode, because its inactive slaves are really "mostly active," and we
only need to suppress broadcasts and multicasts.

	The 802.3ad LACPDU protocol handler will return CONSUME for the
skbs it handles, but that doesn't confuse any drop statistics.

>My general rule is that every special case increments a special
>'absurdity' statistic counter for the code :-)

	Gonna need a whole lot of those for bonding, then.  Maybe it
should have its own ethtool -S stats to handle all of the
bonding-internal absurdity.

>> 	We could probably add an, oh, rx_dropped_inactive, or some
>> variation on that theme, that is incremented at the end of
>> __netif_receive_skb if deliver_exact is set, e.g., something like:
>
>Yes, that looks fine to me.
>
>> 	There's the separate questions of whether there should be more
>> counters (e.g., drops in dev_skb_forward or enqueue_to_backlog), and how
>> to deliver the counter(s) to user space.
>
>Since there is some pain in adding counters, I think we should try to
>find a nice (very small) set of cases to cover all at once.

	There aren't a lot of places that increment rx_dropped outside
of the hardware device driver.  There's this case (__netif_receive_skb),
plus dev_forward_skb, enqueue_to_backlog and one other in
gro_cells_receive.  The dev_forward_skb case has two reasons:
skb_copy_ubufs failure, or !is_skb_forwardable.  Leaving both of the
dev_forward_skb reasons as one new counter, that leaves approximately:

	rx_dropped_inactive, rx_dropped_forward, rx_dropped_backlog and
rx_dropped_gro

	Is four too many?  Am I missing a case somewhere?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: bonding inactive slaves vs rx_dropped

[Ben Hutchings]

On Thu, 2013-02-14 at 15:43 -0800, Jay Vosburgh wrote:
> David Miller <davem@davemloft.net> wrote:
[...]
> >My general rule is that every special case increments a special
> >'absurdity' statistic counter for the code :-)
> 
> 	Gonna need a whole lot of those for bonding, then.  Maybe it
> should have its own ethtool -S stats to handle all of the
> bonding-internal absurdity.
[...]

Yes, I think it should.

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.