Add busy poll support for epoll

From what I can tell there is only a couple spots where we are actually

From: Alexander Duyck <[email protected]>

Quoted 1 line

From what I can tell there is only a couple spots where we are actually

checking the return value of sk_busy_loop. As there are only a few consumers of that data, and the data being checked for can be replaced with a check for !skb_queue_empty() we might as well just pull the code out of sk_busy_loop and place it in the spots that actually need it.

Signed-off-by Alexander Duyck <[email protected]>

Acked-by Eric Dumazet <[email protected]>

---
 include/net/busy_poll.h |    5 ++---
 net/core/datagram.c     |    8 ++++++--
 net/core/dev.c          |   25 +++++++++++--------------
 net/sctp/socket.c       |    9 ++++++---
 4 files changed, 25 insertions(+), 22 deletions(-)

include/net/busy_poll.h +2 -3

index b82d6ba70a14..c55760f4820f 100644
@@ -74,7 +74,7 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return time_after(now, end_time);
 }
 
-bool sk_busy_loop(struct sock *sk, int nonblock);
+void sk_busy_loop(struct sock *sk, int nonblock);
 
 #else /* CONFIG_NET_RX_BUSY_POLL */
 static inline unsigned long net_busy_loop_on(void)

@@ -97,9 +97,8 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return true;
 }
 
-static inline bool sk_busy_loop(struct sock *sk, int nonblock)
+static inline void sk_busy_loop(struct sock *sk, int nonblock)
 {
-       return false;
 }
 
 #endif /* CONFIG_NET_RX_BUSY_POLL */

net/core/datagram.c +0 -0

index ea633342ab0d..4608aa245410 100644
@@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags,
                }

net/core/datagram.c +0 -0

        error = -EAGAIN;
 

net/core/dev.c +11 -14

index ab337bf5bbf4..af70eb6ba682 100644
@@ -5060,21 +5060,19 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock)
                do_softirq();
 }
 
-bool sk_busy_loop(struct sock *sk, int nonblock)
+void sk_busy_loop(struct sock *sk, int nonblock)
 {
        unsigned long end_time = !nonblock ? sk_busy_loop_end_time(sk) : 0;
        int (*napi_poll)(struct napi_struct *napi, int budget);
        void *have_poll_lock = NULL;
        struct napi_struct *napi;
        unsigned int napi_id;
-       int rc;
 
 restart:
        napi_id = READ_ONCE(sk->sk_napi_id);
        if (napi_id < MIN_NAPI_ID)
-               return 0;
+               return;
 
-       rc = false;
        napi_poll = NULL;
 
        rcu_read_lock();

@@ -5085,7 +5083,8 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
 
        preempt_disable();
        for (;;) {
-               rc = 0;
+               int work = 0;
+
                local_bh_disable();
                if (!napi_poll) {
                        unsigned long val = READ_ONCE(napi->state);

@@ -5103,12 +5102,12 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
                        have_poll_lock = netpoll_poll_lock(napi);
                        napi_poll = napi->poll;
                }
-               rc = napi_poll(napi, BUSY_POLL_BUDGET);
-               trace_napi_poll(napi, rc, BUSY_POLL_BUDGET);
+               work = napi_poll(napi, BUSY_POLL_BUDGET);
+               trace_napi_poll(napi, work, BUSY_POLL_BUDGET);
 count:
-               if (rc > 0)
+               if (work > 0)
                        __NET_ADD_STATS(sock_net(sk),
-                                       LINUX_MIB_BUSYPOLLRXPACKETS, rc);
+                                       LINUX_MIB_BUSYPOLLRXPACKETS, work);
                local_bh_enable();
 
                if (nonblock || !skb_queue_empty(&sk->sk_receive_queue) ||

@@ -5121,9 +5120,9 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
                        preempt_enable();
                        rcu_read_unlock();
                        cond_resched();
-                       rc = !skb_queue_empty(&sk->sk_receive_queue);
-                       if (rc || busy_loop_timeout(end_time))
-                               return rc;
+                       if (!skb_queue_empty(&sk->sk_receive_queue) ||
+                           busy_loop_timeout(end_time))
+                               return;
                        goto restart;
                }
                cpu_relax();

@@ -5131,10 +5130,8 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
        if (napi_poll)
                busy_poll_stop(napi, have_poll_lock);
        preempt_enable();
-       rc = !skb_queue_empty(&sk->sk_receive_queue);
 out:
        rcu_read_unlock();
-       return rc;
 }
 EXPORT_SYMBOL(sk_busy_loop);
 

