Re: Re: WARNING: at kernel/softirq.c:159 _local_bh_enable_ip+0x35/0x71()

Sender : Francois Romieu[off-list ref]
Date : 2012-02-21 18:56 (GMT+09:00)
Title : Re: WARNING: at kernel/softirq.c:159 _local_bh_enable_ip+0x35/0x71()

Jongman Heo :
[...]

quoted

Host : windows 7 32bit
VMWare Guest : Fedora 16, with latest linus git kernel (3.3.0-rc4+). 
                        vmxnet3 ethernet driver, running NFS server

This is a 32bit HIGHMEM kernel, right ?

Right, 32bit HIGHMEM is set.

quoted

Following warning triggered with my VMware Linux guest, when NFS connection is requested from outside.

I'd try reverting 39d4a96fd7d2926e46151adbd18b810aeeea8ec0.

-- 
Ueimor

I confirm that reverting the commit fixes the warning.

Regards,
Jongman.

(Heo, your mua broke threading)

Jongman Heo [off-list ref] :

Francois Romieu[off-list ref]

quoted

quoted

Jongman Heo [off-list ref] :
Following warning triggered with my VMware Linux guest, when NFS connection is requested from outside.

I'd try reverting 39d4a96fd7d2926e46151adbd18b810aeeea8ec0.

[...]

I confirm that reverting the commit fixes the warning.

Shreyas, can you provide a proper fix for this bug (see
http://www.spinics.net/lists/netdev/msg189554.html for details).

As far as I understand it you can not claim a fake transport layer header
size for udp and blindly check the available buffer size through
pskb_may_pull later. With a 32 bits HIGHMEM guest config (yuck...) it ends
up enabling bh within a network device start_xmit context.

-- 
Ueimor

Le jeudi 23 février 2012 à 11:42 +0100, Francois Romieu a écrit :

(Heo, your mua broke threading)

Jongman Heo [off-list ref] :

quoted

Francois Romieu[off-list ref]

quoted

quoted

Jongman Heo [off-list ref] :
Following warning triggered with my VMware Linux guest, when NFS connection is requested from outside.

I'd try reverting 39d4a96fd7d2926e46151adbd18b810aeeea8ec0.

[...]

quoted

I confirm that reverting the commit fixes the warning.

Shreyas, can you provide a proper fix for this bug (see
http://www.spinics.net/lists/netdev/msg189554.html for details).

As far as I understand it you can not claim a fake transport layer header
size for udp and blindly check the available buffer size through
pskb_may_pull later. With a 32 bits HIGHMEM guest config (yuck...) it ends
up enabling bh within a network device start_xmit context.

Hmm, I am not sure we still need local_bh_disable()/local_bh_enable() in
kmap_skb_frag()/ kunmap_skb_frag() anymore after commit 3e4d3af501ccc
(mm: stack based kmap_atomic() )

diff --git a/net/core/kmap_skb.h b/net/core/kmap_skb.h
index 81e1ed7..06be5ee 100644
--- a/net/core/kmap_skb.h
+++ b/net/core/kmap_skb.h

@@ -2,18 +2,10 @@
 
 static inline void *kmap_skb_frag(const skb_frag_t *frag)
 {
-#ifdef CONFIG_HIGHMEM
-	BUG_ON(in_irq());
-
-	local_bh_disable();
-#endif
 	return kmap_atomic(skb_frag_page(frag), KM_SKB_DATA_SOFTIRQ);
 }
 
 static inline void kunmap_skb_frag(void *vaddr)
 {
 	kunmap_atomic(vaddr, KM_SKB_DATA_SOFTIRQ);
-#ifdef CONFIG_HIGHMEM
-	local_bh_enable();
-#endif
 }

On Thu, 2012-02-23 at 12:53 +0100, Eric Dumazet wrote:

Hmm, I am not sure we still need local_bh_disable()/local_bh_enable() in
kmap_skb_frag()/ kunmap_skb_frag() anymore after commit 3e4d3af501ccc
(mm: stack based kmap_atomic() )

The only thing to consider is keeping the total stack size under
control, this is somewhat non-trivial since its non-obvious what all
nests.

That said, you're probably right, and we do have a WARN in there
(dependent on CONFIG_DEBUG_HIGHMEM) that yells if we exceed the
available stack size.

The more 'interesting' exercise is determining a better upper bound on
the stack size and updating kmap_types.h accordingly.

The nicer patch would of course be a patch that does:

 s/kunmap_skb_frag/kunmap_atomic/
 s/kmap_skb_frag(\([^)]*\))/kmap_atomic(skb_frag_page(\1))/

There is no need to retain the KM_* argument and the 'helper' functions
are quite pointless at that point.

Le jeudi 23 février 2012 à 14:22 +0100, Peter Zijlstra a écrit :

On Thu, 2012-02-23 at 12:53 +0100, Eric Dumazet wrote:

quoted

Hmm, I am not sure we still need local_bh_disable()/local_bh_enable() in
kmap_skb_frag()/ kunmap_skb_frag() anymore after commit 3e4d3af501ccc
(mm: stack based kmap_atomic() )

The only thing to consider is keeping the total stack size under
control, this is somewhat non-trivial since its non-obvious what all
nests.

That said, you're probably right, and we do have a WARN in there
(dependent on CONFIG_DEBUG_HIGHMEM) that yells if we exceed the
available stack size.

The more 'interesting' exercise is determining a better upper bound on
the stack size and updating kmap_types.h accordingly.

I would say its the same logic than crypto : We might use at most two
contexts for SKB frags : USER or SOFTIRQ

We probably can remove KM_SKB_DATA_SOFTIRQ slot and use fact that this
kmap user can reuse existing USER/SOFTIRQ slots

quoted

diff --git a/net/core/kmap_skb.h b/net/core/kmap_skb.h
index 81e1ed7..06be5ee 100644
--- a/net/core/kmap_skb.h
+++ b/net/core/kmap_skb.h

@@ -2,18 +2,10 @@
 
 static inline void *kmap_skb_frag(const skb_frag_t *frag)
 {
-#ifdef CONFIG_HIGHMEM
-       BUG_ON(in_irq());
-
-       local_bh_disable();
-#endif
        return kmap_atomic(skb_frag_page(frag), KM_SKB_DATA_SOFTIRQ);
 }
 
 static inline void kunmap_skb_frag(void *vaddr)
 {
        kunmap_atomic(vaddr, KM_SKB_DATA_SOFTIRQ);
-#ifdef CONFIG_HIGHMEM
-       local_bh_enable();
-#endif
 } 

The nicer patch would of course be a patch that does:

 s/kunmap_skb_frag/kunmap_atomic/
 s/kmap_skb_frag(\([^)]*\))/kmap_atomic(skb_frag_page(\1))/

