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

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

[Jongman Heo]

> Sender : Francois Romieu<romieu@fr.zoreil.com>
> 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 :
> [...]
> > 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.

> 
> > 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.

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

[Francois Romieu]

(Heo, your mua broke threading)

Jongman Heo <jongman.heo@samsung.com> :
> Francois Romieu<romieu@fr.zoreil.com>
> > > Jongman Heo <jongman.heo@samsung.com> :
> > > 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

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

[Eric Dumazet]

Le jeudi 23 février 2012 à 11:42 +0100, Francois Romieu a écrit :
> (Heo, your mua broke threading)
> 
> Jongman Heo <jongman.heo@samsung.com> :
> > Francois Romieu<romieu@fr.zoreil.com>
> > > > Jongman Heo <jongman.heo@samsung.com> :
> > > > 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.
> 

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
 }

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

[Peter Zijlstra]

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.

> 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.

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

[Eric Dumazet]

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:
> > 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

> > 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 ?

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

[Peter Zijlstra]

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 :-)

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

[David Miller]

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.

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

[Shreyas Bhatewara]

----- Original Message -----
> (Heo, your mua broke threading)
> 
> Jongman Heo <jongman.heo@samsung.com> :
> > Francois Romieu<romieu@fr.zoreil.com>
> > > > Jongman Heo <jongman.heo@samsung.com> :
> > > > 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
> 

Just sent a patch to fix the transport header size.

Thanks.
Shreyas