From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2014-03-04 23:49:56
On PREEMPT_RT enabled systems the interrupt handler run as threads at prio 50
(by default). If a high priority userspace process tries to shut down a busy
network interface it might spin in a yield loop waiting for the device to
become idle. With the interrupt thread having a lower priority than the
looping process it might never be scheduled and so result in a deadlock on UP
systems.
With Magic SysRq the following backtrace can be produced:
test_app R running 0 174 168 0x00000000
[<c02c7070>] (__schedule+0x220/0x3fc) from [<c02c7870>] (preempt_schedule_irq+0x48/0x80)
[<c02c7870>] (preempt_schedule_irq+0x48/0x80) from [<c0008fa8>] (svc_preempt+0x8/0x20)
[<c0008fa8>] (svc_preempt+0x8/0x20) from [<c001a984>] (local_bh_enable+0x18/0x88)
[<c001a984>] (local_bh_enable+0x18/0x88) from [<c025316c>] (dev_deactivate_many+0x220/0x264)
[<c025316c>] (dev_deactivate_many+0x220/0x264) from [<c023be04>] (__dev_close_many+0x64/0xd4)
[<c023be04>] (__dev_close_many+0x64/0xd4) from [<c023be9c>] (__dev_close+0x28/0x3c)
[<c023be9c>] (__dev_close+0x28/0x3c) from [<c023f7f0>] (__dev_change_flags+0x88/0x130)
[<c023f7f0>] (__dev_change_flags+0x88/0x130) from [<c023f904>] (dev_change_flags+0x10/0x48)
[<c023f904>] (dev_change_flags+0x10/0x48) from [<c024c140>] (do_setlink+0x370/0x7ec)
[<c024c140>] (do_setlink+0x370/0x7ec) from [<c024d2f0>] (rtnl_newlink+0x2b4/0x450)
[<c024d2f0>] (rtnl_newlink+0x2b4/0x450) from [<c024cfa0>] (rtnetlink_rcv_msg+0x158/0x1f4)
[<c024cfa0>] (rtnetlink_rcv_msg+0x158/0x1f4) from [<c0256740>] (netlink_rcv_skb+0xac/0xc0)
[<c0256740>] (netlink_rcv_skb+0xac/0xc0) from [<c024bbd8>] (rtnetlink_rcv+0x18/0x24)
[<c024bbd8>] (rtnetlink_rcv+0x18/0x24) from [<c02561b8>] (netlink_unicast+0x13c/0x198)
[<c02561b8>] (netlink_unicast+0x13c/0x198) from [<c025651c>] (netlink_sendmsg+0x264/0x2e0)
[<c025651c>] (netlink_sendmsg+0x264/0x2e0) from [<c022af98>] (sock_sendmsg+0x78/0x98)
[<c022af98>] (sock_sendmsg+0x78/0x98) from [<c022bb50>] (___sys_sendmsg.part.25+0x268/0x278)
[<c022bb50>] (___sys_sendmsg.part.25+0x268/0x278) from [<c022cf08>] (__sys_sendmsg+0x48/0x78)
[<c022cf08>] (__sys_sendmsg+0x48/0x78) from [<c0009320>] (ret_fast_syscall+0x0/0x2c)
This patch works around the problem by replacing yield() by msleep(1), giving
the interrupt thread time to finish, similar to other changes contained in the
rt patch set. Using wait_for_completion() instead would probably be a better
solution.
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Hello,
this patch is intended for -rt, as the problem probably doesn't occur on non rt
systems. However, I put netdev on Cc, as they probably can come up with a
proper solution.
regards,
Marc
net/sched/sch_generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I don't understand this.
yield() should really _mean_ yield.
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Therefore it should allow lower priority threads to run, not just
equal or higher priority ones.
I don't understand this.
yield() should really _mean_ yield.
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Therefore it should allow lower priority threads to run, not just
equal or higher priority ones.
* Typical broken usage is:
*
* while (!event)
* yield();
*
* where one assumes that yield() will let 'the other' process run that will
* make event true. If the current task is a SCHED_FIFO task that will never
* happen. Never use yield() as a progress guarantee!!
My Process runs with SCHED_FIFO and prio > 50, with IRQ at default prio,
which is 50.
Maybe the RT guys can comment on this. I found another interesting
function in the RT patch set: cpu_chill().
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
I don't understand this.
yield() should really _mean_ yield.
It does, but yield() semantics make it useless for what you want to do..
and pretty much undefined for anything other than SCHED_FIFO. If you
really want to give up the CPU to any old body, you have to sleep.
-Mike
* Typical broken usage is:
*
* while (!event)
* yield();
*
* where one assumes that yield() will let 'the other' process run that will
* make event true. If the current task is a SCHED_FIFO task that will never
* happen. Never use yield() as a progress guarantee!!
My Process runs with SCHED_FIFO and prio > 50, with IRQ at default prio,
which is 50.
Maybe the RT guys can comment on this. I found another interesting
function in the RT patch set: cpu_chill().
If you boot mainline without -RT, use threadirqs, start your application
do the same prio thing then you should end up with exactly the same
outcome. Please say so :)
msleep() is safe as long as it is used outside of the softirq. Nice that
you found cpu_chill() but on non-RT it turns to cpu_relax() and you do
not want this here.
wait_event() would be nice in the end to have. For now I take that patch
for -RT.
I don't understand this.
yield() should really _mean_ yield.
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Therefore it should allow lower priority threads to run, not just
equal or higher priority ones.
Until when?
yield() is not a sensible operation in a preemptive multitasking system,
regardless of RT.
Ben.
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
I don't understand this.
yield() should really _mean_ yield.
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Therefore it should allow lower priority threads to run, not just
equal or higher priority ones.
Until when?
yield() is not a sensible operation in a preemptive multitasking system,
regardless of RT.
To me it means "I've got nothing to do if other tasks want to run right
now" Yes, I even see it having this meaning when an RT task executes
it.
How else can you interpret the intent above?
If you change it to msleep(1), you're assigning an extra completely
arbitrary time limit to the yield. The code doesn't want to sleep
for 1ms, that's not what it's asking for.
On the other hand, I do completely agree with other replies stating
that it would be better if we found a way for this code to wait on
something explicitly via a wait queue.
Unfortunately, that's undesirable from another perspective, in that it
would probably require making the fast paths of the qdiscs more
expensive.
I don't understand this.
yield() should really _mean_ yield.
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Therefore it should allow lower priority threads to run, not just
equal or higher priority ones.
Until when?
yield() is not a sensible operation in a preemptive multitasking system,
regardless of RT.
To me it means "I've got nothing to do if other tasks want to run right
now" Yes, I even see it having this meaning when an RT task executes
it.
How else can you interpret the intent above?
The problem is that 'I've got nothing to do ... now' is information
about a *point* in time, which gives the scheduler no clue as to when
this task might be ready again. So far as task *state* goes, it never
ceases to be ready.
If you change it to msleep(1), you're assigning an extra completely
arbitrary time limit to the yield. The code doesn't want to sleep
for 1ms, that's not what it's asking for.
[...]
I think you want to give up a 'time slice' to any task that's available.
But that's not a meaningful concept for all schedulers. If your task is
highest priority and is ready, it must run. You could drop priority
temporarily, but then you need it to somehow be bumped up again at some
time in the future. Well, sleeping effectively does that.
I do understand that unconditionally sleeping also isn't ideal - the
task that unblocks this one might already be running on another CPU and
able to finish sooner than the sleep timeout.
Maybe the answer is a yield_for(time) which sleeps for the given time or
until there's an idle CPU, whichever is sooner. But I don't know how
hard that would be to implement or how widely useful it would be.
Ben.
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
I don't understand this.
yield() should really _mean_ yield.
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Therefore it should allow lower priority threads to run, not just
equal or higher priority ones.
Until when?
yield() is not a sensible operation in a preemptive multitasking system,
regardless of RT.
To me it means "I've got nothing to do if other tasks want to run right
now" Yes, I even see it having this meaning when an RT task executes
it.
How else can you interpret the intent above?
The problem is that 'I've got nothing to do ... now' is information
about a *point* in time, which gives the scheduler no clue as to when
this task might be ready again. So far as task *state* goes, it never
ceases to be ready.
quoted
If you change it to msleep(1), you're assigning an extra completely
arbitrary time limit to the yield. The code doesn't want to sleep
for 1ms, that's not what it's asking for.
[...]
I think you want to give up a 'time slice' to any task that's available.
But that's not a meaningful concept for all schedulers. If your task is
highest priority and is ready, it must run. You could drop priority
temporarily, but then you need it to somehow be bumped up again at some
time in the future. Well, sleeping effectively does that.
I do understand that unconditionally sleeping also isn't ideal - the
task that unblocks this one might already be running on another CPU and
able to finish sooner than the sleep timeout.
Maybe the answer is a yield_for(time) which sleeps for the given time or
until there's an idle CPU, whichever is sooner. But I don't know how
hard that would be to implement or how widely useful it would be.
what is msleep(0) defined to do? would it be any better?
David Lang
IMNSHO there is no way to make the yield() honor this intent if it is
called from a SCHED_FIFO or SCHED_RR task and the task unblocking
it is running at a lower priority. On the PREEMPT_RT systems where
the interrupt handlers are threads an application thread running
at higher priority than some interrupt handlers is a common situation.
The semantics of the scheduler here is "run the highest priority
runnable task until it blocks (FIFO) or its time slice is over
and there are more with the _same_ priority (RR)". This scenario
then collapses into a busy loop never making progress.
If you change it to msleep(1), you're assigning an extra completely
arbitrary time limit to the yield. The code doesn't want to sleep
for 1ms, that's not what it's asking for.
The problem is that there is no way to formulate what it is asking
for in scheduler terms only.
Regards
--
Stano
I don't understand this.
yield() should really _mean_ yield.
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Therefore it should allow lower priority threads to run, not just
equal or higher priority ones.
Until when?
yield() is not a sensible operation in a preemptive multitasking system,
regardless of RT.
To me it means "I've got nothing to do if other tasks want to run right
now" Yes, I even see it having this meaning when an RT task executes
it.
How else can you interpret the intent above?
The problem is that the semantics of yield() are a complete disaster.
yield() only works by some definiton of "works" when the involved
parties are in the same scheduling class.
You can yield from one SCHED_OTHER task to another SCHED_OTHER task,
but yield() does not guarantee any progress. It might work for some
scenarios of yield from one SCHED_FIFO task to another SCHED_FIFO task
even better than in the SCHED_OTHER case, but there is no guarantee
that it works at all.
But yielding across scheduling classes cannot work ever. Not even in
mainline.
That's not a RT sepcific issue, it's just one of these problems which
are unearthed by RT because it does not follow the bog standard
scheduling class and priority assignements of your favourite distro
setup.
Once you tune a few knobs on a mainline kernel, you can provoke the
same issue. It's that simple.
yield() should die both in kernel and user space.
Thanks,
tglx
From: Peter Zijlstra <peterz@infradead.org> Date: 2014-04-02 11:08:01
On Mon, Mar 31, 2014 at 11:49:16PM +0200, Thomas Gleixner wrote:
quoted
quoted
quoted
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Yeah; like Thomas said; that's not what yield() does -- or can do.
yield() only says 'I don't know what to do, you figure it out', but then
fails to provide enough information to actually do something sensible.
It cannot put the task to sleep; because it doesn't pair with a wakeup;
and therefore the task stays an eligible/runnable task from a scheduler
pov.
The scheduler is therefore entirely in its right to pick this task
again; and pretty much has to under many circumstances.
yield() does not, and can not, guarantee forward progress - ever.
Use wait_event() and assorted bits to wait for an actual event. That is
a sleep paired with a wakeup and thus has progress guarantees.
Also; see the comment near yield:
/**
* yield - yield the current processor to other threads.
*
* Do not ever use this function, there's a 99% chance you're doing it wrong.
*
* The scheduler is at all times free to pick the calling task as the most
* eligible task to run, if removing the yield() call from your code breaks
* it, its already broken.
*
* Typical broken usage is:
*
* while (!event)
* yield();
*
* where one assumes that yield() will let 'the other' process run that will
* make event true. If the current task is a SCHED_FIFO task that will never
* happen. Never use yield() as a progress guarantee!!
*
* If you want to use yield() to wait for something, use wait_event().
* If you want to use yield() to be 'nice' for others, use cond_resched().
* If you still want to use yield(), do not!
*/
void __sched yield(void)
From: Eric Dumazet <hidden> Date: 2014-04-02 11:17:44
On Wed, 2014-04-02 at 13:07 +0200, Peter Zijlstra wrote:
On Mon, Mar 31, 2014 at 11:49:16PM +0200, Thomas Gleixner wrote:
quoted
quoted
quoted
quoted
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Yeah; like Thomas said; that's not what yield() does -- or can do.
yield() only says 'I don't know what to do, you figure it out', but then
fails to provide enough information to actually do something sensible.
It cannot put the task to sleep; because it doesn't pair with a wakeup;
and therefore the task stays an eligible/runnable task from a scheduler
pov.
The scheduler is therefore entirely in its right to pick this task
again; and pretty much has to under many circumstances.
yield() does not, and can not, guarantee forward progress - ever.
Use wait_event() and assorted bits to wait for an actual event. That is
a sleep paired with a wakeup and thus has progress guarantees.
Why wouldn't it be safe to redefine yield as msleep(1) ?
net/ipv4/tcp_output.c seems to also use yield().
/* Socket is locked, keep trying until memory is available. */
for (;;) {
skb = alloc_skb_fclone(MAX_TCP_HEADER,
sk->sk_allocation);
if (skb)
break;
yield();
}
From: Peter Zijlstra <peterz@infradead.org> Date: 2014-04-04 15:19:54
On Wed, Apr 02, 2014 at 04:17:39AM -0700, Eric Dumazet wrote:
On Wed, 2014-04-02 at 13:07 +0200, Peter Zijlstra wrote:
quoted
On Mon, Mar 31, 2014 at 11:49:16PM +0200, Thomas Gleixner wrote:
quoted
quoted
quoted
quoted
The intent of a yield() call, like this one here, is unambiguously
that the current thread cannot do anything until some other thread
gets onto the cpu and makes forward progress.
Yeah; like Thomas said; that's not what yield() does -- or can do.
yield() only says 'I don't know what to do, you figure it out', but then
fails to provide enough information to actually do something sensible.
It cannot put the task to sleep; because it doesn't pair with a wakeup;
and therefore the task stays an eligible/runnable task from a scheduler
pov.
The scheduler is therefore entirely in its right to pick this task
again; and pretty much has to under many circumstances.
yield() does not, and can not, guarantee forward progress - ever.
Use wait_event() and assorted bits to wait for an actual event. That is
a sleep paired with a wakeup and thus has progress guarantees.
Why wouldn't it be safe to redefine yield as msleep(1) ?
Because it violates what POSIX says what yield() should do. There is a
semi valid use-case for yield between RR/FIFO tasks of the same
priority. Changing yield to do msleep(1) would break that.
The kernel actually used to rely on that particular semantics; but I
think we took that out because it was fragile as heck.
net/ipv4/tcp_output.c seems to also use yield().
/* Socket is locked, keep trying until memory is available. */
for (;;) {
skb = alloc_skb_fclone(MAX_TCP_HEADER,
sk->sk_allocation);
if (skb)
break;
yield();
}
Yeah, that's crap code too.
Furthermore, changing the network code to use msleep(1) instead of
yield() doesn't make it less crap than it was, just functional crap
instead of broken crap.
The proper way to fix the dev_deactivate_many() is to use wait_event(),
polling for that state is just daft. Afaict there is no reason the qdisc
code could not do a wakeup whenever that condition changes.
And the above loop in tcp_output() is a plain memory deadlock, you
should not loop on allocations like that. If the allocation fails;
propagate the error.
Given that this function must not fail; one should pre-allocate proper
reserves, not simply loop until it works. The loop might be holding up
the work required to release memory.
The proper way to fix the dev_deactivate_many() is to use wait_event(),
polling for that state is just daft. Afaict there is no reason the qdisc
code could not do a wakeup whenever that condition changes.
I actually looked into this, and it's going to add expensive checks
to the fast paths of packet processing.
If it was so easy we'd be doing it that way already.
And the above loop in tcp_output() is a plain memory deadlock, you
should not loop on allocations like that. If the allocation fails;
propagate the error.
There is nothing to "propagate" it to. We have no "event" that will
trigger so that we have a second chance to send the FIN out, it really
must go out before we progress any further at this stage.
The proper way to fix the dev_deactivate_many() is to use wait_event(),
polling for that state is just daft. Afaict there is no reason the qdisc
code could not do a wakeup whenever that condition changes.
I actually looked into this, and it's going to add expensive checks
to the fast paths of packet processing.
If it was so easy we'd be doing it that way already.
Fair enough; and I suppose this waiting side is 'rare' compared to the
packet processing side of things?
In that case you could add a comment with the msleep() and just leave it
at that I suppose.
And the above loop in tcp_output() is a plain memory deadlock, you
should not loop on allocations like that. If the allocation fails;
propagate the error.
There is nothing to "propagate" it to. We have no "event" that will
trigger so that we have a second chance to send the FIN out, it really
must go out before we progress any further at this stage.
From what I remember of TCP/IP the FIN packet is used once per
connection; to terminate said connection. So if we pre-allocate one per
connection, at establishment time (where we can still easily fail) we
should be good and safe.
Now, I suppose there's valid arguments against this -- wasted memory
being one I suspect.
Also, seeing how this code has basically been this way forever and has
not caused (afaik) any actual memory deadlocks (they're hard anyway). We
might just get away with not touching it.
That said, at the very least we want a comment here stating that this is
a potential memory deadlock and why the alternatives aren't any good, so
that future readers (possibly us again) know this has been considered.