netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: udp: increase UDP_MIB_RCVBUFERRORS when ENOBUFS
@ 2020-10-26  9:39 Menglong Dong
  2020-10-26  9:51 ` Paolo Abeni
  0 siblings, 1 reply; 3+ messages in thread
From: Menglong Dong @ 2020-10-26  9:39 UTC (permalink / raw)
  To: davem; +Cc: kuznet, yoshfuji, kuba, netdev, linux-kernel

The error returned from __udp_enqueue_schedule_skb is ENOMEM or ENOBUFS.
For now, only ENOMEM is counted into UDP_MIB_RCVBUFERRORS in
__udp_queue_rcv_skb. UDP_MIB_RCVBUFERRORS should count all of the
failed skb because of memory errors during udp receiving, not just
those because of the limit of sock receive queue. We can see this
in __udp4_lib_mcast_deliver:

		nskb = skb_clone(skb, GFP_ATOMIC);

		if (unlikely(!nskb)) {
			atomic_inc(&sk->sk_drops);
			__UDP_INC_STATS(net, UDP_MIB_RCVBUFERRORS,
					IS_UDPLITE(sk));
			__UDP_INC_STATS(net, UDP_MIB_INERRORS,
					IS_UDPLITE(sk));
			continue;
		}

See, UDP_MIB_RCVBUFERRORS is increased when skb clone failed. From this
point, ENOBUFS from __udp_enqueue_schedule_skb should be counted, too.
It means that the buffer used by all of the UDP sock is to the limit, and
it ought to be counted.

Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>
---
 net/ipv4/udp.c | 4 +---
 net/ipv6/udp.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 09f0a23d1a01..49a69d8d55b3 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2035,9 +2035,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		int is_udplite = IS_UDPLITE(sk);
 
 		/* Note that an ENOMEM error is charged twice */
-		if (rc == -ENOMEM)
-			UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS,
-					is_udplite);
+		UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS, is_udplite);
 		UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
 		kfree_skb(skb);
 		trace_udp_fail_queue_rcv_skb(rc, sk);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 29d9691359b9..d5e23b150fd9 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -634,9 +634,7 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		int is_udplite = IS_UDPLITE(sk);
 
 		/* Note that an ENOMEM error is charged twice */
-		if (rc == -ENOMEM)
-			UDP6_INC_STATS(sock_net(sk),
-					 UDP_MIB_RCVBUFERRORS, is_udplite);
+		UDP6_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS, is_udplite);
 		UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
 		kfree_skb(skb);
 		return -1;
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] net: udp: increase UDP_MIB_RCVBUFERRORS when ENOBUFS
  2020-10-26  9:39 [PATCH] net: udp: increase UDP_MIB_RCVBUFERRORS when ENOBUFS Menglong Dong
@ 2020-10-26  9:51 ` Paolo Abeni
  2020-10-26 12:47   ` Menglong Dong
  0 siblings, 1 reply; 3+ messages in thread
From: Paolo Abeni @ 2020-10-26  9:51 UTC (permalink / raw)
  To: Menglong Dong, davem; +Cc: kuznet, yoshfuji, kuba, netdev, linux-kernel

Hello,

On Mon, 2020-10-26 at 17:39 +0800, Menglong Dong wrote:
> The error returned from __udp_enqueue_schedule_skb is ENOMEM or ENOBUFS.
> For now, only ENOMEM is counted into UDP_MIB_RCVBUFERRORS in
> __udp_queue_rcv_skb. UDP_MIB_RCVBUFERRORS should count all of the
> failed skb because of memory errors during udp receiving, not just because of the limit of sock receive queue. We can see this
> in __udp4_lib_mcast_deliver:
> 
> 		nskb = skb_clone(skb, GFP_ATOMIC);
> 
> 		if (unlikely(!nskb)) {
> 			atomic_inc(&sk->sk_drops);
> 			__UDP_INC_STATS(net, UDP_MIB_RCVBUFERRORS,
> 					IS_UDPLITE(sk));
> 			__UDP_INC_STATS(net, UDP_MIB_INERRORS,
> 					IS_UDPLITE(sk));
> 			continue;
> 		}
> 
> See, UDP_MIB_RCVBUFERRORS is increased when skb clone failed. From this
> point, ENOBUFS from __udp_enqueue_schedule_skb should be counted, too.
> It means that the buffer used by all of the UDP sock is to the limit, and
> it ought to be counted.
> 
> Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>
> ---
>  net/ipv4/udp.c | 4 +---
>  net/ipv6/udp.c | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 09f0a23d1a01..49a69d8d55b3 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2035,9 +2035,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  		int is_udplite = IS_UDPLITE(sk);
>  
>  		/* Note that an ENOMEM error is charged twice */
> -		if (rc == -ENOMEM)
> -			UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS,
> -					is_udplite);
> +		UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS, is_udplite);
>  		UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
>  		kfree_skb(skb);
>  		trace_udp_fail_queue_rcv_skb(rc, sk);
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 29d9691359b9..d5e23b150fd9 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -634,9 +634,7 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  		int is_udplite = IS_UDPLITE(sk);
>  
>  		/* Note that an ENOMEM error is charged twice */
> -		if (rc == -ENOMEM)
> -			UDP6_INC_STATS(sock_net(sk),
> -					 UDP_MIB_RCVBUFERRORS, is_udplite);
> +		UDP6_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS, is_udplite);
>  		UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
>  		kfree_skb(skb);
>  		return -1;

The diffstat is nice, but I'm unsure we can do this kind of change
(well, I really think we should not do it): it will fool any kind of
existing users (application, scripts, admin) currently reading the
above counters and expecting UDP_MIB_RCVBUFERRORS being increased with
the existing schema.

Cheers,

Paolo


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] net: udp: increase UDP_MIB_RCVBUFERRORS when ENOBUFS
  2020-10-26  9:51 ` Paolo Abeni
