| Message ID | 20241126175402.1506-1-ffmancera@riseup.net (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] udp: call sock_def_readable() if socket is not SOCK_FASYNC | expand |
On Tue, Nov 26, 2024 at 6:56 PM Fernando Fernandez Mancera <ffmancera@riseup.net> wrote: > > If a socket is not SOCK_FASYNC, sock_def_readable() needs to be called > even if receive queue was not empty. Otherwise, if several threads are > listening on the same socket with blocking recvfrom() calls they might > hang waiting for data to be received. > SOCK_FASYNC seems completely orthogonal to the issue. First sock_def_readable() should wakeup all threads, I wonder what is happening. UDP can store incoming packets into sk->sk_receive_queue and udp_sk(sk)->reader_queue Paolo, should __skb_wait_for_more_packets() for UDP socket look at both queues ?
On Tue, Nov 26, 2024 at 7:32 PM Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Nov 26, 2024 at 6:56 PM Fernando Fernandez Mancera > <ffmancera@riseup.net> wrote: > > > > If a socket is not SOCK_FASYNC, sock_def_readable() needs to be called > > even if receive queue was not empty. Otherwise, if several threads are > > listening on the same socket with blocking recvfrom() calls they might > > hang waiting for data to be received. > > > > SOCK_FASYNC seems completely orthogonal to the issue. > > First sock_def_readable() should wakeup all threads, I wonder what is happening. Oh well, __skb_wait_for_more_packets() is using prepare_to_wait_exclusive(), so in this case sock_def_readable() is waking only one thread. > > UDP can store incoming packets into sk->sk_receive_queue and > udp_sk(sk)->reader_queue > > Paolo, should __skb_wait_for_more_packets() for UDP socket look at both queues ?
Hi, On 26/11/2024 19:41, Eric Dumazet wrote: > On Tue, Nov 26, 2024 at 7:32 PM Eric Dumazet <edumazet@google.com> wrote: >> >> On Tue, Nov 26, 2024 at 6:56 PM Fernando Fernandez Mancera >> <ffmancera@riseup.net> wrote: >>> >>> If a socket is not SOCK_FASYNC, sock_def_readable() needs to be called >>> even if receive queue was not empty. Otherwise, if several threads are >>> listening on the same socket with blocking recvfrom() calls they might >>> hang waiting for data to be received. >>> >> >> SOCK_FASYNC seems completely orthogonal to the issue. >> >> First sock_def_readable() should wakeup all threads, I wonder what is happening. > Well, it might be. But I noticed that if SOCK_FASYNC is set then sk_wake_async_rcu() do its work and everything is fine. This is why I thought checking on the flag was a good idea. > Oh well, __skb_wait_for_more_packets() is using > prepare_to_wait_exclusive(), so in this case sock_def_readable() is > waking only one thread. > Yes, this is what I was expecting. What would be the solution? Should I change it to "prepare_to_wait()" instead? Although, I don't know the implication that change might have. Thank you for the comments, first time working on UDP socket implementation so they are very helpful :) >> >> UDP can store incoming packets into sk->sk_receive_queue and >> udp_sk(sk)->reader_queue >> >> Paolo, should __skb_wait_for_more_packets() for UDP socket look at both queues ?
On Tue, Nov 26, 2024 at 8:18 PM Fernando F. Mancera <ffmancera@riseup.net> wrote: > > Hi, > > On 26/11/2024 19:41, Eric Dumazet wrote: > > On Tue, Nov 26, 2024 at 7:32 PM Eric Dumazet <edumazet@google.com> wrote: > >> > >> On Tue, Nov 26, 2024 at 6:56 PM Fernando Fernandez Mancera > >> <ffmancera@riseup.net> wrote: > >>> > >>> If a socket is not SOCK_FASYNC, sock_def_readable() needs to be called > >>> even if receive queue was not empty. Otherwise, if several threads are > >>> listening on the same socket with blocking recvfrom() calls they might > >>> hang waiting for data to be received. > >>> > >> > >> SOCK_FASYNC seems completely orthogonal to the issue. > >> > >> First sock_def_readable() should wakeup all threads, I wonder what is happening. > > > > Well, it might be. But I noticed that if SOCK_FASYNC is set then > sk_wake_async_rcu() do its work and everything is fine. This is why I > thought checking on the flag was a good idea. > How have you tested SOCK_FASYNC ? SOCK_FASYNC is sending signals. If SIGIO is blocked, I am pretty sure the bug is back. > > Oh well, __skb_wait_for_more_packets() is using > > prepare_to_wait_exclusive(), so in this case sock_def_readable() is > > waking only one thread. > > > > Yes, this is what I was expecting. What would be the solution? Should I > change it to "prepare_to_wait()" instead? Although, I don't know the > implication that change might have. Sadly, we will have to revert, this exclusive wake is subtle.
On 26/11/2024 20:26, Eric Dumazet wrote: > On Tue, Nov 26, 2024 at 8:18 PM Fernando F. Mancera > <ffmancera@riseup.net> wrote: >> >> Hi, >> >> On 26/11/2024 19:41, Eric Dumazet wrote: >>> On Tue, Nov 26, 2024 at 7:32 PM Eric Dumazet <edumazet@google.com> wrote: >>>> >>>> On Tue, Nov 26, 2024 at 6:56 PM Fernando Fernandez Mancera >>>> <ffmancera@riseup.net> wrote: >>>>> >>>>> If a socket is not SOCK_FASYNC, sock_def_readable() needs to be called >>>>> even if receive queue was not empty. Otherwise, if several threads are >>>>> listening on the same socket with blocking recvfrom() calls they might >>>>> hang waiting for data to be received. >>>>> >>>> >>>> SOCK_FASYNC seems completely orthogonal to the issue. >>>> >>>> First sock_def_readable() should wakeup all threads, I wonder what is happening. >>> >> >> Well, it might be. But I noticed that if SOCK_FASYNC is set then >> sk_wake_async_rcu() do its work and everything is fine. This is why I >> thought checking on the flag was a good idea. >> > > How have you tested SOCK_FASYNC ? > > SOCK_FASYNC is sending signals. If SIGIO is blocked, I am pretty sure > the bug is back. > Ah, I didn't know SIGIO was going to be blocked. > >>> Oh well, __skb_wait_for_more_packets() is using >>> prepare_to_wait_exclusive(), so in this case sock_def_readable() is >>> waking only one thread. >>> >> >> Yes, this is what I was expecting. What would be the solution? Should I >> change it to "prepare_to_wait()" instead? Although, I don't know the >> implication that change might have. > > Sadly, we will have to revert, this exclusive wake is subtle. If that is the case, let me send a revert patch. Thanks for the fast replies :)
On Tue, Nov 26, 2024 at 8:30 PM Fernando F. Mancera <ffmancera@riseup.net> wrote: > > > > On 26/11/2024 20:26, Eric Dumazet wrote: > > On Tue, Nov 26, 2024 at 8:18 PM Fernando F. Mancera > > <ffmancera@riseup.net> wrote: > >> > >> Hi, > >> > >> On 26/11/2024 19:41, Eric Dumazet wrote: > >>> On Tue, Nov 26, 2024 at 7:32 PM Eric Dumazet <edumazet@google.com> wrote: > >>>> > >>>> On Tue, Nov 26, 2024 at 6:56 PM Fernando Fernandez Mancera > >>>> <ffmancera@riseup.net> wrote: > >>>>> > >>>>> If a socket is not SOCK_FASYNC, sock_def_readable() needs to be called > >>>>> even if receive queue was not empty. Otherwise, if several threads are > >>>>> listening on the same socket with blocking recvfrom() calls they might > >>>>> hang waiting for data to be received. > >>>>> > >>>> > >>>> SOCK_FASYNC seems completely orthogonal to the issue. > >>>> > >>>> First sock_def_readable() should wakeup all threads, I wonder what is happening. > >>> > >> > >> Well, it might be. But I noticed that if SOCK_FASYNC is set then > >> sk_wake_async_rcu() do its work and everything is fine. This is why I > >> thought checking on the flag was a good idea. > >> > > > > How have you tested SOCK_FASYNC ? > > > > SOCK_FASYNC is sending signals. If SIGIO is blocked, I am pretty sure > > the bug is back. > > > > Ah, I didn't know SIGIO was going to be blocked. > > > > >>> Oh well, __skb_wait_for_more_packets() is using > >>> prepare_to_wait_exclusive(), so in this case sock_def_readable() is > >>> waking only one thread. > >>> > >> > >> Yes, this is what I was expecting. What would be the solution? Should I > >> change it to "prepare_to_wait()" instead? Although, I don't know the > >> implication that change might have. > > > > Sadly, we will have to revert, this exclusive wake is subtle. > > If that is the case, let me send a revert patch. Thanks for the fast > replies :) Please wait one day before sending a new patch, thanks.
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 6a01905d379f..4c398e6945ee 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1722,6 +1722,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) if (!sock_flag(sk, SOCK_DEAD)) { if (becomes_readable || sk->sk_data_ready != sock_def_readable || + !sock_flag(sk, SOCK_FASYNC) || READ_ONCE(sk->sk_peek_off) >= 0) INDIRECT_CALL_1(sk->sk_data_ready, sock_def_readable, sk);
If a socket is not SOCK_FASYNC, sock_def_readable() needs to be called even if receive queue was not empty. Otherwise, if several threads are listening on the same socket with blocking recvfrom() calls they might hang waiting for data to be received. Link: https://bugzilla.redhat.com/2308477 Fixes: 612b1c0dec5b ("udp: avoid calling sock_def_readable() if possible") Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net> --- net/ipv4/udp.c | 1 + 1 file changed, 1 insertion(+)