net/sctp/socket.c +6 -3

index 72cc3ecf6516..ccc08fc39722 100644
@@ -7518,9 +7518,12 @@ struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags,
                if (sk->sk_shutdown & RCV_SHUTDOWN)
                        break;
 
-               if (sk_can_busy_loop(sk) &&
-                   sk_busy_loop(sk, noblock))
-                       continue;
+               if (sk_can_busy_loop(sk)) {
+                       sk_busy_loop(sk, noblock);
+
+                       if (!skb_queue_empty(&sk->sk_receive_queue))
+                               continue;
+               }
 
                /* User doesn't want to wait.  */
                error = -EAGAIN;

Quoted 6 lines · Alexander Duyck

net/core/datagram.c +0 -0

index ea633342ab0d..4608aa245410 100644
@@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags,
                }

net/core/datagram.c struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags, · line 264

Quoted 10 lines

net/core/datagram.c +6 -2

 
                spin_unlock_irqrestore(&queue->lock, cpu_flags);
-       } while (sk_can_busy_loop(sk) &&
-                sk_busy_loop(sk, flags & MSG_DONTWAIT));
+
+               if (!sk_can_busy_loop(sk))
+                       break;
+
+               sk_busy_loop(sk, flags & MSG_DONTWAIT);
+       } while (!skb_queue_empty(&sk->sk_receive_queue));

since this change I am hitting stalls where it's looping in this while-loop with syzkaller.

It worked prior to this change because sk->sk_napi_id was not set thus sk_busy_loop would make us get out of the loop.

Now, it keeps on looping because there is an skb in the queue with skb->len == 0 and we are peeking with an offset, thus __skb_try_recv_from_queue will return NULL and thus we have no way of getting out of the loop.

I'm not sure what would be the best way to fix it. I don't see why we end up with an skb in the list with skb->len == 0. So, shooting a quick e-mail, maybe somebody has an idea :-)

I have the syzkaller-reproducer if needed.

Hello,

Quoted 48 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Quoted 1 line

From what I can tell there is only a couple spots where we are actually

checking the return value of sk_busy_loop. As there are only a few consumers of that data, and the data being checked for can be replaced with a check for !skb_queue_empty() we might as well just pull the code out of sk_busy_loop and place it in the spots that actually need it.

Signed-off-by Alexander Duyck <[email protected]>

Acked-by Eric Dumazet <[email protected]>

---
 include/net/busy_poll.h |    5 ++---
 net/core/datagram.c     |    8 ++++++--
 net/core/dev.c          |   25 +++++++++++--------------
 net/sctp/socket.c       |    9 ++++++---
 4 files changed, 25 insertions(+), 22 deletions(-)

include/net/busy_poll.h +2 -3

index b82d6ba70a14..c55760f4820f 100644
@@ -74,7 +74,7 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return time_after(now, end_time);
 }
 
-bool sk_busy_loop(struct sock *sk, int nonblock);
+void sk_busy_loop(struct sock *sk, int nonblock);
 
 #else /* CONFIG_NET_RX_BUSY_POLL */
 static inline unsigned long net_busy_loop_on(void)

@@ -97,9 +97,8 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return true;
 }
 
-static inline bool sk_busy_loop(struct sock *sk, int nonblock)
+static inline void sk_busy_loop(struct sock *sk, int nonblock)
 {
-       return false;
 }
 
 #endif /* CONFIG_NET_RX_BUSY_POLL */

net/core/datagram.c +0 -0

index ea633342ab0d..4608aa245410 100644
@@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct
 sock *sk, unsigned int flags,
                }