@ 2020-10-26 12:47   ` Menglong Dong
  0 siblings, 0 replies; 3+ messages in thread
From: Menglong Dong @ 2020-10-26 12:47 UTC (permalink / raw)
  To: Paolo Abeni, davem; +Cc: kuznet, yoshfuji, kuba, netdev, linux-kernel

Hello~

On Mon, Oct 26, 2020 at 5:52 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hello,
>
> On Mon, 2020-10-26 at 17:39 +0800, Menglong Dong wrote:
> > The error returned from __udp_enqueue_schedule_skb is ENOMEM or ENOBUFS.
> > For now, only ENOMEM is counted into UDP_MIB_RCVBUFERRORS in
> > __udp_queue_rcv_skb. UDP_MIB_RCVBUFERRORS should count all of the
> > failed skb because of memory errors during udp receiving, not just because of the limit of sock receive queue. We can see this
> > in __udp4_lib_mcast_deliver:
> >
> >               nskb = skb_clone(skb, GFP_ATOMIC);
> >
> >               if (unlikely(!nskb)) {
> >                       atomic_inc(&sk->sk_drops);
> >                       __UDP_INC_STATS(net, UDP_MIB_RCVBUFERRORS,
> >                                       IS_UDPLITE(sk));
> >                       __UDP_INC_STATS(net, UDP_MIB_INERRORS,
> >                                       IS_UDPLITE(sk));
> >                       continue;
> >               }
> >
> > See, UDP_MIB_RCVBUFERRORS is increased when skb clone failed. From this
> > point, ENOBUFS from __udp_enqueue_schedule_skb should be counted, too.
> > It means that the buffer used by all of the UDP sock is to the limit, and
> > it ought to be counted.
> >
> > Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>
> > ---
> >  net/ipv4/udp.c | 4 +---
> >  net/ipv6/udp.c | 4 +---
> >  2 files changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 09f0a23d1a01..49a69d8d55b3 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -2035,9 +2035,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> >               int is_udplite = IS_UDPLITE(sk);
> >
> >               /* Note that an ENOMEM error is charged twice */
> > -             if (rc == -ENOMEM)
> > -                     UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS,
> > -                                     is_udplite);
> > +             UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS, is_udplite);
> >               UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> >               kfree_skb(skb);
> >               trace_udp_fail_queue_rcv_skb(rc, sk);
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index 29d9691359b9..d5e23b150fd9 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -634,9 +634,7 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> >               int is_udplite = IS_UDPLITE(sk);
> >
> >               /* Note that an ENOMEM error is charged twice */
> > -             if (rc == -ENOMEM)
> > -                     UDP6_INC_STATS(sock_net(sk),
> > -                                      UDP_MIB_RCVBUFERRORS, is_udplite);
> > +             UDP6_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS, is_udplite);
> >               UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> >               kfree_skb(skb);
> >               return -1;
>
> The diffstat is nice, but I'm unsure we can do this kind of change
> (well, I really think we should not do it): it will fool any kind of
> existing users (application, scripts, admin) currently reading the
> above counters and expecting UDP_MIB_RCVBUFERRORS being increased with
> the existing schema.
>
> Cheers,
>
> Paolo
>

Well, your words make sense, this change isn't friendly for the existing users.
It really puzzled me when this ENOBUFS happened, no counters were done and
I hardly figured out what happened.

So, is it a good idea to introduce a 'UDP_MIB_MEMERRORS'?

Cheers,

Menglong Dong

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-26 12:47 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26  9:39 [PATCH] net: udp: increase UDP_MIB_RCVBUFERRORS when ENOBUFS Menglong Dong
2020-10-26  9:51 ` Paolo Abeni
2020-10-26 12:47   ` Menglong Dong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).