From: Fabian Frederick <hidden> Date: 2015-02-20 18:13:13
Use helper functions to access current->state.
Direct assignments are prone to races and therefore buggy.
current->state = TASK_RUNNING is replaced by __set_current_state()
Thanks to Peter Zijlstra for the exact definition of the problem.
Suggested-By: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fabian Frederick <redacted>
---
drivers/net/wan/cosa.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-02-20 18:58:12
On Fri, Feb 20, 2015 at 09:34:28PM +0300, Sergei Shtylyov wrote:
Hello.
On 02/20/2015 09:12 PM, Fabian Frederick wrote:
quoted
Use helper functions to access current->state.
Direct assignments are prone to races and therefore buggy.
quoted
current->state = TASK_RUNNING is replaced by __set_current_state()
You sometimes use __set_current_state() and sometimes set_current_state().
It depends on which state; setting yourself TASK_RUNNING is free of
wakeup races -- you're already running after all, so it can safely use
__set_current_state().
Setting a blocking state otoh needs set_current_state() which issues a
full memory barriers with the store (critically in this case,
effectively after the store) such that it orders the state store with a
subsequent load in the condition check if it really needs to go to
sleep.
In full:
current->state = TASK_UNINTERRUPTIBLE; wait = false;
smp_mb(); smp_wmb();
if (wait) p->state = TASK_RUNNING;
schedule();
Without that smp_mb(); the following order is possible:
if (wait)
wait = false;
smp_wmb();
p->state = TASK_RUNNING;
current->state = TASK_UNINTERRUPTIBLE;
schedule();
And we'll wait forever more..
From: Eric Dumazet <hidden> Date: 2015-02-20 19:31:57
On Fri, 2015-02-20 at 19:58 +0100, Peter Zijlstra wrote:
On Fri, Feb 20, 2015 at 09:34:28PM +0300, Sergei Shtylyov wrote:
quoted
Hello.
On 02/20/2015 09:12 PM, Fabian Frederick wrote:
quoted
Use helper functions to access current->state.
Direct assignments are prone to races and therefore buggy.
quoted
current->state = TASK_RUNNING is replaced by __set_current_state()
You sometimes use __set_current_state() and sometimes set_current_state().
It depends on which state; setting yourself TASK_RUNNING is free of
wakeup races -- you're already running after all, so it can safely use
__set_current_state().
Maybe this might be self documented in set_current_state(),
as we have about 120 calls to __set_current_state(TASK_RUNNING)
From: Eric Dumazet <hidden> Date: 2015-02-20 19:44:41
On Fri, 2015-02-20 at 11:31 -0800, Eric Dumazet wrote:
On Fri, 2015-02-20 at 19:58 +0100, Peter Zijlstra wrote:
quoted
On Fri, Feb 20, 2015 at 09:34:28PM +0300, Sergei Shtylyov wrote:
quoted
Hello.
On 02/20/2015 09:12 PM, Fabian Frederick wrote:
quoted
Use helper functions to access current->state.
Direct assignments are prone to races and therefore buggy.
quoted
current->state = TASK_RUNNING is replaced by __set_current_state()
You sometimes use __set_current_state() and sometimes set_current_state().
It depends on which state; setting yourself TASK_RUNNING is free of
wakeup races -- you're already running after all, so it can safely use
__set_current_state().
Maybe this might be self documented in set_current_state(),
as we have about 120 calls to __set_current_state(TASK_RUNNING)
And about 138 calls to set_current_state(TASK_RUNNING)
lkml.kernel.org/r/20150206163947.GR21418@twins.programming.kicks-ass.net
The problem is that there _might_ be someone relying on that barrier.
Its (very) unlikely, but you don't want to risk subtle borkage just
because. And I'm too lazy to go audit all of them :/
From: Fabian Frederick <hidden> Date: 2015-02-21 07:42:39
On 20 February 2015 at 19:58 Peter Zijlstra [off-list ref] wrote:
On Fri, Feb 20, 2015 at 09:34:28PM +0300, Sergei Shtylyov wrote:
quoted
Hello.
On 02/20/2015 09:12 PM, Fabian Frederick wrote:
quoted
Use helper functions to access current->state.
Direct assignments are prone to races and therefore buggy.
quoted
current->state = TASK_RUNNING is replaced by __set_current_state()
You sometimes use __set_current_state() and sometimes
set_current_state().
It depends on which state; setting yourself TASK_RUNNING is free of
wakeup races -- you're already running after all, so it can safely use
__set_current_state().
Setting a blocking state otoh needs set_current_state() which issues a
full memory barriers with the store (critically in this case,
effectively after the store) such that it orders the state store with a
subsequent load in the condition check if it really needs to go to
sleep.
In full:
current->state = TASK_UNINTERRUPTIBLE; wait = false;
smp_mb(); smp_wmb();
if (wait) p->state = TASK_RUNNING;
schedule();
Without that smp_mb(); the following order is possible:
if (wait)
wait = false;
smp_wmb();
p->state = TASK_RUNNING;
current->state = TASK_UNINTERRUPTIBLE;
schedule();
And we'll wait forever more..
Do I have to add more comments in changelogs or is it OK for you ?
Maybe something like:
"
current->state = TASK_RUNNING can safely be converted to __set_current_state()
as we're already in that state. Other assignments are converted to
set_current_state() (which uses set_mb()).
"
Regards,
Fabian
From: David Miller <davem@davemloft.net> Date: 2015-02-22 20:25:13
From: Fabian Frederick <redacted>
Date: Fri, 20 Feb 2015 19:12:56 +0100
Use helper functions to access current->state.
Direct assignments are prone to races and therefore buggy.
current->state = TASK_RUNNING is replaced by __set_current_state()
Thanks to Peter Zijlstra for the exact definition of the problem.
Suggested-By: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fabian Frederick <redacted>