net/core/datagram.c struct sk_buff *__skb_try_recv_datagram(struct · line 265

Quoted 10 lines

net/core/datagram.c +6 -2

 
                spin_unlock_irqrestore(&queue->lock, cpu_flags);
-       } while (sk_can_busy_loop(sk) &&
-                sk_busy_loop(sk, flags & MSG_DONTWAIT));
+
+               if (!sk_can_busy_loop(sk))
+                       break;
+
+               sk_busy_loop(sk, flags & MSG_DONTWAIT);
+       } while (!skb_queue_empty(&sk->sk_receive_queue));

since this change I am hitting stalls where it's looping in this while-loop with syzkaller.

It worked prior to this change because sk->sk_napi_id was not set thus sk_busy_loop would make us get out of the loop.

Now, it keeps on looping because there is an skb in the queue with skb->len == 0 and we are peeking with an offset, thus __skb_try_recv_from_queue will return NULL and thus we have no way of getting out of the loop.

I'm not sure what would be the best way to fix it. I don't see why we end up with an skb in the list with skb->len == 0. So, shooting a quick e-mail, maybe somebody has an idea :-)

I have the syzkaller-reproducer if needed.

Hi,

Quoted 74 lines · Christoph Paasch

Hello,

Quoted 48 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Quoted 1 line

From what I can tell there is only a couple spots where we are actually

checking the return value of sk_busy_loop. As there are only a few consumers of that data, and the data being checked for can be replaced with a check for !skb_queue_empty() we might as well just pull the code out of sk_busy_loop and place it in the spots that actually need it.

Signed-off-by Alexander Duyck <[email protected]>

Acked-by Eric Dumazet <[email protected]>

---
 include/net/busy_poll.h |    5 ++---
 net/core/datagram.c     |    8 ++++++--
 net/core/dev.c          |   25 +++++++++++--------------
 net/sctp/socket.c       |    9 ++++++---
 4 files changed, 25 insertions(+), 22 deletions(-)

include/net/busy_poll.h +2 -3

index b82d6ba70a14..c55760f4820f 100644
@@ -74,7 +74,7 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return time_after(now, end_time);
 }
 
-bool sk_busy_loop(struct sock *sk, int nonblock);
+void sk_busy_loop(struct sock *sk, int nonblock);
 
 #else /* CONFIG_NET_RX_BUSY_POLL */
 static inline unsigned long net_busy_loop_on(void)

@@ -97,9 +97,8 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return true;
 }
 
-static inline bool sk_busy_loop(struct sock *sk, int nonblock)
+static inline void sk_busy_loop(struct sock *sk, int nonblock)
 {
-       return false;
 }
 
 #endif /* CONFIG_NET_RX_BUSY_POLL */

net/core/datagram.c +0 -0

index ea633342ab0d..4608aa245410 100644
@@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct
 sock *sk, unsigned int flags,
                }

net/core/datagram.c struct sk_buff *__skb_try_recv_datagram(struct · line 265

Quoted 10 lines

net/core/datagram.c +6 -2

 
                spin_unlock_irqrestore(&queue->lock, cpu_flags);
-       } while (sk_can_busy_loop(sk) &&
-                sk_busy_loop(sk, flags & MSG_DONTWAIT));
+
+               if (!sk_can_busy_loop(sk))
+                       break;
+
+               sk_busy_loop(sk, flags & MSG_DONTWAIT);
+       } while (!skb_queue_empty(&sk->sk_receive_queue));

since this change I am hitting stalls where it's looping in this while-loop with syzkaller.

It worked prior to this change because sk->sk_napi_id was not set thus sk_busy_loop would make us get out of the loop.

Now, it keeps on looping because there is an skb in the queue with skb->len == 0 and we are peeking with an offset, thus __skb_try_recv_from_queue will return NULL and thus we have no way of getting out of the loop.

Quoted 4 lines

I'm not sure what would be the best way to fix it. I don't see why we end up with an skb in the list with skb->len == 0. So, shooting a quick e-mail, maybe somebody has an idea :-)
I have the syzkaller-reproducer if needed.

IIRC we can have 0 len UDP packet sitting on sk_receive_queue since:

commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac

Author: samanthakumar <[email protected]>
Date:   Tue Apr 5 12:41:15 2016 -0400

