* Pinning down a blocked task to extract diagnostics
@ 2020-03-05 0:50 Paul E. McKenney
2020-03-05 3:52 ` Steven Rostedt
2020-03-05 8:07 ` Peter Zijlstra
0 siblings, 2 replies; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-05 0:50 UTC (permalink / raw)
To: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann,
rostedt, bsegall, mgorman
Cc: linux-kernel
Hello!
Suppose that I need to extract diagnostics information from a blocked
task, but that I absolutely cannot tolerate this task awakening in the
midst of this extraction process. Is the following code the right way
to make this work given a task "t"?
raw_spin_lock_irq(&t->pi_lock);
if (t->on_rq) {
/* Task no longer blocked, so ignore it. */
} else {
/* Extract consistent diagnostic information. */
}
raw_spin_unlock_irq(&t->pi_lock);
It looks like all the wakeup paths acquire ->pi_lock, but I figured I
should actually ask...
Thanx, Paul
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 0:50 Pinning down a blocked task to extract diagnostics Paul E. McKenney
@ 2020-03-05 3:52 ` Steven Rostedt
2020-03-05 4:25 ` Paul E. McKenney
2020-03-05 8:07 ` Peter Zijlstra
1 sibling, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-03-05 3:52 UTC (permalink / raw)
To: Paul E. McKenney
Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann,
bsegall, mgorman, linux-kernel
On Wed, 4 Mar 2020 16:50:49 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:
> Hello!
>
> Suppose that I need to extract diagnostics information from a blocked
> task, but that I absolutely cannot tolerate this task awakening in the
> midst of this extraction process. Is the following code the right way
> to make this work given a task "t"?
>
> raw_spin_lock_irq(&t->pi_lock);
> if (t->on_rq) {
> /* Task no longer blocked, so ignore it. */
> } else {
> /* Extract consistent diagnostic information. */
> }
> raw_spin_unlock_irq(&t->pi_lock);
>
> It looks like all the wakeup paths acquire ->pi_lock, but I figured I
> should actually ask...
IIUC, the rtmutex code uses pi_lock to tinker with the task while it is
blocked (not woken). But the on_rq test may not be correct. It appears
that can change without holding the task's pi_lock. I'm looking at the
ttwu_do_activate() code which modifies the t->on_rq without holding the
pi_lock. Seems you may need to check the p->state as well. See the
comment in try_to_wake_up() about testing on_rq vs the state.
-- Steve
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 3:52 ` Steven Rostedt
@ 2020-03-05 4:25 ` Paul E. McKenney
0 siblings, 0 replies; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-05 4:25 UTC (permalink / raw)
To: Steven Rostedt
Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann,
bsegall, mgorman, linux-kernel
On Wed, Mar 04, 2020 at 10:52:26PM -0500, Steven Rostedt wrote:
> On Wed, 4 Mar 2020 16:50:49 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
>
> > Hello!
> >
> > Suppose that I need to extract diagnostics information from a blocked
> > task, but that I absolutely cannot tolerate this task awakening in the
> > midst of this extraction process. Is the following code the right way
> > to make this work given a task "t"?
> >
> > raw_spin_lock_irq(&t->pi_lock);
> > if (t->on_rq) {
> > /* Task no longer blocked, so ignore it. */
> > } else {
> > /* Extract consistent diagnostic information. */
> > }
> > raw_spin_unlock_irq(&t->pi_lock);
> >
> > It looks like all the wakeup paths acquire ->pi_lock, but I figured I
> > should actually ask...
>
> IIUC, the rtmutex code uses pi_lock to tinker with the task while it is
> blocked (not woken). But the on_rq test may not be correct. It appears
> that can change without holding the task's pi_lock. I'm looking at the
> ttwu_do_activate() code which modifies the t->on_rq without holding the
> pi_lock. Seems you may need to check the p->state as well. See the
> comment in try_to_wake_up() about testing on_rq vs the state.
Thank you, Steve!
Thanx, Paul
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 0:50 Pinning down a blocked task to extract diagnostics Paul E. McKenney
2020-03-05 3:52 ` Steven Rostedt
@ 2020-03-05 8:07 ` Peter Zijlstra
2020-03-05 8:13 ` Peter Zijlstra
1 sibling, 1 reply; 12+ messages in thread
From: Peter Zijlstra @ 2020-03-05 8:07 UTC (permalink / raw)
To: Paul E. McKenney
Cc: mingo, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt,
bsegall, mgorman, linux-kernel
On Wed, Mar 04, 2020 at 04:50:49PM -0800, Paul E. McKenney wrote:
> Hello!
>
> Suppose that I need to extract diagnostics information from a blocked
> task, but that I absolutely cannot tolerate this task awakening in the
> midst of this extraction process. Is the following code the right way
> to make this work given a task "t"?
>
> raw_spin_lock_irq(&t->pi_lock);
> if (t->on_rq) {
> /* Task no longer blocked, so ignore it. */
> } else {
> /* Extract consistent diagnostic information. */
> }
> raw_spin_unlock_irq(&t->pi_lock);
>
> It looks like all the wakeup paths acquire ->pi_lock, but I figured I
> should actually ask...
Close, the thing pi_lock actually guards is the t->state transition *to*
TASK_WAKING/TASK_RUNNING, so something like this:
raw_spin_lock_irq(&t->pi_lock);
switch (t->state) {
case TASK_RUNNING:
case TASK_WAKING:
/* ignore */
break;
default:
/* Extract consistent diagnostic information. */
break;
}
raw_spin_unlock_irq(&t->pi_lock);
ought to work. But if you're going to do this, please add a reference to
that code in a comment on top of try_to_wake_up(), such that we can
later find all the code that relies on this.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 8:07 ` Peter Zijlstra
@ 2020-03-05 8:13 ` Peter Zijlstra
2020-03-05 14:01 ` Steven Rostedt
2020-03-05 14:22 ` Paul E. McKenney
0 siblings, 2 replies; 12+ messages in thread
From: Peter Zijlstra @ 2020-03-05 8:13 UTC (permalink / raw)
To: Paul E. McKenney
Cc: mingo, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt,
bsegall, mgorman, linux-kernel
On Thu, Mar 05, 2020 at 09:07:55AM +0100, Peter Zijlstra wrote:
> On Wed, Mar 04, 2020 at 04:50:49PM -0800, Paul E. McKenney wrote:
> > Hello!
> >
> > Suppose that I need to extract diagnostics information from a blocked
> > task, but that I absolutely cannot tolerate this task awakening in the
> > midst of this extraction process. Is the following code the right way
> > to make this work given a task "t"?
> >
> > raw_spin_lock_irq(&t->pi_lock);
> > if (t->on_rq) {
> > /* Task no longer blocked, so ignore it. */
> > } else {
> > /* Extract consistent diagnostic information. */
> > }
> > raw_spin_unlock_irq(&t->pi_lock);
> >
> > It looks like all the wakeup paths acquire ->pi_lock, but I figured I
> > should actually ask...
>
> Close, the thing pi_lock actually guards is the t->state transition *to*
> TASK_WAKING/TASK_RUNNING, so something like this:
Almost, we must indeed also check ->on_rq, otherwise it might change the
state back itself.
>
> raw_spin_lock_irq(&t->pi_lock);
> switch (t->state) {
> case TASK_RUNNING:
> case TASK_WAKING:
> /* ignore */
> break;
>
> default:
if (t->on_rq)
break;
> /* Extract consistent diagnostic information. */
> break;
> }
> raw_spin_unlock_irq(&t->pi_lock);
>
> ought to work. But if you're going to do this, please add a reference to
> that code in a comment on top of try_to_wake_up(), such that we can
> later find all the code that relies on this.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 8:13 ` Peter Zijlstra
@ 2020-03-05 14:01 ` Steven Rostedt
2020-03-05 14:22 ` Paul E. McKenney
1 sibling, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-03-05 14:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, mingo, juri.lelli, vincent.guittot,
dietmar.eggemann, bsegall, mgorman, linux-kernel
On Thu, 5 Mar 2020 09:13:37 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> > Close, the thing pi_lock actually guards is the t->state transition *to*
> > TASK_WAKING/TASK_RUNNING, so something like this:
>
> Almost, we must indeed also check ->on_rq, otherwise it might change the
> state back itself.
>
> >
> > raw_spin_lock_irq(&t->pi_lock);
> > switch (t->state) {
> > case TASK_RUNNING:
> > case TASK_WAKING:
> > /* ignore */
> > break;
> >
> > default:
Don't we need a smp_rmb() here, otherwise we could have the same issue as
described in the comment in try_to_wake_up()?
-- Steve
> if (t->on_rq)
> break;
>
> > /* Extract consistent diagnostic information. */
> > break;
> > }
> > raw_spin_unlock_irq(&t->pi_lock);
> >
> > ought to work. But if you're going to do this, please add a reference to
> > that code in a comment on top of try_to_wake_up(), such that we can
> > later find all the code that relies on this.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 8:13 ` Peter Zijlstra
2020-03-05 14:01 ` Steven Rostedt
@ 2020-03-05 14:22 ` Paul E. McKenney
2020-03-05 14:28 ` Steven Rostedt
1 sibling, 1 reply; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-05 14:22 UTC (permalink / raw)
To: Peter Zijlstra
Cc: mingo, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt,
bsegall, mgorman, linux-kernel
On Thu, Mar 05, 2020 at 09:13:37AM +0100, Peter Zijlstra wrote:
> On Thu, Mar 05, 2020 at 09:07:55AM +0100, Peter Zijlstra wrote:
> > On Wed, Mar 04, 2020 at 04:50:49PM -0800, Paul E. McKenney wrote:
> > > Hello!
> > >
> > > Suppose that I need to extract diagnostics information from a blocked
> > > task, but that I absolutely cannot tolerate this task awakening in the
> > > midst of this extraction process. Is the following code the right way
> > > to make this work given a task "t"?
> > >
> > > raw_spin_lock_irq(&t->pi_lock);
> > > if (t->on_rq) {
> > > /* Task no longer blocked, so ignore it. */
> > > } else {
> > > /* Extract consistent diagnostic information. */
> > > }
> > > raw_spin_unlock_irq(&t->pi_lock);
> > >
> > > It looks like all the wakeup paths acquire ->pi_lock, but I figured I
> > > should actually ask...
> >
> > Close, the thing pi_lock actually guards is the t->state transition *to*
> > TASK_WAKING/TASK_RUNNING, so something like this:
>
> Almost, we must indeed also check ->on_rq, otherwise it might change the
> state back itself.
>
> >
> > raw_spin_lock_irq(&t->pi_lock);
> > switch (t->state) {
> > case TASK_RUNNING:
> > case TASK_WAKING:
> > /* ignore */
> > break;
> >
> > default:
> if (t->on_rq)
> break;
>
> > /* Extract consistent diagnostic information. */
> > break;
> > }
> > raw_spin_unlock_irq(&t->pi_lock);
> >
> > ought to work. But if you're going to do this, please add a reference to
> > that code in a comment on top of try_to_wake_up(), such that we can
> > later find all the code that relies on this.
How about if I add something like this, located right by try_to_wake_up()?
bool try_to_keep_sleeping(struct task_struct *t)
{
raw_spin_lock_irq(&t->pi_lock);
switch (t->state) {
case TASK_RUNNING:
case TASK_WAKING:
raw_spin_unlock_irq(&t->pi_lock);
return false;
default:
if (t->on_rq) {
raw_spin_unlock_irq(&t->pi_lock);
return false;
}
/* OK to extract consistent diagnostic information. */
return true;
}
/* NOTREACHED */
}
Then a use might look like this:
if (try_to_keep_sleeping(t))
/* Extract consistent diagnostic information. */
raw_spin_unlock_irq(&t->pi_lock);
} else {
/* Woo-hoo! It started running again!!! */
}
Is there a better way to approach this?
Thanx, Paul
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 14:22 ` Paul E. McKenney
@ 2020-03-05 14:28 ` Steven Rostedt
2020-03-05 15:36 ` Paul E. McKenney
0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-03-05 14:28 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Peter Zijlstra, mingo, juri.lelli, vincent.guittot,
dietmar.eggemann, bsegall, mgorman, linux-kernel
On Thu, 5 Mar 2020 06:22:45 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:
> On Thu, Mar 05, 2020 at 09:13:37AM +0100, Peter Zijlstra wrote:
> > On Thu, Mar 05, 2020 at 09:07:55AM +0100, Peter Zijlstra wrote:
> > > On Wed, Mar 04, 2020 at 04:50:49PM -0800, Paul E. McKenney wrote:
> > > > Hello!
> > > >
> > > > Suppose that I need to extract diagnostics information from a blocked
> > > > task, but that I absolutely cannot tolerate this task awakening in the
> > > > midst of this extraction process. Is the following code the right way
> > > > to make this work given a task "t"?
> > > >
> > > > raw_spin_lock_irq(&t->pi_lock);
> > > > if (t->on_rq) {
> > > > /* Task no longer blocked, so ignore it. */
> > > > } else {
> > > > /* Extract consistent diagnostic information. */
> > > > }
> > > > raw_spin_unlock_irq(&t->pi_lock);
> > > >
> > > > It looks like all the wakeup paths acquire ->pi_lock, but I figured I
> > > > should actually ask...
> > >
> > > Close, the thing pi_lock actually guards is the t->state transition *to*
> > > TASK_WAKING/TASK_RUNNING, so something like this:
> >
> > Almost, we must indeed also check ->on_rq, otherwise it might change the
> > state back itself.
> >
> > >
> > > raw_spin_lock_irq(&t->pi_lock);
> > > switch (t->state) {
> > > case TASK_RUNNING:
> > > case TASK_WAKING:
> > > /* ignore */
> > > break;
> > >
> > > default:
> > if (t->on_rq)
> > break;
> >
> > > /* Extract consistent diagnostic information. */
> > > break;
> > > }
> > > raw_spin_unlock_irq(&t->pi_lock);
> > >
> > > ought to work. But if you're going to do this, please add a reference to
> > > that code in a comment on top of try_to_wake_up(), such that we can
> > > later find all the code that relies on this.
>
> How about if I add something like this, located right by try_to_wake_up()?
>
> bool try_to_keep_sleeping(struct task_struct *t)
> {
> raw_spin_lock_irq(&t->pi_lock);
> switch (t->state) {
> case TASK_RUNNING:
> case TASK_WAKING:
> raw_spin_unlock_irq(&t->pi_lock);
> return false;
>
> default:
> if (t->on_rq) {
Somehow I think there still needs to be a read barrier before the test to
on_rq.
> raw_spin_unlock_irq(&t->pi_lock);
> return false;
> }
>
> /* OK to extract consistent diagnostic information. */
> return true;
> }
> /* NOTREACHED */
> }
>
> Then a use might look like this:
>
> if (try_to_keep_sleeping(t))
> /* Extract consistent diagnostic information. */
> raw_spin_unlock_irq(&t->pi_lock);
Perhaps we should have a allow_awake(t) to match it?
allow_awake(t);
Where we have:
static inline allow_awake(struct task_struct *t)
{
raw_spin_unlock_irq(&t->pi_lock);
}
-- Steve?
> } else {
> /* Woo-hoo! It started running again!!! */
> }
>
> Is there a better way to approach this?
>
> Thanx, Paul
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 14:28 ` Steven Rostedt
@ 2020-03-05 15:36 ` Paul E. McKenney
2020-03-06 1:40 ` Paul E. McKenney
0 siblings, 1 reply; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-05 15:36 UTC (permalink / raw)
To: Steven Rostedt
Cc: Peter Zijlstra, mingo, juri.lelli, vincent.guittot,
dietmar.eggemann, bsegall, mgorman, linux-kernel
On Thu, Mar 05, 2020 at 09:28:45AM -0500, Steven Rostedt wrote:
> On Thu, 5 Mar 2020 06:22:45 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
>
> > On Thu, Mar 05, 2020 at 09:13:37AM +0100, Peter Zijlstra wrote:
> > > On Thu, Mar 05, 2020 at 09:07:55AM +0100, Peter Zijlstra wrote:
> > > > On Wed, Mar 04, 2020 at 04:50:49PM -0800, Paul E. McKenney wrote:
> > > > > Hello!
> > > > >
> > > > > Suppose that I need to extract diagnostics information from a blocked
> > > > > task, but that I absolutely cannot tolerate this task awakening in the
> > > > > midst of this extraction process. Is the following code the right way
> > > > > to make this work given a task "t"?
> > > > >
> > > > > raw_spin_lock_irq(&t->pi_lock);
> > > > > if (t->on_rq) {
> > > > > /* Task no longer blocked, so ignore it. */
> > > > > } else {
> > > > > /* Extract consistent diagnostic information. */
> > > > > }
> > > > > raw_spin_unlock_irq(&t->pi_lock);
> > > > >
> > > > > It looks like all the wakeup paths acquire ->pi_lock, but I figured I
> > > > > should actually ask...
> > > >
> > > > Close, the thing pi_lock actually guards is the t->state transition *to*
> > > > TASK_WAKING/TASK_RUNNING, so something like this:
> > >
> > > Almost, we must indeed also check ->on_rq, otherwise it might change the
> > > state back itself.
> > >
> > > >
> > > > raw_spin_lock_irq(&t->pi_lock);
> > > > switch (t->state) {
> > > > case TASK_RUNNING:
> > > > case TASK_WAKING:
> > > > /* ignore */
> > > > break;
> > > >
> > > > default:
> > > if (t->on_rq)
> > > break;
> > >
> > > > /* Extract consistent diagnostic information. */
> > > > break;
> > > > }
> > > > raw_spin_unlock_irq(&t->pi_lock);
> > > >
> > > > ought to work. But if you're going to do this, please add a reference to
> > > > that code in a comment on top of try_to_wake_up(), such that we can
> > > > later find all the code that relies on this.
> >
> > How about if I add something like this, located right by try_to_wake_up()?
> >
> > bool try_to_keep_sleeping(struct task_struct *t)
> > {
> > raw_spin_lock_irq(&t->pi_lock);
> > switch (t->state) {
> > case TASK_RUNNING:
> > case TASK_WAKING:
> > raw_spin_unlock_irq(&t->pi_lock);
> > return false;
> >
> > default:
> > if (t->on_rq) {
>
> Somehow I think there still needs to be a read barrier before the test to
> on_rq.
This is nowhere near a fastpath, so if there is uncertainty it gets
the smp_rmb(). Or an smp_load_acquire() on t->state.
> > raw_spin_unlock_irq(&t->pi_lock);
> > return false;
> > }
> >
> > /* OK to extract consistent diagnostic information. */
> > return true;
> > }
> > /* NOTREACHED */
> > }
> >
> > Then a use might look like this:
> >
> > if (try_to_keep_sleeping(t))
> > /* Extract consistent diagnostic information. */
> > raw_spin_unlock_irq(&t->pi_lock);
>
> Perhaps we should have a allow_awake(t) to match it?
>
> allow_awake(t);
>
> Where we have:
>
> static inline allow_awake(struct task_struct *t)
> {
> raw_spin_unlock_irq(&t->pi_lock);
> }
Makes sense to me!
Thanx, Paul
> -- Steve?
>
> > } else {
> > /* Woo-hoo! It started running again!!! */
> > }
> >
> > Is there a better way to approach this?
> >
> > Thanx, Paul
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-05 15:36 ` Paul E. McKenney
@ 2020-03-06 1:40 ` Paul E. McKenney
2020-03-06 16:56 ` Steven Rostedt
0 siblings, 1 reply; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-06 1:40 UTC (permalink / raw)
To: Steven Rostedt
Cc: Peter Zijlstra, mingo, juri.lelli, vincent.guittot,
dietmar.eggemann, bsegall, mgorman, linux-kernel
On Thu, Mar 05, 2020 at 07:36:38AM -0800, Paul E. McKenney wrote:
> On Thu, Mar 05, 2020 at 09:28:45AM -0500, Steven Rostedt wrote:
> > On Thu, 5 Mar 2020 06:22:45 -0800
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> >
> > > On Thu, Mar 05, 2020 at 09:13:37AM +0100, Peter Zijlstra wrote:
[ . . . ]
> > > How about if I add something like this, located right by try_to_wake_up()?
> > >
> > > bool try_to_keep_sleeping(struct task_struct *t)
> > > {
> > > raw_spin_lock_irq(&t->pi_lock);
> > > switch (t->state) {
> > > case TASK_RUNNING:
> > > case TASK_WAKING:
> > > raw_spin_unlock_irq(&t->pi_lock);
> > > return false;
> > >
> > > default:
> > > if (t->on_rq) {
> >
> > Somehow I think there still needs to be a read barrier before the test to
> > on_rq.
>
> This is nowhere near a fastpath, so if there is uncertainty it gets
> the smp_rmb(). Or an smp_load_acquire() on t->state.
>
> > > raw_spin_unlock_irq(&t->pi_lock);
> > > return false;
> > > }
> > >
> > > /* OK to extract consistent diagnostic information. */
> > > return true;
> > > }
> > > /* NOTREACHED */
> > > }
> > >
> > > Then a use might look like this:
> > >
> > > if (try_to_keep_sleeping(t))
> > > /* Extract consistent diagnostic information. */
> > > raw_spin_unlock_irq(&t->pi_lock);
> >
> > Perhaps we should have a allow_awake(t) to match it?
> >
> > allow_awake(t);
> >
> > Where we have:
> >
> > static inline allow_awake(struct task_struct *t)
> > {
> > raw_spin_unlock_irq(&t->pi_lock);
> > }
>
> Makes sense to me!
So how about like this?
Thanx, Paul
------------------------------------------------------------------------
commit e2821ae6c6a6adaabc89ccd9babf4375a78e0626
Author: Paul E. McKenney <paulmck@kernel.org>
Date: Thu Mar 5 16:53:58 2020 -0800
sched/core: Add functions to prevent sleepers from awakening
In some cases, it is necessary to examine a consistent version of a
sleeping process's state, in other words, it is necessary to keep
that process in sleeping state. This commit therefore provides a
try_to_keep_sleeping() function that acquires ->pi_lock to prevent
wakeups from proceeding, returning true if the function is still asleep,
and otherwise releasing ->pi_lock and returning false.
This commit also provides an allow_awake() function (as suggested by
by Steven Rostedt) that reverses the effect of a successful call to
try_to_keep_sleeping(), allowing the process to once again be awakened.
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
[ paulmck: Apply feedback from Peter Zijlstra and Steven Rostedt. ]
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3283c8d..aefea4a 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1148,4 +1148,7 @@ int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, i
(wait)->flags = 0; \
} while (0)
+bool try_to_keep_sleeping(struct task_struct *p);
+void allow_awake(struct task_struct *p);
+
#endif /* _LINUX_WAIT_H */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index fc1dfc0..b665ff7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2654,6 +2654,48 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
}
/**
+ * try_to_keep_sleeping - Attempt to force task to remain off runqueues
+ * @p: The process to remain asleep.
+ *
+ * Acquires the process's ->pi_lock and checks state. If the process
+ * is still blocked, returns @true and leave ->pi_lock held, otherwise
+ * releases ->pi_locked and returns @false.
+ */
+bool try_to_keep_sleeping(struct task_struct *p)
+{
+ lockdep_assert_irqs_enabled();
+ raw_spin_lock_irq(&p->pi_lock);
+ switch (p->state) {
+ case TASK_RUNNING:
+ case TASK_WAKING:
+ raw_spin_unlock_irq(&p->pi_lock);
+ return false;
+
+ default:
+ smp_rmb(); /* See comments in try_to_wake_up(). */
+ if (p->on_rq) {
+ raw_spin_unlock_irq(&p->pi_lock);
+ return false;
+ }
+ return true; /* Process is now stuck in blocked state. */
+ }
+ /* NOTREACHED */
+}
+
+/**
+ * allow_awake - Allow a kept-sleeping process to awaken
+ * @p: Process to be allowed to awaken.
+ *
+ * Given that @p was passed to an earlier call to try_to_keep_sleeping
+ * that returned @true, hence preventing @p from waking up, allow @p
+ * to once again be awakened.
+ */
+void allow_awake(struct task_struct *p)
+{
+ raw_spin_unlock_irq(&p->pi_lock);
+}
+
+/**
* wake_up_process - Wake up a specific process
* @p: The process to be woken up.
*
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-06 1:40 ` Paul E. McKenney
@ 2020-03-06 16:56 ` Steven Rostedt
2020-03-06 21:01 ` Paul E. McKenney
0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-03-06 16:56 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Peter Zijlstra, mingo, juri.lelli, vincent.guittot,
dietmar.eggemann, bsegall, mgorman, linux-kernel
On Thu, 5 Mar 2020 17:40:27 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:
> commit e2821ae6c6a6adaabc89ccd9babf4375a78e0626
> Author: Paul E. McKenney <paulmck@kernel.org>
> Date: Thu Mar 5 16:53:58 2020 -0800
>
> sched/core: Add functions to prevent sleepers from awakening
>
> In some cases, it is necessary to examine a consistent version of a
> sleeping process's state, in other words, it is necessary to keep
> that process in sleeping state. This commit therefore provides a
> try_to_keep_sleeping() function that acquires ->pi_lock to prevent
> wakeups from proceeding, returning true if the function is still asleep,
> and otherwise releasing ->pi_lock and returning false.
>
> This commit also provides an allow_awake() function (as suggested by
> by Steven Rostedt) that reverses the effect of a successful call to
> try_to_keep_sleeping(), allowing the process to once again be awakened.
>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> [ paulmck: Apply feedback from Peter Zijlstra and Steven Rostedt. ]
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ben Segall <bsegall@google.com>
> Cc: Mel Gorman <mgorman@suse.de>
>
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 3283c8d..aefea4a 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -1148,4 +1148,7 @@ int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, i
> (wait)->flags = 0; \
> } while (0)
>
> +bool try_to_keep_sleeping(struct task_struct *p);
> +void allow_awake(struct task_struct *p);
> +
> #endif /* _LINUX_WAIT_H */
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index fc1dfc0..b665ff7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2654,6 +2654,48 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> }
>
> /**
> + * try_to_keep_sleeping - Attempt to force task to remain off runqueues
> + * @p: The process to remain asleep.
> + *
> + * Acquires the process's ->pi_lock and checks state. If the process
> + * is still blocked, returns @true and leave ->pi_lock held, otherwise
> + * releases ->pi_locked and returns @false.
I would add a comment here that this is paired with allow_awake(). As well
as a "Returns" statement.
* Returns:
* false if the task is awake, then no lock is taken.
* true if the task is sleeping, and then task's pi_lock will be held.
* allow_awake() must be used to release the pi_lock and let
* task @p awake again.
> + */
> +bool try_to_keep_sleeping(struct task_struct *p)
> +{
> + lockdep_assert_irqs_enabled();
> + raw_spin_lock_irq(&p->pi_lock);
> + switch (p->state) {
> + case TASK_RUNNING:
> + case TASK_WAKING:
> + raw_spin_unlock_irq(&p->pi_lock);
> + return false;
> +
> + default:
> + smp_rmb(); /* See comments in try_to_wake_up(). */
I remember Peter asking to add a comment in try_to_wake_up() stating that
this is used, so that if that code is changed, this code may also need to
be updated.
-- Steve
> + if (p->on_rq) {
> + raw_spin_unlock_irq(&p->pi_lock);
> + return false;
> + }
> + return true; /* Process is now stuck in blocked state. */
> + }
> + /* NOTREACHED */
> +}
> +
> +/**
> + * allow_awake - Allow a kept-sleeping process to awaken
> + * @p: Process to be allowed to awaken.
> + *
> + * Given that @p was passed to an earlier call to try_to_keep_sleeping
> + * that returned @true, hence preventing @p from waking up, allow @p
> + * to once again be awakened.
> + */
> +void allow_awake(struct task_struct *p)
> +{
> + raw_spin_unlock_irq(&p->pi_lock);
> +}
> +
> +/**
> * wake_up_process - Wake up a specific process
> * @p: The process to be woken up.
> *
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Pinning down a blocked task to extract diagnostics
2020-03-06 16:56 ` Steven Rostedt
@ 2020-03-06 21:01 ` Paul E. McKenney
0 siblings, 0 replies; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-06 21:01 UTC (permalink / raw)
To: Steven Rostedt
Cc: Peter Zijlstra, mingo, juri.lelli, vincent.guittot,
dietmar.eggemann, bsegall, mgorman, linux-kernel
On Fri, Mar 06, 2020 at 11:56:11AM -0500, Steven Rostedt wrote:
> On Thu, 5 Mar 2020 17:40:27 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
>
> > commit e2821ae6c6a6adaabc89ccd9babf4375a78e0626
> > Author: Paul E. McKenney <paulmck@kernel.org>
> > Date: Thu Mar 5 16:53:58 2020 -0800
> >
> > sched/core: Add functions to prevent sleepers from awakening
> >
> > In some cases, it is necessary to examine a consistent version of a
> > sleeping process's state, in other words, it is necessary to keep
> > that process in sleeping state. This commit therefore provides a
> > try_to_keep_sleeping() function that acquires ->pi_lock to prevent
> > wakeups from proceeding, returning true if the function is still asleep,
> > and otherwise releasing ->pi_lock and returning false.
> >
> > This commit also provides an allow_awake() function (as suggested by
> > by Steven Rostedt) that reverses the effect of a successful call to
> > try_to_keep_sleeping(), allowing the process to once again be awakened.
> >
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > [ paulmck: Apply feedback from Peter Zijlstra and Steven Rostedt. ]
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Juri Lelli <juri.lelli@redhat.com>
> > Cc: Vincent Guittot <vincent.guittot@linaro.org>
> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ben Segall <bsegall@google.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> >
> > diff --git a/include/linux/wait.h b/include/linux/wait.h
> > index 3283c8d..aefea4a 100644
> > --- a/include/linux/wait.h
> > +++ b/include/linux/wait.h
> > @@ -1148,4 +1148,7 @@ int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, i
> > (wait)->flags = 0; \
> > } while (0)
> >
> > +bool try_to_keep_sleeping(struct task_struct *p);
> > +void allow_awake(struct task_struct *p);
> > +
> > #endif /* _LINUX_WAIT_H */
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index fc1dfc0..b665ff7 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -2654,6 +2654,48 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> > }
> >
> > /**
> > + * try_to_keep_sleeping - Attempt to force task to remain off runqueues
> > + * @p: The process to remain asleep.
> > + *
> > + * Acquires the process's ->pi_lock and checks state. If the process
> > + * is still blocked, returns @true and leave ->pi_lock held, otherwise
> > + * releases ->pi_locked and returns @false.
>
> I would add a comment here that this is paired with allow_awake(). As well
> as a "Returns" statement.
>
> * Returns:
> * false if the task is awake, then no lock is taken.
> * true if the task is sleeping, and then task's pi_lock will be held.
> * allow_awake() must be used to release the pi_lock and let
> * task @p awake again.
Good point, added.
> > + */
> > +bool try_to_keep_sleeping(struct task_struct *p)
> > +{
> > + lockdep_assert_irqs_enabled();
> > + raw_spin_lock_irq(&p->pi_lock);
> > + switch (p->state) {
> > + case TASK_RUNNING:
> > + case TASK_WAKING:
> > + raw_spin_unlock_irq(&p->pi_lock);
> > + return false;
> > +
> > + default:
> > + smp_rmb(); /* See comments in try_to_wake_up(). */
>
> I remember Peter asking to add a comment in try_to_wake_up() stating that
> this is used, so that if that code is changed, this code may also need to
> be updated.
Adding a reference to try_to_keep_sleeping() in try_to_wake_up()'s
smp_rmb() comment, as shown below?
Thanx, Paul
------------------------------------------------------------------------
commit 57008d8adaa1e93acedea23e4858b9831e0dd075
Author: Paul E. McKenney <paulmck@kernel.org>
Date: Thu Mar 5 16:53:58 2020 -0800
sched/core: Add functions to prevent sleepers from awakening
In some cases, it is necessary to examine a consistent version of a
sleeping process's state, in other words, it is necessary to keep
that process in sleeping state. This commit therefore provides a
try_to_keep_sleeping() function that acquires ->pi_lock to prevent
wakeups from proceeding, returning true if the function is still asleep,
and otherwise releasing ->pi_lock and returning false.
This commit also provides an allow_awake() function (as suggested by
by Steven Rostedt) that reverses the effect of a successful call to
try_to_keep_sleeping(), allowing the process to once again be awakened.
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
[ paulmck: Apply feedback from Peter Zijlstra and Steven Rostedt. ]
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3283c8d..aefea4a 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1148,4 +1148,7 @@ int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, i
(wait)->flags = 0; \
} while (0)
+bool try_to_keep_sleeping(struct task_struct *p);
+void allow_awake(struct task_struct *p);
+
#endif /* _LINUX_WAIT_H */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index fc1dfc0..a935a3d 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2580,6 +2580,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
*
* Pairs with the LOCK+smp_mb__after_spinlock() on rq->lock in
* __schedule(). See the comment for smp_mb__after_spinlock().
+ *
+ * A similar smp_rmb() lives in try_to_keep_sleeping().
*/
smp_rmb();
if (p->on_rq && ttwu_remote(p, wake_flags))
@@ -2654,6 +2656,54 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
}
/**
+ * try_to_keep_sleeping - Attempt to force task to remain off runqueues
+ * @p: The process to remain asleep.
+ *
+ * Acquires the process's ->pi_lock and checks state. If the process
+ * is still blocked, returns @true and leave ->pi_lock held, otherwise
+ * releases ->pi_lock and returns @false.
+ *
+ * Returns:
+ * @false if the task is awake, in which case no lock is held.
+ * @true if the task is sleeping, in which case the process's
+ * ->pi_lock will be held. Use allow_awake() to release
+ * this lock and thus allow process @p to awaken.
+ */
+bool try_to_keep_sleeping(struct task_struct *p)
+{
+ lockdep_assert_irqs_enabled();
+ raw_spin_lock_irq(&p->pi_lock);
+ switch (p->state) {
+ case TASK_RUNNING:
+ case TASK_WAKING:
+ raw_spin_unlock_irq(&p->pi_lock);
+ return false;
+
+ default:
+ smp_rmb(); /* See comments in try_to_wake_up(). */
+ if (p->on_rq) {
+ raw_spin_unlock_irq(&p->pi_lock);
+ return false;
+ }
+ return true; /* Process is now stuck in blocked state. */
+ }
+ /* NOTREACHED */
+}
+
+/**
+ * allow_awake - Allow a kept-sleeping process to awaken
+ * @p: Process to be allowed to awaken.
+ *
+ * Given that @p was passed to an earlier call to try_to_keep_sleeping
+ * that returned @true, hence preventing @p from waking up, allow @p
+ * to once again be awakened.
+ */
+void allow_awake(struct task_struct *p)
+{
+ raw_spin_unlock_irq(&p->pi_lock);
+}
+
+/**
* wake_up_process - Wake up a specific process
* @p: The process to be woken up.
*
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-03-06 21:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-05 0:50 Pinning down a blocked task to extract diagnostics Paul E. McKenney
2020-03-05 3:52 ` Steven Rostedt
2020-03-05 4:25 ` Paul E. McKenney
2020-03-05 8:07 ` Peter Zijlstra
2020-03-05 8:13 ` Peter Zijlstra
2020-03-05 14:01 ` Steven Rostedt
2020-03-05 14:22 ` Paul E. McKenney
2020-03-05 14:28 ` Steven Rostedt
2020-03-05 15:36 ` Paul E. McKenney
2020-03-06 1:40 ` Paul E. McKenney
2020-03-06 16:56 ` Steven Rostedt
2020-03-06 21:01 ` Paul E. McKenney
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.