NOHZ: local_softirq_pending 08

With kernel 2.6.32-rc3-00052-g0eca52a I got following KERN_ERR messages just while using firefox:

[  130.527399] NOHZ: local_softirq_pending 08

Any idea? or known issue?

Quoted 6 lines · Dave Young

With kernel 2.6.32-rc3-00052-g0eca52a I got following KERN_ERR messages just while using firefox:

[  130.527399] NOHZ: local_softirq_pending 08

Any idea? or known issue?

Are you using b43 (or wl12x1)? If so, it's a known issue, but the driver was recently left without an active maintainer in a brouhaha about a bug fix.

Cf. http://thread.gmane.org/gmane.linux.kernel.wireless.general/39440

Absent proof that mac80211 is safe to run with BHs enabled, the correct solution is disabling tasklets around the RX function, unlike all the proposed patches. However, Michael thinks it's such a bad solution that he has refused to implement it.

FWIW, I believe the bug to be in b43 and wl12x1, and not as Michael thinks in the stack.

Ehm, no. That's not exactly true.
We call the non-_irqsafe functions, which by definition are designed to run in non-irq (soft or hard) context. At least that's how I understand the documentation, last time I read it.

Why don't you simply do local_bh_disable() in those functions, if they require bh disabled, instead of depending on the driver doing it?

Quoted 2 lines

FWIW, I believe the bug to be in b43 and wl12x1, and not as Michael thinks in the stack.

If mac80211 requires BHs disabled, it should do this.

Quoted 4 lines · Dave Young

FWIW, I believe the bug to be in b43 and wl12x1, and not as Michael thinks in the stack.

If mac80211 requires BHs disabled, it should do this.

Quoted 3 lines · Dave Young

With kernel 2.6.32-rc3-00052-g0eca52a I got following KERN_ERR messages just while using firefox:

Quoted 3 lines

[  130.527399] NOHZ: local_softirq_pending 08

Any idea? or known issue?

Are you using b43 (or wl12x1)? If so, it's a known issue, but the driver was recently left without an active maintainer in a brouhaha about a bug fix.

Cf. http://thread.gmane.org/gmane.linux.kernel.wireless.general/39440

Absent proof that mac80211 is safe to run with BHs enabled, the correct solution is disabling tasklets around the RX function, unlike all the proposed patches. However, Michael thinks it's such a bad solution that he has refused to implement it. So far, nobody has bothered to fix the drivers.

FWIW, I believe the bug to be in b43 and wl12x1, and not as Michael thinks in the stack.

johannes

Quoted 4 lines · Dave Young

With kernel 2.6.32-rc3-00052-g0eca52a I got following KERN_ERR messages just while using firefox:

[  130.527399] NOHZ: local_softirq_pending 08

Quoted 1 line

Any idea? or known issue?

Are you using b43 (or wl12x1)? If so, it's a known issue, but the driver was recently left without an active maintainer in a brouhaha about a bug fix.

Cf. http://thread.gmane.org/gmane.linux.kernel.wireless.general/39440

Can you explain a bit more what that message is about? I am encountering it in a completely different context (PPP over ISDN) and I would like to know where to start looking for the cause and developing a fix. The thread on linux.kernel.wireless.general only seems to address the specific situation in the wireless stack.

Quoted 4 lines

Can you explain a bit more what that message is about? I am encountering it in a completely different context (PPP over ISDN) [...]

Basically, calling netif_rx() with softirqs enabled.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[CCing PPP people]

Am 11.10.2009 13:40 schrieb Johannes Berg:

Quoted 7 lines · Tilman Schmidt

Quoted 4 lines

Can you explain a bit more what that message is about? I am encountering it in a completely different context (PPP over ISDN) [...]

Basically, calling netif_rx() with softirqs enabled.

AFAICS that would have to be the netif_rx() call in ppp_receive_nonmp_frame() [drivers/net/ppp_generic.c#L1791], called (via others) from the tasklet work function ppp_sync_process() [drivers/net/ppp_synctty.c#L545]. Should that be changed to the
"if (in_interrupt()) netif_rx(skb) else netif_rx_ni(skb)" stanza from the linux.kernel.wireless.general thread then?

The PPP receive paths in ppp_generic.c do a local_bh_disable()/ local_bh_enable() around packet receiving (via ppp_recv_lock()/ ppp_recv_unlock() in ppp_do_recv).

So at least that part is perfectly fine.

ppp_input(), as called from ppp_sync_process(), also disables BH's around ppp_do_recv() calls (via read_lock_bh()/read_unlock_bh()).

So that's fine too.

Do you have a bug report or are you just scanning around looking for trouble? :-)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 6 lines · David Miller