udp: remove headers from UDP packets before queueing

Both __skb_try_recv_datagram() and napi_busy_loop() assume that we received some packets if the queue is not empty. When peeking such assumption is not true, we should check if the last packet is changed, as __skb_recv_datagram() already does.

So I *think* the root cause of
this issue is older than Alex's patch.

The following - completely untested - should avoid the unbounded loop, but it's not a complete fix, I *think* we should also change sk_busy_loop_end() in a similar way, but that is a little more complex due to the additional indirections.

Could you please test it?

Any feedback welcome!

Could you please test it?

Paolo
---

net/core/datagram.c +1 -1

index b2651bb6d2a3..e657289db4ac 100644
@@ -279,7 +279,7 @@ struct sk_buff *__skb_try_recv_datagram(struct sock
 *sk, unsigned int flags,
                        break;
 
                sk_busy_loop(sk, flags & MSG_DONTWAIT);
-       } while (!skb_queue_empty(&sk->sk_receive_queue));
+       } while (sk->sk_receive_queue.prev != *last);
 
        error = -EAGAIN;

Hi,

Quoted 74 lines · Christoph Paasch

Hello,

Quoted 48 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Quoted 1 line

From what I can tell there is only a couple spots where we are actually

checking the return value of sk_busy_loop. As there are only a few consumers of that data, and the data being checked for can be replaced with a check for !skb_queue_empty() we might as well just pull the code out of sk_busy_loop and place it in the spots that actually need it.

Signed-off-by Alexander Duyck <[email protected]>

Acked-by Eric Dumazet <[email protected]>

---
 include/net/busy_poll.h |    5 ++---
 net/core/datagram.c     |    8 ++++++--
 net/core/dev.c          |   25 +++++++++++--------------
 net/sctp/socket.c       |    9 ++++++---
 4 files changed, 25 insertions(+), 22 deletions(-)

include/net/busy_poll.h +2 -3

index b82d6ba70a14..c55760f4820f 100644
@@ -74,7 +74,7 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return time_after(now, end_time);
 }
 
-bool sk_busy_loop(struct sock *sk, int nonblock);
+void sk_busy_loop(struct sock *sk, int nonblock);
 
 #else /* CONFIG_NET_RX_BUSY_POLL */
 static inline unsigned long net_busy_loop_on(void)

@@ -97,9 +97,8 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return true;
 }
 
-static inline bool sk_busy_loop(struct sock *sk, int nonblock)
+static inline void sk_busy_loop(struct sock *sk, int nonblock)
 {
-       return false;
 }
 
 #endif /* CONFIG_NET_RX_BUSY_POLL */

net/core/datagram.c +0 -0

index ea633342ab0d..4608aa245410 100644
@@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct
 sock *sk, unsigned int flags,
                }

net/core/datagram.c struct sk_buff *__skb_try_recv_datagram(struct · line 265

Quoted 10 lines

net/core/datagram.c +6 -2

 
                spin_unlock_irqrestore(&queue->lock, cpu_flags);
-       } while (sk_can_busy_loop(sk) &&
-                sk_busy_loop(sk, flags & MSG_DONTWAIT));
+
+               if (!sk_can_busy_loop(sk))
+                       break;
+
+               sk_busy_loop(sk, flags & MSG_DONTWAIT);
+       } while (!skb_queue_empty(&sk->sk_receive_queue));

since this change I am hitting stalls where it's looping in this while-loop with syzkaller.

It worked prior to this change because sk->sk_napi_id was not set thus sk_busy_loop would make us get out of the loop.

Now, it keeps on looping because there is an skb in the queue with skb->len == 0 and we are peeking with an offset, thus __skb_try_recv_from_queue will return NULL and thus we have no way of getting out of the loop.

Quoted 5 lines

I'm not sure what would be the best way to fix it. I don't see why we end up with an skb in the list with skb->len == 0. So, shooting a quick e-mail, maybe somebody has an idea :-)

I have the syzkaller-reproducer if needed.

IIRC we can have 0 len UDP packet sitting on sk_receive_queue since:

commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac

