[PATCH 1/2] net/core: use local_bh_disable() in netif_rx_ni()

[PATCH 1/2] net/core: use local_bh_disable() in netif_rx_ni()

[Sebastian Andrzej Siewior]

In 2004 [0] netif_rx_ni() gained a preempt_disable() section around
netif_rx() and its do_softirq() + testing for it. The do_softirq() part
is required because netif_rx() raises the softirq but does not invoke
it. The preempt_disable() is required to avoid running the BH in
parallel.
All this can be avoided be putting this into a local_bh_disable()ed
section. The local_bh_enable() part will invoke do_softirq() if
required.

[0] Make netif_rx_ni preempt-safe
    http://oss.sgi.com/projects/netdev/archive/2004-10/msg02211.html

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/core/dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b8d6dd9e8b5c..b1f8a89322bd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3928,11 +3928,9 @@ int netif_rx_ni(struct sk_buff *skb)
 
 	trace_netif_rx_ni_entry(skb);
 
-	preempt_disable();
+	local_bh_disable();
 	err = netif_rx_internal(skb);
-	if (local_softirq_pending())
-		do_softirq();
-	preempt_enable();
+	local_bh_enable();
 
 	return err;
 }
-- 
2.11.0

[PATCH 2/2] net/core: remove explicit do_softirq() from busy_poll_stop()

[Sebastian Andrzej Siewior]

Since commit 217f69743681 ("net: busy-poll: allow preemption in
sk_busy_loop()") there is an explicit do_softirq() invocation after
local_bh_enable() has been invoked.
I don't understand why we need this because local_bh_enable() will
invoke do_softirq() once the softirq counter reached zero and we have
softirq-related work pending.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/core/dev.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b1f8a89322bd..447d37fe89e6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5281,8 +5281,6 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock)
 	if (rc == BUSY_POLL_BUDGET)
 		__napi_schedule(napi);
 	local_bh_enable();
-	if (local_softirq_pending())
-		do_softirq();
 }
 
 void napi_busy_loop(unsigned int napi_id,
-- 
2.11.0

Re: [PATCH 1/2] net/core: use local_bh_disable() in netif_rx_ni()

[David Miller]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri, 16 Jun 2017 19:23:59 +0200

> In 2004 [0] netif_rx_ni() gained a preempt_disable() section around
> netif_rx() and its do_softirq() + testing for it. The do_softirq() part
> is required because netif_rx() raises the softirq but does not invoke
> it. The preempt_disable() is required to avoid running the BH in
> parallel.
> All this can be avoided be putting this into a local_bh_disable()ed
> section. The local_bh_enable() part will invoke do_softirq() if
> required.
> 
> [0] Make netif_rx_ni preempt-safe
>     http://oss.sgi.com/projects/netdev/archive/2004-10/msg02211.html
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Why make extra work?  The current code is cheaper.

Doing all of that dance with the local_bh_enable() function call is
more expensive than the inlined counter bump and softirq state
check.

I'm not applying this without a better justification, sorry.

Re: [PATCH 2/2] net/core: remove explicit do_softirq() from busy_poll_stop()

[David Miller]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri, 16 Jun 2017 19:24:00 +0200

> Since commit 217f69743681 ("net: busy-poll: allow preemption in
> sk_busy_loop()") there is an explicit do_softirq() invocation after
> local_bh_enable() has been invoked.
> I don't understand why we need this because local_bh_enable() will
> invoke do_softirq() once the softirq counter reached zero and we have
> softirq-related work pending.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

This indeed is superfluous, applied, thanks.