The PPP receive paths in ppp_generic.c do a local_bh_disable()/ local_bh_enable() around packet receiving (via ppp_recv_lock()/ ppp_recv_unlock() in ppp_do_recv).

So at least that part is perfectly fine.

Quoted 7 lines

ppp_input(), as called from ppp_sync_process(), also disables BH's around ppp_do_recv() calls (via read_lock_bh()/read_unlock_bh()).

So that's fine too.

Do you have a bug report or are you just scanning around looking for trouble? :-)

I have encountered the message in the subject during a test of the Gigaset CAPI driver, and would like to determine whether it's a bug in the driver, a bug somewhere else, or no bug at all. The test scenario was PPP over ISDN with pppd+capiplugin. In an alternative scenario, also PPP over ISDN but with smpppd+capidrv, the message did not occur.

Johannes' answer pointed me to the netif_rx() function. The Gigaset driver itself doesn't call that function at all. In the scenario where I saw the message, it was the SYNC_PPP line discipline that did. But from your explanation I gather that the cause cannot lie there.

So now I'm looking for other possible causes of that message.

Quoted 24 lines

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 6 lines · David Miller

The PPP receive paths in ppp_generic.c do a local_bh_disable()/ local_bh_enable() around packet receiving (via ppp_recv_lock()/ ppp_recv_unlock() in ppp_do_recv).

So at least that part is perfectly fine.

Quoted 7 lines

ppp_input(), as called from ppp_sync_process(), also disables BH's around ppp_do_recv() calls (via read_lock_bh()/read_unlock_bh()).

So that's fine too.

Do you have a bug report or are you just scanning around looking for trouble? :-)

I have encountered the message in the subject during a test of the Gigaset CAPI driver, and would like to determine whether it's a bug in the driver, a bug somewhere else, or no bug at all. The test scenario was PPP over ISDN with pppd+capiplugin. In an alternative scenario, also PPP over ISDN but with smpppd+capidrv, the message did not occur.

Quoted 7 lines

Johannes' answer pointed me to the netif_rx() function. The Gigaset driver itself doesn't call that function at all. In the scenario where I saw the message, it was the SYNC_PPP line discipline that did. But from your explanation I gather that the cause cannot lie there.

So now I'm looking for other possible causes of that message.

Anyway, I agree with Michael Buesch there is no reason to waste time for tracking all netif_rx vs netif_rx_ni uses, and it seems we could avoid it by using the "proper" version of raise_softirq_irqoff() in __napi_schedule(). Could anybody try if I'm not wrong?

Thanks,
Jarek P.
---

 net/core/dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/core/dev.c +1 -2

index 28b0b9e..7fc4009 100644
@@ -2728,7 +2728,7 @@ void __napi_schedule(struct napi_struct *n)
 
 	local_irq_save(flags);
 	list_add_tail(&n->poll_list, &__get_cpu_var(softnet_data).poll_list);
-	__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+	raise_softirq_irqoff(NET_RX_SOFTIRQ);
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL(__napi_schedule);
--

To unsubscribe from this list: send the line "unsubscribe linux-ppp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jarek Poplawski wrote, On 10/15/2009 01:40 PM:

Quoted 8 lines · Tilman Schmidt

Quoted 7 lines

I have encountered the message in the subject during a test of the Gigaset CAPI driver, and would like to determine whether it's a bug in the driver, a bug somewhere else, or no bug at all. The test scenario was PPP over ISDN with pppd+capiplugin. In an alternative scenario, also PPP over ISDN but with smpppd+capidrv, the message did not occur.

Quoted 4 lines

Quoted 4 lines

Johannes' answer pointed me to the netif_rx() function. The Gigaset driver itself doesn't call that function at all. In the scenario where I saw the message, it was the SYNC_PPP line discipline that did.

Quoted 14 lines

Anyway, I agree with Michael Buesch there is no reason to waste time for tracking all netif_rx vs netif_rx_ni uses, and it seems we could avoid it by using the "proper" version of raise_softirq_irqoff() in __napi_schedule(). Could anybody try if I'm not wrong?

 net/core/dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/core/dev.c +0 -0

index 28b0b9e..7fc4009 100644
@@ -2728,7 +2728,7 @@ void __napi_schedule(struct napi_struct *n)
 

net/core/dev.c void __napi_schedule(struct napi_struct *n) · line 2734

