Re: e1000_netpoll(): disable_irq() triggers might_sleep() on linux-next

On 12/02/14 17:35, Sabrina Dubroca wrote:

Hello, sorry for the delay.

2014-10-29, 20:36:03 +0100, Peter Zijlstra wrote:

quoted

On Wed, Oct 29, 2014 at 07:33:00PM +0100, Thomas Gleixner wrote:

quoted

Yuck. No. You are just papering over the problem.

What happens if you add 'threadirqs' to the kernel command line? Or if
the interrupt line is shared with a real threaded interrupt user?

The proper solution is to have a poll_lock for e1000 which serializes
the hardware interrupt against netpoll instead of using
disable/enable_irq().

In fact that's less expensive than the disable/enable_irq() dance and
the chance of contention is pretty low. If done right it will be a
NOOP for the CONFIG_NET_POLL_CONTROLLER=n case.

OK a little something like so then I suppose.. But I suspect most all
the network drivers will need this and maybe more, disable_irq() is a
popular little thing and we 'just' changed semantics on them.

---
 drivers/net/ethernet/intel/e1000/e1000.h      |  2 ++
 drivers/net/ethernet/intel/e1000/e1000_main.c | 22 +++++++++++++++++-----
 kernel/irq/manage.c                           |  2 +-
 3 files changed, 20 insertions(+), 6 deletions(-)

I've been running with variants of this patch, things seem ok.

As noted earlier, there are a lot of drivers doing this disable_irq +
irq_handler + enable_irq sequence.  I found about 60.
Many already take a lock in the interrupt handler, and look like we
could just remove the call to disable_irq (example: cp_interrupt,
drivers/net/ethernet/realtek/8139cp.c).

Here's how I modified your patch.  The locking compiles away if
CONFIG_NET_POLL_CONTROLLER=n.

I can work on converting all the drivers from disable_irq to
netpoll_irq_lock, if that's okay with you.

In igb there's also a synchronize_irq() called from the netpoll
controller (in igb_irq_disable()), I think a similar locking scheme
would work.
I also saw a few disable_irq_nosync and disable_percpu_irq. These are
okay?

[ ... ]

Hello,

Earlier today I ran into the bug mentioned at the start of this thread
with kernel 3.19-rc1 and the e1000e driver. Can anyone tell me what the
latest status is ?

Thanks,

Bart.