* [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).