Quoted 7 lines

net/core/dev.c +1 -1

 	local_irq_save(flags);
 	list_add_tail(&n->poll_list, &__get_cpu_var(softnet_data).poll_list);
-	__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+	raise_softirq_irqoff(NET_RX_SOFTIRQ);
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL(__napi_schedule);

I have tested your patch and I can confirm that it fixes the messages. I have not noticed any ill effects.

On Wed, Oct 21, 2009 at 08:46:40PM +0200, Tilman Schmidt wrote:
...

Quoted 2 lines

I have tested your patch and I can confirm that it fixes the messages. I have not noticed any ill effects.

OK. So, in any case, here is this next variant/proposal.

Thanks,
Jarek P.
------------------------>
net: Adjust softirq raising in __napi_schedule

This patch changes __raise_softirq_irqoff() to raise_softirq_irqoff() in __napi_schedule() to enable proper softirq scheduling from process context. The main intent is to let use netif_rx() universally, and make netif_rx_ni() redundant.

Currently using netif_rx() instead of netif_rx_ni() triggers:
"NOHZ: local_softirq_pending 08" warnings, but additional cost of one "if" on the fast path doesn't seem to justify maintaining it separately.

This patch is based on the analysis, suggestions and the original patch for mac80211 by: Michael Buesch <[email protected]>

Another patch calling netif_rx variants conditionally was done by:
Oliver Hartkopp <[email protected]>

Reported-by Michael Buesch <[email protected]>

Reported-by Oliver Hartkopp <[email protected]>

Reported-by Tilman Schmidt <[email protected]>

Diagnosed-by Michael Buesch <[email protected]>

Tested-by Tilman Schmidt <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---

 net/core/dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/core/dev.c +0 -0

index 28b0b9e..7fc4009 100644

This still doesn't make any sense.

There may or may not be a lot of code that assumes that everything else is run with other tasklets disabled, and that it cannot be interrupted by a tasklet and thus create a race.

Can you prove that is not the case, across the entire networking layer?

On Wed, 2009-10-21 at 23:19 +0200, Jarek Poplawski wrote:

net/core/dev.c void __napi_schedule(struct napi_struct *n) · line 2731

Quoted 8 lines

net/core/dev.c +1 -1

@@ -2728,7 +2728,7 @@ void __napi_schedule(struct napi_struct *n)
 
 	local_irq_save(flags);
 	list_add_tail(&n->poll_list, &__get_cpu_var(softnet_data).poll_list);
-	__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+	raise_softirq_irqoff(NET_RX_SOFTIRQ);

This still doesn't make any sense.

There may or may not be a lot of code that assumes that everything else is run with other tasklets disabled, and that it cannot be interrupted by a tasklet and thus create a race.

Can you prove that is not the case, across the entire networking layer?

Quoted 9 lines

net/core/dev.c void __napi_schedule(struct napi_struct *n) · line 2731

Quoted 2 lines

net/core/dev.c +1 -1

-	__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+	raise_softirq_irqoff(NET_RX_SOFTIRQ);

This still doesn't make any sense.

There may or may not be a lot of code that assumes that everything else is run with other tasklets disabled, and that it cannot be interrupted by a tasklet and thus create a race.

Quoted 1 line

Can you prove that is not the case, across the entire networking layer?