Author: samanthakumar <[email protected]>
Date:   Tue Apr 5 12:41:15 2016 -0400

udp: remove headers from UDP packets before queueing

Both __skb_try_recv_datagram() and napi_busy_loop() assume that we received some packets if the queue is not empty. When peeking such assumption is not true, we should check if the last packet is changed, as __skb_recv_datagram() already does. So I *think* the root cause of this issue is older than Alex's patch.

The following - completely untested - should avoid the unbounded loop, but it's not a complete fix, I *think* we should also change sk_busy_loop_end() in a similar way, but that is a little more complex due to the additional indirections.

Could you please test it?

Any feedback welcome!

Could you please test it?

Paolo
---

net/core/datagram.c +1 -1

index b2651bb6d2a3..e657289db4ac 100644
@@ -279,7 +279,7 @@ struct sk_buff *__skb_try_recv_datagram(struct sock
 *sk, unsigned int flags,
                        break;
 
                sk_busy_loop(sk, flags & MSG_DONTWAIT);
-       } while (!skb_queue_empty(&sk->sk_receive_queue));
+       } while (sk->sk_receive_queue.prev != *last);
 
        error = -EAGAIN;

Quoted 90 lines · Paolo Abeni

Hi,

Quoted 74 lines · Christoph Paasch

Hello,

Quoted 48 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Quoted 1 line

From what I can tell there is only a couple spots where we are actually

checking the return value of sk_busy_loop. As there are only a few consumers of that data, and the data being checked for can be replaced with a check for !skb_queue_empty() we might as well just pull the code out of sk_busy_loop and place it in the spots that actually need it.

Signed-off-by Alexander Duyck <[email protected]>

Acked-by Eric Dumazet <[email protected]>

---
 include/net/busy_poll.h |    5 ++---
 net/core/datagram.c     |    8 ++++++--
 net/core/dev.c          |   25 +++++++++++--------------
 net/sctp/socket.c       |    9 ++++++---
 4 files changed, 25 insertions(+), 22 deletions(-)

include/net/busy_poll.h +2 -3

index b82d6ba70a14..c55760f4820f 100644
@@ -74,7 +74,7 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return time_after(now, end_time);
 }
 
-bool sk_busy_loop(struct sock *sk, int nonblock);
+void sk_busy_loop(struct sock *sk, int nonblock);
 
 #else /* CONFIG_NET_RX_BUSY_POLL */
 static inline unsigned long net_busy_loop_on(void)

@@ -97,9 +97,8 @@ static inline bool busy_loop_timeout(unsigned long end_time)
        return true;
 }
 
-static inline bool sk_busy_loop(struct sock *sk, int nonblock)
+static inline void sk_busy_loop(struct sock *sk, int nonblock)
 {
-       return false;
 }
 
 #endif /* CONFIG_NET_RX_BUSY_POLL */

net/core/datagram.c +0 -0

index ea633342ab0d..4608aa245410 100644
@@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct
 sock *sk, unsigned int flags,
                }

net/core/datagram.c struct sk_buff *__skb_try_recv_datagram(struct · line 265

Quoted 10 lines

net/core/datagram.c +6 -2

 
                spin_unlock_irqrestore(&queue->lock, cpu_flags);
-       } while (sk_can_busy_loop(sk) &&
-                sk_busy_loop(sk, flags & MSG_DONTWAIT));
+
+               if (!sk_can_busy_loop(sk))
+                       break;
+
+               sk_busy_loop(sk, flags & MSG_DONTWAIT);
+       } while (!skb_queue_empty(&sk->sk_receive_queue));

since this change I am hitting stalls where it's looping in this while-loop with syzkaller.

It worked prior to this change because sk->sk_napi_id was not set thus sk_busy_loop would make us get out of the loop.

Now, it keeps on looping because there is an skb in the queue with skb->len == 0 and we are peeking with an offset, thus __skb_try_recv_from_queue will return NULL and thus we have no way of getting out of the loop.

Quoted 5 lines

I'm not sure what would be the best way to fix it. I don't see why we end up with an skb in the list with skb->len == 0. So, shooting a quick e-mail, maybe somebody has an idea :-)

