[PATCH net-next] net: Use rx_nohandler for unhandled packets

[PATCH net-next] net: Use rx_nohandler for unhandled packets

[Patrick Talbert]

Since caf586e5f23c ("net: add a core netdev->rx_dropped counter") incoming
packets which do not have a handler cause a counter named rx_dropped to be
incremented. This can lead to confusion as some see a non-zero "drop"
counter as cause for concern.

To avoid any confusion, instead use the existing rx_nohandler counter. Its
name more closely aligns with the activity being tracked here.

Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
---
 include/linux/netdevice.h |  5 +++--
 net/core/dev.c            | 15 +++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9ef20389622d..3d194cea9859 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1618,8 +1618,9 @@ enum netdev_priv_flags {
  *			do not use this in drivers
  *	@tx_dropped:	Dropped packets by core network,
  *			do not use this in drivers
- *	@rx_nohandler:	nohandler dropped packets by core network on
- *			inactive devices, do not use this in drivers
+ *	@rx_nohandler:	Dropped packets by core network when they were not handled
+ *			by any protocol/socket or the device was inactive,
+ *			do not use this in drivers.
  *	@carrier_up_count:	Number of times the carrier has been up
  *	@carrier_down_count:	Number of times the carrier has been down
  *
diff --git a/net/core/dev.c b/net/core/dev.c
index 2c277b8aba38..11e500f8ffa3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5123,20 +5123,19 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
 			goto drop;
 		*ppt_prev = pt_prev;
 	} else {
-drop:
-		if (!deliver_exact)
-			atomic_long_inc(&skb->dev->rx_dropped);
-		else
-			atomic_long_inc(&skb->dev->rx_nohandler);
+		/* We have not delivered the skb anywhere */
+		atomic_long_inc(&skb->dev->rx_nohandler);
 		kfree_skb(skb);
-		/* Jamal, now you will not able to escape explaining
-		 * me how you were going to use this. :-)
-		 */
 		ret = NET_RX_DROP;
 	}
 
 out:
 	return ret;
+drop:
+	atomic_long_inc(&skb->dev->rx_dropped);
+	kfree_skb(skb);
+	ret = NET_RX_DROP;
+	goto out;
 }
 
 static int __netif_receive_skb_one_core(struct sk_buff *skb, bool pfmemalloc)
-- 
2.18.1

Re: [PATCH net-next] net: Use rx_nohandler for unhandled packets

[Jakub Kicinski]

On Wed, 11 Dec 2019 17:21:07 +0100, Patrick Talbert wrote:
> Since caf586e5f23c ("net: add a core netdev->rx_dropped counter") incoming
> packets which do not have a handler cause a counter named rx_dropped to be
> incremented. This can lead to confusion as some see a non-zero "drop"
> counter as cause for concern.
> 
> To avoid any confusion, instead use the existing rx_nohandler counter. Its
> name more closely aligns with the activity being tracked here.
> 
> Signed-off-by: Patrick Talbert <ptalbert@redhat.com>

Looks like commit 6e7333d315a7 ("net: add rx_nohandler stat counter")
is far more relevant here, it added rx_nohandler but kept using
rx_dropped for non-exact delivery.

Jarod, could you take a look?

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 9ef20389622d..3d194cea9859 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1618,8 +1618,9 @@ enum netdev_priv_flags {
>   *			do not use this in drivers
>   *	@tx_dropped:	Dropped packets by core network,
>   *			do not use this in drivers
> - *	@rx_nohandler:	nohandler dropped packets by core network on
> - *			inactive devices, do not use this in drivers
> + *	@rx_nohandler:	Dropped packets by core network when they were not handled
> + *			by any protocol/socket or the device was inactive,
> + *			do not use this in drivers.
>   *	@carrier_up_count:	Number of times the carrier has been up
>   *	@carrier_down_count:	Number of times the carrier has been down
>   *
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2c277b8aba38..11e500f8ffa3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5123,20 +5123,19 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
>  			goto drop;
>  		*ppt_prev = pt_prev;
>  	} else {
> -drop:
> -		if (!deliver_exact)
> -			atomic_long_inc(&skb->dev->rx_dropped);
> -		else
> -			atomic_long_inc(&skb->dev->rx_nohandler);
> +		/* We have not delivered the skb anywhere */
> +		atomic_long_inc(&skb->dev->rx_nohandler);
>  		kfree_skb(skb);
> -		/* Jamal, now you will not able to escape explaining
> -		 * me how you were going to use this. :-)
> -		 */
>  		ret = NET_RX_DROP;
>  	}
>  
>  out:
>  	return ret;
> +drop:
> +	atomic_long_inc(&skb->dev->rx_dropped);
> +	kfree_skb(skb);
> +	ret = NET_RX_DROP;
> +	goto out;
>  }
>  
>  static int __netif_receive_skb_one_core(struct sk_buff *skb, bool pfmemalloc)

Re: [PATCH net-next] net: Use rx_nohandler for unhandled packets