There is no need to retain the KM_* argument and the 'helper' functions
are quite pointless at that point.

Well, definitely a cleanup is possible, but probably not suitable for
3.3 and stable kernels ?

On Thu, 2012-02-23 at 15:21 +0100, Eric Dumazet wrote:

Well, definitely a cleanup is possible, but probably not suitable for
3.3 and stable kernels ? 

Nah, that'd be better suited for a development branch like :-)

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Thu, 23 Feb 2012 11:42:37 +0100

As far as I understand it you can not claim a fake transport layer header
size for udp and blindly check the available buffer size through
pskb_may_pull later. With a 32 bits HIGHMEM guest config (yuck...) it ends
up enabling bh within a network device start_xmit context.

Right, lying about the transport header size is going to give you nothing
but trouble, you absolutely cannot do this or networking breaks, and your
transmit handler doesn't run in a context where you can legally "fix"
things up.

We recently fixed even IPoIB in this regard.

----- Original Message -----

(Heo, your mua broke threading)

Jongman Heo [off-list ref] :

quoted

Francois Romieu[off-list ref]

quoted

quoted

Jongman Heo [off-list ref] :
Following warning triggered with my VMware Linux guest, when
NFS connection is requested from outside.

I'd try reverting 39d4a96fd7d2926e46151adbd18b810aeeea8ec0.

[...]

quoted

I confirm that reverting the commit fixes the warning.

Shreyas, can you provide a proper fix for this bug (see
http://www.spinics.net/lists/netdev/msg189554.html for details).

As far as I understand it you can not claim a fake transport layer
header
size for udp and blindly check the available buffer size through
pskb_may_pull later. With a 32 bits HIGHMEM guest config (yuck...) it
ends
up enabling bh within a network device start_xmit context.

--
Ueimor

Just sent a patch to fix the transport header size.

Thanks.
Shreyas