I have the syzkaller-reproducer if needed.

IIRC we can have 0 len UDP packet sitting on sk_receive_queue since:

commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac

Author: samanthakumar <[email protected]>
Date:   Tue Apr 5 12:41:15 2016 -0400

Quoted 7 lines

udp: remove headers from UDP packets before queueing

Both __skb_try_recv_datagram() and napi_busy_loop() assume that we received some packets if the queue is not empty. When peeking such assumption is not true, we should check if the last packet is changed, as __skb_recv_datagram() already does. So I *think* the root cause of this issue is older than Alex's patch.

I agree.

Quoted 4 lines

The following - completely untested - should avoid the unbounded loop, but it's not a complete fix, I *think* we should also change sk_busy_loop_end() in a similar way, but that is a little more complex due to the additional indirections.

As far as sk_busy_loop_end we could look at just forking sk_busy_loop and writing a separate implementation for datagram sockets that uses a different loop_end function. It shouldn't take much to change since all we would need to do is pass a structure containing the sk and last pointers instead of just passing the sk directly as the loop_end argument.

Quoted 3 lines

Could you please test it?

Any feedback welcome!

The change below looks good to me.

Quoted 20 lines

Could you please test it?

Paolo
---

net/core/datagram.c +1 -1

index b2651bb6d2a3..e657289db4ac 100644
@@ -279,7 +279,7 @@ struct sk_buff *__skb_try_recv_datagram(struct sock
 *sk, unsigned int flags,
                        break;
 
                sk_busy_loop(sk, flags & MSG_DONTWAIT);
-       } while (!skb_queue_empty(&sk->sk_receive_queue));
+       } while (sk->sk_receive_queue.prev != *last);
 
        error = -EAGAIN;

Quoted 17 lines · Alexander Duyck

Quoted 4 lines · Paolo Abeni

The following - completely untested - should avoid the unbounded loop, but it's not a complete fix, I *think* we should also change sk_busy_loop_end() in a similar way, but that is a little more complex due to the additional indirections.

As far as sk_busy_loop_end we could look at just forking sk_busy_loop and writing a separate implementation for datagram sockets that uses a different loop_end function. It shouldn't take much to change since all we would need to do is pass a structure containing the sk and last pointers instead of just passing the sk directly as the loop_end argument.

Quoted 3 lines

Could you please test it?

Any feedback welcome!

Quoted 1 line

The change below looks good to me.

I just tried it out. Worked for me!

You can add my Tested-by if you do a formal patch-submission:

Tested-by Christoph Paasch <[email protected]>

Hi,

Quoted 25 lines · Christoph Paasch

Quoted 13 lines · Alexander Duyck

Quoted 4 lines · Paolo Abeni

The following - completely untested - should avoid the unbounded loop, but it's not a complete fix, I *think* we should also change sk_busy_loop_end() in a similar way, but that is a little more complex due to the additional indirections.

As far as sk_busy_loop_end we could look at just forking sk_busy_loop and writing a separate implementation for datagram sockets that uses a different loop_end function. It shouldn't take much to change since all we would need to do is pass a structure containing the sk and last pointers instead of just passing the sk directly as the loop_end argument.

Quoted 5 lines

Quoted 3 lines

Could you please test it?

Any feedback welcome!

The change below looks good to me.

I just tried it out. Worked for me!

You can add my Tested-by if you do a formal patch-submission:

Quoted 1 line

Tested-by Christoph Paasch <[email protected]>

Thanks for testing!

I'm trying to reproduce the issue locally, but I'm unable. I think that the current UDP implementation is not affected, as we always ensure sk_receive_queue is empty before busy polling.

Unix sockets should not
be affected, too, as busy polling should not have any effect there (sk_napi_id should be never >= MIN_NAPI_ID). Can you reproduce the issue on an unpatched, recent, upstream kernel?

Can you please provide the syzkaller repro?

Thanks,

Paolo

Quoted 30 lines · Paolo Abeni

Hi,

Quoted 26 lines · Christoph Paasch

Quoted 13 lines · Alexander Duyck

Quoted 4 lines · Paolo Abeni