I'm not sure I can understand your question. This patch is mainly to avoid using netif_rx()/netif_rx_ni() pair as a test of proper process context handling; IMHO there're better tools for this (lockdep, WARN_ON's).

Quoted 4 lines · Jarek Poplawski

I'm not sure I can understand your question. This patch is mainly to avoid using netif_rx()/netif_rx_ni() pair as a test of proper process context handling; IMHO there're better tools for this (lockdep, WARN_ON's).

I'm saying that it seems to me, as indicated by the API (and without proof otherwise that's how it is) the networking layer needs to have packets handed to it with softirqs disabled.

This really should be obvious. You're fixing the warning at the source of the warning, rather than the source of the problem.

Strange. Then what are the two separate functions netif_rx() and netif_rx_ni() for?

Quoted 2 lines

This really should be obvious. You're fixing the warning at the source of the warning, rather than the source of the problem.

Good idea. So please do tell us where the source of the problem is.

Quoted 2 lines · Tilman Schmidt

Strange. Then what are the two separate functions netif_rx() and netif_rx_ni() for?

netif_rx_ni() disables preemption.

[...] the networking layer needs to have
packets handed to it with softirqs disabled.

Quoted 4 lines

Quoted 2 lines

This really should be obvious. You're fixing the warning at the source of the warning, rather than the source of the problem.

Good idea. So please do tell us where the source of the problem is.

You use netif_rx_ni() instead of netif_rx() at whatever place that causes this problem.

Quoted 1 line

netif_rx_ni() disables preemption.

You wrote earlier:

Quoted 2 lines

[...] the networking layer needs to have
packets handed to it with softirqs disabled.

How disabling preemption can fix something which needs softirqs disabled? Could you be more precise?

Quoted 4 lines · Jarek Poplawski

Quoted 1 line

netif_rx_ni() disables preemption.

You wrote earlier:

Quoted 5 lines

Quoted 2 lines

[...] the networking layer needs to have
packets handed to it with softirqs disabled.

How disabling preemption can fix something which needs softirqs disabled? Could you be more precise?

No, I wrote that I didn't know. I suppose now that I looked at it I do know, and only disabling preemption is required.

Quoted 2 lines

No, I wrote that I didn't know. I suppose now that I looked at it I do know, and only disabling preemption is required.

But netif_rx has preemption disabled most of the time (by hardirqs disabling). So maybe disabling preemption isn't the main reason here either?

Quoted 6 lines

Quoted 3 lines

No, I wrote that I didn't know. I suppose now that I looked at it I do know, and only disabling preemption is required.

But netif_rx has preemption disabled most of the time (by hardirqs disabling). So maybe disabling preemption isn't the main reason here either?

Not for netpoll though, which may or may not be relevant (if I were to venture a guess I'd say it isn't and it disables preemption to be able to do the softirq thing)

However, I lost track now of why we're discussing this.

Basically it boils down to using netif_rx() when in (soft)irq, and netif_rx_ni() when in process context. That could just be an optimisation, but it's a very valid one.

Quoted 1 line

However, I lost track now of why we're discussing this.

The starting point were several reports of the kernel message:

NOHZ: local_softirq_pending 08

Originally most if not all of them came from wireless networking, but I muddied the waters by adding to the mix a case involving ISDN. You stated that all the solutions proposed so far were wrong, so we're naturally turning to you for guidance on what the right solution might be.

Quoted 3 lines

Basically it boils down to using netif_rx() when in (soft)irq, and netif_rx_ni() when in process context. That could just be an optimisation, but it's a very valid one.

Hmmm. That seems to contradict your earlier statement to me that simply replacing a call to netif_rx() by one to netif_rx_ni() when not in interrupt context isn't a valid solution either. What am I missing?

Quoted 4 lines

Quoted 4 lines

Basically it boils down to using netif_rx() when in (soft)irq, and netif_rx_ni() when in process context. That could just be an optimisation, but it's a very valid one.

Quoted 4 lines

Hmmm. That seems to contradict your earlier statement to me that simply replacing a call to netif_rx() by one to netif_rx_ni() when not in interrupt context isn't a valid solution either. What am I missing?

Well, I think you misunderstood me. It would be correct to do this, if and only if the code that calls it doesn't need the extra guarantee.

Any code (say ISDN code) that calls netif_rx() is clearly assuming to always be running in (soft)irq context, otherwise it couldn't call netif_rx() unconditionally. Agree so far?

So now if you change the ISDN code to call netif_rx_ni(), you've changed the assumption that the ISDN code makes -- that it is running in (soft)irq context. Therefore, you need to verify that this is actually a correct change, which is what I tried to say.

Quoted 3 lines

Any code (say ISDN code) that calls netif_rx() is clearly assuming to always be running in (soft)irq context, otherwise it couldn't call netif_rx() unconditionally. Agree so far?

Well, in fact I'm not sure. :-) All I know is that in the ISDN case, no such assumption is explicitly stated anywhere. (The code in question is called from the rcvcallb_skb() callback method which the hardware driver calls when data has been received, and the description of that method in Documentation/isdn/INTERFACE does not say anything about the context in which it may be called.) The relevant code in drivers/isdn/i4l/isdn_ppp.c is rather old, perhaps even older than softirqs and the netif_rx() / netif_rx_ni() split. (Bear in mind that we are talking about the old ISDN4Linux subsystem which initially didn't even make it into the 2.6 series because it was considered obsolete.) It seems quite possible to me that just no one ever thought about that question.

Quoted 4 lines

So now if you change the ISDN code to call netif_rx_ni(), you've changed the assumption that the ISDN code makes -- that it is running in (soft)irq context. Therefore, you need to verify that this is actually a correct change, which is what I tried to say.

