Pinning down a blocked task to extract diagnostics

Pinning down a blocked task to extract diagnostics

[Paul E. McKenney]

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

Re: Pinning down a blocked task to extract diagnostics

[Steven Rostedt]

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

Re: Pinning down a blocked task to extract diagnostics

[Paul E. McKenney]

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

Re: Pinning down a blocked task to extract diagnostics

[Peter Zijlstra]

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.

Re: Pinning down a blocked task to extract diagnostics

[Peter Zijlstra]

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.

Re: Pinning down a blocked task to extract diagnostics

[Steven Rostedt]

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.  

Re: Pinning down a blocked task to extract diagnostics

[Paul E. McKenney]

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

Re: Pinning down a blocked task to extract diagnostics

[Steven Rostedt]

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

Re: Pinning down a blocked task to extract diagnostics

[Paul E. McKenney]

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
> 

Re: Pinning down a blocked task to extract diagnostics

[Paul E. McKenney]

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.
  *

Re: Pinning down a blocked task to extract diagnostics

[Steven Rostedt]

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.
>   *

Re: Pinning down a blocked task to extract diagnostics

[Paul E. McKenney]

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.
  *