The following - completely untested - should avoid the unbounded loop, but it's not a complete fix, I *think* we should also change sk_busy_loop_end() in a similar way, but that is a little more complex due to the additional indirections.

As far as sk_busy_loop_end we could look at just forking sk_busy_loop and writing a separate implementation for datagram sockets that uses a different loop_end function. It shouldn't take much to change since all we would need to do is pass a structure containing the sk and last pointers instead of just passing the sk directly as the loop_end argument.

Quoted 5 lines

Quoted 3 lines

Could you please test it?

Any feedback welcome!

The change below looks good to me.

I just tried it out. Worked for me!

You can add my Tested-by if you do a formal patch-submission:

Tested-by Christoph Paasch <[email protected]>

Quoted 5 lines

Thanks for testing!

I'm trying to reproduce the issue locally, but I'm unable. I think that the current UDP implementation is not affected, as we always ensure sk_receive_queue is empty before busy polling.

But right after check is done we release the queue lock, so a packet might come right after the test has been done.

Quoted 19 lines · Christoph Paasch

Quoted 17 lines · Alexander Duyck

Quoted 4 lines · Paolo Abeni

The following - completely untested - should avoid the unbounded loop, but it's not a complete fix, I *think* we should also change sk_busy_loop_end() in a similar way, but that is a little more complex due to the additional indirections.

As far as sk_busy_loop_end we could look at just forking sk_busy_loop and writing a separate implementation for datagram sockets that uses a different loop_end function. It shouldn't take much to change since all we would need to do is pass a structure containing the sk and last pointers instead of just passing the sk directly as the loop_end argument.

Quoted 3 lines

Could you please test it?

Any feedback welcome!

Quoted 7 lines

Quoted 1 line

The change below looks good to me.

I just tried it out. Worked for me!

You can add my Tested-by if you do a formal patch-submission:

Tested-by Christoph Paasch <[email protected]>

Thanks for testing!

I'm trying to reproduce the issue locally, but I'm unable. I think that the current UDP implementation is not affected, as we always ensure sk_receive_queue is empty before busy polling. Unix sockets should not be affected, too, as busy polling should not have any effect there (sk_napi_id should be never >= MIN_NAPI_ID). Can you reproduce the issue on an unpatched, recent, upstream kernel?

Can you please provide the syzkaller repro?

From: Alexander Duyck <[email protected]>

This patch flips the logic we were using to determine if the busy polling has timed out. The main motivation for this is that we will need to support two different possible timeout values in the future and by recording the start time rather than when we would want to end we can focus on making the end_time specific to the task be it epoll or socket based polling.

Signed-off-by Alexander Duyck <[email protected]>

---

From: Sridhar Samudrala <[email protected]>

This patch adds busy poll support to epoll. The implementation is meant to be opportunistic in that it will take the NAPI ID from the last socket that is added to the ready list that contains a valid NAPI ID and it will use that for busy polling until the ready list goes empty. Once the ready list goes empty the NAPI ID is reset and busy polling is disabled until a new socket is added to the ready list.

In addition when we insert a new socket into the epoll we record the NAPI ID and assume we are going to receive events on it. If that doesn't occur it will be evicted as the active NAPI ID and we will resume normal behavior.

An application can use SO_INCOMING_CPU or SO_REUSEPORT_ATTACH_C/EBPF socket options to spread the incoming connections to specific worker threads based on the incoming queue. This enables epoll for each worker thread to have only sockets that receive packets from a single queue. So when an application calls epoll_wait() and there are no events available to report, busy polling is done on the associated queue to pull the packets.

Signed-off-by Sridhar Samudrala <[email protected]>

Signed-off-by Alexander Duyck <[email protected]>

---
 fs/eventpoll.c |   93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

v3: Split off the code for limiting busy_poll and busy_read into a separate
patch for net.
Updated patch that changed busy loop time tracking so that it uses "local_clock() >> 10" as we originally did.
Tweaked "Change return type.." patch by moving declaration of "work" inside the loop where is was accessed and always reset to 0. Added "Acked-by" for patches that received acks.

This patch set adds support for using busy polling with epoll.