Understood. However, the fact that the local_softirq_pending message is appearing would seem to indicate that this assumption was wrong to begin with, wouldn't it?

I'm not sure I can understand your question. This patch is mainly to avoid using netif_rx()/netif_rx_ni() pair as a test of proper process context handling; IMHO there're better tools for this (lockdep, WARN_ON's).

From: Jarek Poplawski <[email protected]>
Date: Wed, 21 Oct 2009 23:39:47 +0200

Quoted 4 lines

I'm not sure I can understand your question. This patch is mainly to avoid using netif_rx()/netif_rx_ni() pair as a test of proper process context handling; IMHO there're better tools for this (lockdep, WARN_ON's).

Semantically I think your patch is correct, but I wonder about cost.

Something that is a simply per-cpu inline "or" operation is now a function call and potentially mispredicted branch inside of raise_softirq_irqoff().

And netif_rx() is indeed a fast path for tunnels and other users so this does matter.

I like having people call things in the correct context the function was built for, and thus we can avoiryd completely useless operations and tests as we can now in netif_rx().

Makaing things general purpose costs something, and it costs too much here for this critical routine, sorry.

I was just having a talk with Nick Piggin about these kinds of issues today, too few people care about these ever encrouching tiny pieces of bloat that slow the kernel down gradually over time, and I simply won't stand for it when I notice it :-)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I have encountered the message in the subject during a test of the Gigaset CAPI driver, and would like to determine whether it's a bug in the driver, a bug somewhere else, or no bug at all. The test scenario was PPP over ISDN with pppd+capiplugin. In an alternative scenario, also PPP over ISDN but with smpppd+capidrv, the message did not occur.

I'm sorry, I had confused the two cases. The message occurs in the smpppd+capidrv scenario, not with pppd+capiplugin.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Johannes' answer pointed me to the netif_rx() function. The Gigaset driver itself doesn't call that function at all. In the scenario where I saw the message, it was the SYNC_PPP line discipline that did.

This analysis was therefore wrong. It would be the netif_rx() call towards the end of isdn_ppp_push_higher() in drivers/isdn/i4l/isdn_ppp.c L1177.

drivers/isdn/i4l/isdn_ppp.c isdn_ppp_push_higher(isdn_net_dev * net_dev, isdn_net_local * lp, struct sk_buff · line 1180

Quoted 11 lines · Tilman Schmidt

drivers/isdn/i4l/isdn_ppp.c +4 -1

@@ -1174,7 +1174,10 @@ isdn_ppp_push_higher(isdn_net_dev * net_dev, isdn_net_local * lp, struct sk_buff
 #endif /* CONFIG_IPPP_FILTER */
 	skb->dev = dev;
 	skb_reset_mac_header(skb);
-	netif_rx(skb);
+	if (in_interrupt())
+		netif_rx(skb);
+	else
+		netif_rx_ni(skb);

So you've verified that the entire i4l stack can cope with being called twice on the same CPU, from different contexts?

Quoted 6 lines

Quoted 6 lines

drivers/isdn/i4l/isdn_ppp.c +0 -0

@@ -1174,7 +1174,10 @@ isdn_ppp_push_higher(isdn_net_dev * net_dev, isdn_net_local * lp, struct sk_buff
 #endif /* CONFIG_IPPP_FILTER */
 	skb->dev = dev;
 	skb_reset_mac_header(skb);

Quoted 8 lines

drivers/isdn/i4l/isdn_ppp.c isdn_ppp_push_higher(isdn_net_dev * net_dev, isdn_net_local * lp, struct sk_buff · line 1180

Quoted 5 lines

drivers/isdn/i4l/isdn_ppp.c +4 -1

-	netif_rx(skb);
+	if (in_interrupt())
+		netif_rx(skb);
+	else
+		netif_rx_ni(skb);

So you've verified that the entire i4l stack can cope with being called twice on the same CPU, from different contexts?

What makes you think so?

Quoted 4 lines · Tilman Schmidt

Quoted 2 lines · Johannes Berg

So you've verified that the entire i4l stack can cope with being called twice on the same CPU, from different contexts?

What makes you think so?

I thought I'd explained this in my other email. *sigh*

With kernel 2.6.32-rc3-00052-g0eca52a I got following KERN_ERR messages just while using firefox:

[  130.527399] NOHZ: local_softirq_pending 08
[  130.559807] NOHZ: local_softirq_pending 08

...

Any idea? or known issue?