From: Peter Zijlstra <peterz@infradead.org> Date: 2014-10-29 18:07:39
On Wed, Oct 29, 2014 at 04:56:20PM +0100, Sabrina Dubroca wrote:
commit e22b886a8a43b ("sched/wait: Add might_sleep() checks") included
in today's linux-next added a check that fires on e1000 with netpoll:
BUG: sleeping function called from invalid context at kernel/irq/manage.c:104
in_atomic(): 1, irqs_disabled(): 1, pid: 1, name: systemd
no locks held by systemd/1.
irq event stamp: 10102965
hardirqs last enabled at (10102965): [<ffffffff810cbafd>] vprintk_emit+0x2dd/0x6a0
hardirqs last disabled at (10102964): [<ffffffff810cb897>] vprintk_emit+0x77/0x6a0
softirqs last enabled at (10102342): [<ffffffff810666aa>] __do_softirq+0x27a/0x6f0
softirqs last disabled at (10102337): [<ffffffff81066e86>] irq_exit+0x56/0xe0
Preemption disabled at:[<ffffffff817de50d>] printk_emit+0x31/0x33
CPU: 1 PID: 1 Comm: systemd Not tainted 3.18.0-rc2-next-20141029-dirty #222
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140617_173321-var-lib-archbuild-testing-x86_64-tobias 04/01/2014
ffffffff81a82291 ffff88001e743978 ffffffff817df31d 0000000000000000
0000000000000000 ffff88001e7439a8 ffffffff8108dfa2 ffff88001e7439a8
ffffffff81a82291 0000000000000068 0000000000000000 ffff88001e7439d8
Call Trace:
[<ffffffff817df31d>] dump_stack+0x4f/0x7c
[<ffffffff8108dfa2>] ___might_sleep+0x182/0x2b0
[<ffffffff8108e10a>] __might_sleep+0x3a/0xc0
[<ffffffff810ce358>] synchronize_irq+0x38/0xa0
[<ffffffff810ce690>] disable_irq+0x20/0x30
[<ffffffff815d7253>] e1000_netpoll+0x23/0x60
[<ffffffff81678d02>] netpoll_poll_dev+0x72/0x3a0
[<ffffffff816791e7>] netpoll_send_skb_on_dev+0x1b7/0x2e0
[<ffffffff816795f3>] netpoll_send_udp+0x2e3/0x490
Oh cute.. not entirely sure what to do there. This only works if you
_know_ desc->threads_active will never be !0.
The best I can come up with is something like this, which avoids the
might_sleep() in the one special case.
Thomas?
---
kernel/irq/manage.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2014-10-29 18:33:33
On Wed, 29 Oct 2014, Peter Zijlstra wrote:
On Wed, Oct 29, 2014 at 04:56:20PM +0100, Sabrina Dubroca wrote:
quoted
commit e22b886a8a43b ("sched/wait: Add might_sleep() checks") included
in today's linux-next added a check that fires on e1000 with netpoll:
BUG: sleeping function called from invalid context at kernel/irq/manage.c:104
in_atomic(): 1, irqs_disabled(): 1, pid: 1, name: systemd
no locks held by systemd/1.
irq event stamp: 10102965
hardirqs last enabled at (10102965): [<ffffffff810cbafd>] vprintk_emit+0x2dd/0x6a0
hardirqs last disabled at (10102964): [<ffffffff810cb897>] vprintk_emit+0x77/0x6a0
softirqs last enabled at (10102342): [<ffffffff810666aa>] __do_softirq+0x27a/0x6f0
softirqs last disabled at (10102337): [<ffffffff81066e86>] irq_exit+0x56/0xe0
Preemption disabled at:[<ffffffff817de50d>] printk_emit+0x31/0x33
CPU: 1 PID: 1 Comm: systemd Not tainted 3.18.0-rc2-next-20141029-dirty #222
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140617_173321-var-lib-archbuild-testing-x86_64-tobias 04/01/2014
ffffffff81a82291 ffff88001e743978 ffffffff817df31d 0000000000000000
0000000000000000 ffff88001e7439a8 ffffffff8108dfa2 ffff88001e7439a8
ffffffff81a82291 0000000000000068 0000000000000000 ffff88001e7439d8
Call Trace:
[<ffffffff817df31d>] dump_stack+0x4f/0x7c
[<ffffffff8108dfa2>] ___might_sleep+0x182/0x2b0
[<ffffffff8108e10a>] __might_sleep+0x3a/0xc0
[<ffffffff810ce358>] synchronize_irq+0x38/0xa0
[<ffffffff810ce690>] disable_irq+0x20/0x30
[<ffffffff815d7253>] e1000_netpoll+0x23/0x60
[<ffffffff81678d02>] netpoll_poll_dev+0x72/0x3a0
[<ffffffff816791e7>] netpoll_send_skb_on_dev+0x1b7/0x2e0
[<ffffffff816795f3>] netpoll_send_udp+0x2e3/0x490
Oh cute.. not entirely sure what to do there. This only works if you
_know_ desc->threads_active will never be !0.
The best I can come up with is something like this, which avoids the
might_sleep() in the one special case.
Thomas?
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.
Thanks,
tglx
From: Peter Zijlstra <peterz@infradead.org> Date: 2014-10-29 19:36:09
On Wed, Oct 29, 2014 at 07:33:00PM +0100, Thomas Gleixner wrote:
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(-)
From: Jeff Kirsher <hidden> Date: 2014-10-29 19:40:50
On Wed, 2014-10-29 at 20:36 +0100, Peter Zijlstra wrote:
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.
Thomas- if you are fine with Peter's patch, I can get this under
testing.
From: Thomas Gleixner <hidden> Date: 2014-10-29 19:54:06
On Wed, 29 Oct 2014, Jeff Kirsher wrote:
On Wed, 2014-10-29 at 20:36 +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.
Thomas- if you are fine with Peter's patch, I can get this under
testing.
I'm fine with it except for the comment part of disable_irq(), but
that does not matter :)
One nitpick: Instead of having the lock unconditionally, I'd make it
depend on CONFIG_NET_POLL_CONTROLLER.
#ifdef CONFIG_NET_POLL_CONTROLLER
static inline void netpoll_lock(struct e1000_adapter *adapter)
{
spin_lock(&adapter->irq_lock);
}
static inline void netpoll_unlock(struct e1000_adapter *adapter)
{
spin_unlock(&adapter->irq_lock);
}
#else
static inline void netpoll_lock(struct e1000_adapter *adapter) { }
static inline void netpoll_unlock(struct e1000_adapter *adapter) { }
#endif
and use that instead of the unconditional spin[un]lock() invocations.
But that's up to you.
Thanks,
tglx
From: Thomas Gleixner <hidden> Date: 2014-10-29 19:49:31
On Wed, 29 Oct 2014, Peter Zijlstra wrote:
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.
We changed that almost 4 years ago :) What we 'just' did was to add a
prominent warning into the code.
From: Peter Zijlstra <peterz@infradead.org> Date: 2014-10-29 19:50:59
On Wed, Oct 29, 2014 at 08:49:03PM +0100, Thomas Gleixner wrote:
On Wed, 29 Oct 2014, 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.
We changed that almost 4 years ago :) What we 'just' did was to add a
prominent warning into the code.
You know that is the same right... they didn't know it was broken
therefore it wasn't :-), but now they need to go actually do stuff about
it, an entirely different proposition.
From: Thomas Gleixner <hidden> Date: 2014-10-29 20:07:41
On Wed, 29 Oct 2014, Peter Zijlstra wrote:
On Wed, Oct 29, 2014 at 08:49:03PM +0100, Thomas Gleixner wrote:
quoted
On Wed, 29 Oct 2014, 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.
We changed that almost 4 years ago :) What we 'just' did was to add a
prominent warning into the code.
You know that is the same right... they didn't know it was broken
therefore it wasn't :-), but now they need to go actually do stuff about
it, an entirely different proposition.
Right, and of course the world and some more has the very same code
there:
poll_controller()
{
disable_irq();
dev_interrupt_handler();
enable_irq();
}
Trying to twist my brain to come up with a solution which avoids the
spinlock, but I have a hard time to come up with one.
The only thing I came up with so far is to avoid adding locks to every
driver incarnation and instead put it into struct net_device and
provide helper functions for the lock/unlock case.
That does not change the fact that we need to deal with that on a per
driver basis :(
Thanks,
tglx
From: Thomas Gleixner <hidden> Date: 2014-10-29 20:24:12
On Wed, 29 Oct 2014, Thomas Gleixner wrote:
On Wed, 29 Oct 2014, Peter Zijlstra wrote:
quoted
On Wed, Oct 29, 2014 at 08:49:03PM +0100, Thomas Gleixner wrote:
quoted
On Wed, 29 Oct 2014, 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.
We changed that almost 4 years ago :) What we 'just' did was to add a
prominent warning into the code.
You know that is the same right... they didn't know it was broken
therefore it wasn't :-), but now they need to go actually do stuff about
it, an entirely different proposition.
Right, and of course the world and some more has the very same code
there:
poll_controller()
{
disable_irq();
dev_interrupt_handler();
enable_irq();
}
Trying to twist my brain to come up with a solution which avoids the
spinlock, but I have a hard time to come up with one.
The only thing I came up with so far is to avoid adding locks to every
driver incarnation and instead put it into struct net_device and
provide helper functions for the lock/unlock case.
That does not change the fact that we need to deal with that on a per
driver basis :(
But at least it allows to mitigate the impact by making it conditional
at a central point.
static inline void netpoll_lock(struct net_device *nd)
{
if (netpoll_active(nd))
spin_lock(&nd->netpoll_lock);
}
and let the core code make sure that activation/deactivation of
netpoll on a particular interface is serialized against the interrupt
and netpoll calls.
Not sure if it's worth the trouble, but at least it allows to deal
with it in the core instead of dealing with it on a per driver base.
Thanks,
tglx
From: Peter Zijlstra <peterz@infradead.org> Date: 2014-10-29 20:51:37
On Wed, Oct 29, 2014 at 09:23:42PM +0100, Thomas Gleixner wrote:
But at least it allows to mitigate the impact by making it conditional
at a central point.
static inline void netpoll_lock(struct net_device *nd)
{
if (netpoll_active(nd))
spin_lock(&nd->netpoll_lock);
}
branch fail vs lock might be a toss on most machines, but if we're
hitting cold cachelines we loose big.
and let the core code make sure that activation/deactivation of
netpoll on a particular interface is serialized against the interrupt
and netpoll calls.
Not sure if it's worth the trouble, but at least it allows to deal
with it in the core instead of dealing with it on a per driver base.
Does multi-queue have one netdev per queue or does that need moar
logicz?
From: Thomas Gleixner <hidden> Date: 2014-10-29 21:03:49
On Wed, 29 Oct 2014, Peter Zijlstra wrote:
On Wed, Oct 29, 2014 at 09:23:42PM +0100, Thomas Gleixner wrote:
quoted
But at least it allows to mitigate the impact by making it conditional
at a central point.
static inline void netpoll_lock(struct net_device *nd)
{
if (netpoll_active(nd))
spin_lock(&nd->netpoll_lock);
}
branch fail vs lock might be a toss on most machines, but if we're
hitting cold cachelines we loose big.
Well, if the net_device is not cache hot on irq entry you have lost
already. The extra branch/lock is not going to add much to that.
Thanks,
tglx
Hello, sorry for the delay.
2014-10-29, 20:36:03 +0100, Peter Zijlstra wrote:
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?
Thanks.
---
From: Bart Van Assche <bvanassche@acm.org> Date: 2014-12-22 16:16:55
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.
Sorry but this patch looks incorrect to me. Since e1000_netpoll() can be
called with interrupts disabled e1000_intr() must not enable interrupts
unconditionally. Shouldn't the save / restore variants be used in
e1000_intr() instead of spin_lock_irq() and spin_unlock_irq() ? See also
the invocation of call_console_drivers() in console_unlock().
Bart.