[David Miller]

From: Patrick Talbert <ptalbert@redhat.com>
Date: Wed, 11 Dec 2019 17:21:07 +0100

> Since caf586e5f23c ("net: add a core netdev->rx_dropped counter") incoming
> packets which do not have a handler cause a counter named rx_dropped to be
> incremented. This can lead to confusion as some see a non-zero "drop"
> counter as cause for concern.
> 
> To avoid any confusion, instead use the existing rx_nohandler counter. Its
> name more closely aligns with the activity being tracked here.
> 
> Signed-off-by: Patrick Talbert <ptalbert@redhat.com>

I disagree with this change.

When deliver_exact is false we try to deliver the packet to an appropriate
ptype handler.  And we end up in the counter bump if no ptype handler
exists.

Therefore, the two counters allow to distinguish two very different
situations and providing that distinction is quite valuable.

I think this distinction was very much intentional.  Having people
understand that rx_dropped can have this meaning is merely a matter of
education.

I'm not applying this patch, sorry.

Re: [PATCH net-next] net: Use rx_nohandler for unhandled packets

[Patrick Talbert]

On Sun, Dec 15, 2019 at 9:41 PM David Miller <davem@davemloft.net> wrote:
>
> From: Patrick Talbert <ptalbert@redhat.com>
> Date: Wed, 11 Dec 2019 17:21:07 +0100
>
> > Since caf586e5f23c ("net: add a core netdev->rx_dropped counter") incoming
> > packets which do not have a handler cause a counter named rx_dropped to be
> > incremented. This can lead to confusion as some see a non-zero "drop"
> > counter as cause for concern.
> >
> > To avoid any confusion, instead use the existing rx_nohandler counter. Its
> > name more closely aligns with the activity being tracked here.
> >
> > Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
>
> I disagree with this change.

Thank you for the review and consideration.

>
> When deliver_exact is false we try to deliver the packet to an appropriate
> ptype handler.  And we end up in the counter bump if no ptype handler
> exists.

If we get to this point in __netif_receive_skb_core() it means the
packet was not passed on to any handler or socket but it is not
because anything went wrong. It simply means no one cared about it. I
suppose a counter named rx_nothandled or rx_ignored would fit better
but for now, the existing rx_nohandler counter much more closely
describes what has happened. Lumping these discards into rx_dropped
along with buffer errors and that ilk is misleading at best.

>
> Therefore, the two counters allow to distinguish two very different
> situations and providing that distinction is quite valuable.

As is, we have packets which are not handled due to no fault and
packets which were discarded due to some fault lumped into rx_dropped.
I think it is much more useful to clarify that distinction by
utilizing separate counters versus worrying that someone might miss
the distinction between an inactive slave drop and any other drop.

>
> I think this distinction was very much intentional.  Having people
> understand that rx_dropped can have this meaning is merely a matter of
> education.

From what I recall, the patch to add rx_nohandler was a reaction to
caf586e5f23c ("net: add a core netdev->rx_dropped counter") being
backported to RHEL7 and customers suddenly seeing rx_dopped piling up
on inactive bond slaves. I wish it had been more broad to trap all the
unhandled traffic, not just inactive slave traffic, but it wasn't.

I appreciate netdev rx_dropped by itself should not be considered a
count of errors but I argue that is not at all common knowledge. At
Red Hat it is a weekly, if not daily, occurrence that a customer opens
a case to ask about a non-zero rx_dropped counter. Whether they
noticed it themselves or some 3rd party monitoring software
highlighted it. Sometimes it reflects a real problem but more often
than not it turns out there is some benign LLC traffic (or the like)
arriving on the interface. That's a seemingly simple and nice answer
but it often takes a good bit of time to convince people they can
ignore a non-zero rx_dropped (if not ignore, at least appreciate it is
not a fault of the NIC/OS). For example, with pcaps and by monitoring
the counter you can show a 1:1 correlation between specific types of
traffic which do not match /proc/net/ptype and increases in
rx_dropped. Its neat but it takes time and often distracts from
whatever the real problem is.

>
> I'm not applying this patch, sorry.
>

If you do not agree with adjusting the purpose of rx_nohandler to
track these events then would you agree to a new counter?

Re: [PATCH net-next] net: Use rx_nohandler for unhandled packets

[David Miller]

From: Patrick Talbert <ptalbert@redhat.com>
Date: Mon, 16 Dec 2019 15:07:14 +0100

> On Sun, Dec 15, 2019 at 9:41 PM David Miller <davem@davemloft.net> wrote:
>> I'm not applying this patch, sorry.
> 
> If you do not agree with adjusting the purpose of rx_nohandler to
> track these events then would you agree to a new counter?

I guess the people relying on the current rx_dropped behavior don't
matter and we can just change things from underneath them?  No,
actually, we cannot do that.

Educating people to solve this problem has the benefit of not screwing
over the users who understand what the counter means in this situation
already.