[PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[David Mackey]

Using dev_kfree_skb_any() will resolve the below issue when a
netconsole message is transmitted in an irq.

 ------------[ cut here ]------------
 WARNING: at net/core/skbuff.c:451 skb_release_head_state+0x7b/0xe1()
 ...
 Pid: 0, comm: swapper/2 Not tainted 3.4.55 #1
 Call Trace:
  <IRQ>  [<ffffffff8104934c>] warn_slowpath_common+0x85/0x9d
  [<ffffffff8104937e>] warn_slowpath_null+0x1a/0x1c
  [<ffffffff81429aa7>] skb_release_head_state+0x7b/0xe1
  [<ffffffff814297e1>] __kfree_skb+0x16/0x81
  [<ffffffff814298a0>] consume_skb+0x54/0x69
  [<ffffffffa015925b>] bnx2_tx_int.clone.6+0x1b0/0x33e [bnx2]
  [<ffffffff8129c54d>] ? unmask_msi_irq+0x10/0x12
  [<ffffffffa015aa06>] bnx2_poll_work+0x3a/0x73 [bnx2]
  [<ffffffffa015aa73>] bnx2_poll_msix+0x34/0xb4 [bnx2]
  [<ffffffff814466a2>] netpoll_poll_dev+0xb9/0x1b7
  [<ffffffff814467d7>] ? find_skb+0x37/0x82
  [<ffffffff814461ed>] netpoll_send_skb_on_dev+0x117/0x200
  [<ffffffff81446a52>] netpoll_send_udp+0x230/0x242
  [<ffffffffa0174296>] write_msg+0xa7/0xfb [netconsole]
  [<ffffffff814258a4>] ? sk_free+0x1c/0x1e
  [<ffffffff810495ad>] __call_console_drivers+0x7d/0x8f
  [<ffffffff81049674>] _call_console_drivers+0xb5/0xd0
  [<ffffffff8104a134>] console_unlock+0x131/0x219
  [<ffffffff8104a7f9>] vprintk+0x3bc/0x405
  [<ffffffff81460073>] ? NF_HOOK.clone.1+0x4c/0x53
  [<ffffffff81460308>] ? ip_rcv+0x23c/0x268
  [<ffffffff814ddd4f>] printk+0x68/0x71
  [<ffffffff813315b3>] __dev_printk+0x78/0x7a
  [<ffffffff813316b2>] dev_warn+0x53/0x55
  [<ffffffff8127f181>] ? swiotlb_unmap_sg_attrs+0x47/0x5c
  [<ffffffffa004f876>] complete_scsi_command+0x28a/0x4a0 [hpsa]
  [<ffffffffa004fadb>] finish_cmd+0x4f/0x66 [hpsa]
  [<ffffffffa004fd97>] process_indexed_cmd+0x48/0x54 [hpsa]
  [<ffffffffa004ff25>] do_hpsa_intr_msi+0x4e/0x77 [hpsa]
  [<ffffffff810baebb>] handle_irq_event_percpu+0x5e/0x1b6
  [<ffffffff81088a0b>] ? timekeeping_update+0x43/0x45
  [<ffffffff810bb04b>] handle_irq_event+0x38/0x54
  [<ffffffff8102bd1e>] ? ack_apic_edge+0x36/0x3a
  [<ffffffff810bd762>] handle_edge_irq+0xa5/0xc8
  [<ffffffff81010d56>] handle_irq+0x127/0x135
  [<ffffffff814e3426>] ? __atomic_notifier_call_chain+0x12/0x14
  [<ffffffff814e343c>] ? atomic_notifier_call_chain+0x14/0x16
  [<ffffffff814e897d>] do_IRQ+0x4d/0xb4
  [<ffffffff814dffea>] common_interrupt+0x6a/0x6a
  <EOI>  [<ffffffff812b7603>] ? intel_idle+0xd8/0x112
  [<ffffffff812b7603>] ? intel_idle+0xd8/0x112
  [<ffffffff812b75e9>] ? intel_idle+0xbe/0x112
  [<ffffffff814012fc>] cpuidle_enter+0x12/0x14
  [<ffffffff814019c2>] cpuidle_idle_call+0xd1/0x19b
  [<ffffffff81016551>] cpu_idle+0xb6/0xff
  [<ffffffff814d726b>] start_secondary+0xc8/0xca
 ---[ end trace 3f15cd66441c770d ]---

Signed-off-by: David Mackey <tdmackey@booleanhaiku.com>
---
 drivers/net/ethernet/broadcom/bnx2.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index e838a3f..372cbb5 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -2868,7 +2868,7 @@ bnx2_tx_int(struct bnx2 *bp, struct bnx2_napi *bnapi, int budget)
 		sw_cons = BNX2_NEXT_TX_BD(sw_cons);
 
 		tx_bytes += skb->len;
-		dev_kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		tx_pkt++;
 		if (tx_pkt == budget)
 			break;
-- 
1.7.9.5

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[David Miller]

From: David Mackey <tdmackey@booleanhaiku.com>
Date: Tue, 29 Oct 2013 15:16:38 -0700

> Using dev_kfree_skb_any() will resolve the below issue when a
> netconsole message is transmitted in an irq.
 ...
> Signed-off-by: David Mackey <tdmackey@booleanhaiku.com>

This is absolutely not the correct fix.

The netpoll facility must invoke ->poll() in an environment which
is compatible, locking and interrupt/soft-interrupt wise, as that
in which it is normally called.

Therefore, bnx2_tx_int(), which is invoked from the driver's ->poll()
method, should not need to use dev_kfree_skb_any().  The real problem
is somewhere else.

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[Cong Wang]

On Tue, Oct 29, 2013 at 7:42 PM, David Miller <davem@davemloft.net> wrote:
> From: David Mackey <tdmackey@booleanhaiku.com>
> Date: Tue, 29 Oct 2013 15:16:38 -0700
>
>> Using dev_kfree_skb_any() will resolve the below issue when a
>> netconsole message is transmitted in an irq.
>  ...
>> Signed-off-by: David Mackey <tdmackey@booleanhaiku.com>
>
> This is absolutely not the correct fix.
>
> The netpoll facility must invoke ->poll() in an environment which
> is compatible, locking and interrupt/soft-interrupt wise, as that
> in which it is normally called.
>

Normally ->poll() is called in softirq context, while netpoll could
be called in any context depending on its caller.

Also, netpoll disables IRQ for tx, which is another difference.

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 29 Oct 2013 20:50:08 -0700

> Normally ->poll() is called in softirq context, while netpoll could
> be called in any context depending on its caller.

It still makes amends to make the execution context still looks
"compatible" as far as locking et al. is concerned.

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[Cong Wang]

On Tue, Oct 29, 2013 at 11:40 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Tue, 29 Oct 2013 20:50:08 -0700
>
>> Normally ->poll() is called in softirq context, while netpoll could
>> be called in any context depending on its caller.
>
> It still makes amends to make the execution context still looks
> "compatible" as far as locking et al. is concerned.

Adjusting netpoll code for IRQ context is much harder
than just calling dev_kfree_skb_any()...

What's more, we have similar change before:

commit ed79bab847d8e5a2986d8ff43c49c6fb8ee3265f
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Wed Oct 14 14:36:43 2009 +0000

    virtio_net: use dev_kfree_skb_any() in free_old_xmit_skbs()

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 30 Oct 2013 12:23:52 -0700

> On Tue, Oct 29, 2013 at 11:40 PM, David Miller <davem@davemloft.net> wrote:
>> From: Cong Wang <xiyou.wangcong@gmail.com>
>> Date: Tue, 29 Oct 2013 20:50:08 -0700
>>
>>> Normally ->poll() is called in softirq context, while netpoll could
>>> be called in any context depending on its caller.
>>
>> It still makes amends to make the execution context still looks
>> "compatible" as far as locking et al. is concerned.
> 
> Adjusting netpoll code for IRQ context is much harder
> than just calling dev_kfree_skb_any()...
> 
> What's more, we have similar change before:
> 
> commit ed79bab847d8e5a2986d8ff43c49c6fb8ee3265f
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Wed Oct 14 14:36:43 2009 +0000
> 
>     virtio_net: use dev_kfree_skb_any() in free_old_xmit_skbs()

Explain to me then why other ethernet drivers implemented identically,
such as tg3, can use plain dev_kfree_skb() just fine?

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[Cong Wang]

On Wed, Oct 30, 2013 at 2:32 PM, David Miller <davem@davemloft.net> wrote:
>
> Explain to me then why other ethernet drivers implemented identically,
> such as tg3, can use plain dev_kfree_skb() just fine?

I don't think they are fine, I just don't see bug reports
for them. At very least, I saw a same bug report for be2net too.

To reproduce this bug, we need to find some one calling printk()
within IRQ handler, which seems rare? It seems there are few
people using hpsa driver together with netconsole.

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 30 Oct 2013 15:01:12 -0700

> On Wed, Oct 30, 2013 at 2:32 PM, David Miller <davem@davemloft.net> wrote:
>>
>> Explain to me then why other ethernet drivers implemented identically,
>> such as tg3, can use plain dev_kfree_skb() just fine?
> 
> I don't think they are fine, I just don't see bug reports
> for them. At very least, I saw a same bug report for be2net too.
> 
> To reproduce this bug, we need to find some one calling printk()
> within IRQ handler, which seems rare? It seems there are few
> people using hpsa driver together with netconsole.

We've had this conversation before I believe, and I absolutely do not
want to have to add IRQ safety to every ->poll() implementation.

We have to provide a softint compatible environment for this callback
to run in else everything is completely broken.

All these drivers can safely assume softirq safe locking is
sufficient, you're suggesting we need to take this hardirq safety and
I'm really not willing to allow things to go that far.  A lot of
effort has been expended precisely to avoid that kind of overhead and
cost.

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 31 Oct 2013 21:19:16 -0700

> 2013年10月30日 下午9:26于 "David Miller" <davem@davemloft.net>写道：
>>
>> We have to provide a softint compatible environment for this callback
>> to run in else everything is completely broken.
>>
>> All these drivers can safely assume softirq safe locking is
>> sufficient, you're suggesting we need to take this hardirq safety and
>> I'm really not willing to allow things to go that far.  A lot of
>> effort has been expended precisely to avoid that kind of overhead and
>> cost.
> 
> Alright, I am thinking to move netpoll_poll_dev() to a delayed work.
 
What if the printk is outputting a message that will help us discover
that work queues are deadlocked?

You can't delay the message, because every layer of indirection you
add increases the possibility that the message it never seen.  You
have to do it synchronously.

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[Ben Hutchings]

On Fri, 2013-11-01 at 18:01 -0400, David Miller wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Thu, 31 Oct 2013 21:19:16 -0700
> 
> > 2013年10月30日 下午9:26于 "David Miller" <davem@davemloft.net>写道：
> >>
> >> We have to provide a softint compatible environment for this callback
> >> to run in else everything is completely broken.
> >>
> >> All these drivers can safely assume softirq safe locking is
> >> sufficient, you're suggesting we need to take this hardirq safety and
> >> I'm really not willing to allow things to go that far.  A lot of
> >> effort has been expended precisely to avoid that kind of overhead and
> >> cost.
> > 
> > Alright, I am thinking to move netpoll_poll_dev() to a delayed work.
>  
> What if the printk is outputting a message that will help us discover
> that work queues are deadlocked?
> 
> You can't delay the message, because every layer of indirection you
> add increases the possibility that the message it never seen.  You
> have to do it synchronously.

As you've said, the ndo_start_xmit and NAPI poll operations are intended
to be called in softirq context, so everything that interlocks with them
will use spin_lock_bh().  Calling them from hardirq context obviously
opens the possibility of a deadlock.  How do you expect anyone to solve
that?

I think that most of the time netpoll doesn't actually call the NAPI
poll function, and the driver ndo_start_xmit function doesn't take any
locks, so we don't actually hit the deadlock in practice (on mainline
kernels - RT is a different story).

Obviously, the less machinery netpoll relies on continuing to work, the
better, so it should preferably defer to sofirq context rather than
workqueue context.  I think this means hooking queue_process() into
net_tx_action(), and then cutting out much of the rest of netpoll.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] bnx2: Use dev_kfree_skb_any() in bnx2_tx_int()

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 1 Nov 2013 23:34:50 +0000

> As you've said, the ndo_start_xmit and NAPI poll operations are intended
> to be called in softirq context, so everything that interlocks with them
> will use spin_lock_bh().  Calling them from hardirq context obviously
> opens the possibility of a deadlock.  How do you expect anyone to solve
> that?

That's not what I said.

I did not say that it must be invoked in softirq context.

I said that it MUST LOOK like it is being invoked in softirq
context as far as the ->poll() code paths can tell.

But yes, that's hard.

The thing is, ->poll() is atomic.  You never will have poll calls
recurse into eachother.  This is why we strongly encourage all driver
authors to make their ->poll() implementations lockless.

And, wouldn't you know it, tg3 is a driver that does this
properly.

Therefore, there are no netpoll no locking problems, because the poll
implementation takes no locks and therefore doesn't care.

That's why tg3 doesn't have any of these netpoll issues.