| Message ID | 20241126175402.1506-1-ffmancera@riseup.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| 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.
Hi, I'm sorry for the latency here. On 11/26/24 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. > > 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. Very likely whatever I'll add here will be of little use, still... AFAICS prepare_to_wait_exclusive() is there since pre git times, so its usage not be the cause of behaviors changes. >> 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 ? That in case multiple threads are woken-up and thread-1 splices sk_receive_queue into reader_queue before thread-2 has any chance of checking the first, I guess? With prepare_to_wait_exclusive, checking a single queue should be ok. /P
Hi Paolo, On 27/11/2024 08:46, Paolo Abeni wrote: > Hi, > > I'm sorry for the latency here. > > On 11/26/24 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. >> >> 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. > > Very likely whatever I'll add here will be of little use, still... > > AFAICS prepare_to_wait_exclusive() is there since pre git times, so its > usage not be the cause of behaviors changes. > I don't think the problem is on the usage of prepare_to_wait_exclusive() but on the fact that after 612b1c0dec5bc7367f90fc508448b8d0d7c05414 sock_def_readable() is not being called everytime __udp_enqueue_schedule_skb() is called, instead it is called when the socket was empty before. On a single thread scenario this is completely fine but some issues were reported on scenarios involving multiple threads. Please see: https://bugzilla.redhat.com/2308477 >>> 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 ? > > That in case multiple threads are woken-up and thread-1 splices > sk_receive_queue into reader_queue before thread-2 has any chance of > checking the first, I guess? > > With prepare_to_wait_exclusive, checking a single queue should be ok. > The scenario I have to reproduce this easily is the following: 1. Create 1 UDP socket 2. Create 5 threads and let them run into a blocking recvfrom() 3. Send 5 small UDP datagrams from main thread to the UDP socket 4. The 5 threads independently terminate after receiving 1 datagram. This, doesn't work after 612b1c0dec5bc7367f90fc508448b8d0d7c05414 as the first time we call __udp_enqueue_schedule_skb() we call sock_def_readable() and it wakes up a single thread which receives the packet and terminates. The following times we call __udp_enqueue_schedule_skb() we are avoiding the sock_def_readable() call. This causes the other threads are not being woke up and they are blocked waiting on the recvfrom() forever. Thank you, Fernando. > /P >
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(+)