Linux-rt-users archive on lore.kernel.org
 help / color / Atom feed
* [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work
@ 2020-01-24 11:11 Scott Wood
  2020-01-24 11:11 ` [PATCH RT 2/2] sched: migrate_enable: Remove __schedule() call Scott Wood
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Scott Wood @ 2020-01-24 11:11 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-rt-users, linux-kernel, Steven Rostedt, Thomas Gleixner,
	Scott Wood

Commit e6c287b1512d ("sched: migrate_enable: Use stop_one_cpu_nowait()")
adds a busy wait to deal with an edge case where the migrated thread
can resume running on another CPU before the stopper has consumed
cpu_stop_work.  However, this is done with preemption disabled and can
potentially lead to deadlock.

While it is not guaranteed that the cpu_stop_work will be consumed before
the migrating thread resumes and exits the stack frame, it is guaranteed
that nothing other than the stopper can run on the old cpu between the
migrating thread scheduling out and the cpu_stop_work being consumed.
Thus, we can store cpu_stop_work in per-cpu data without it being
reused too early.

Fixes: e6c287b1512d ("sched: migrate_enable: Use stop_one_cpu_nowait()")
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Scott Wood <swood@redhat.com>
---
Ignore the other 1/2 just sent -- forgot the RT in the subject and
didn't quite hit Ctrl-C in time.

 kernel/sched/core.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 754f6afb438d..7713e9c34ad1 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8189,6 +8189,9 @@ static void migrate_disabled_sched(struct task_struct *p)
 	p->migrate_disable_scheduled = 1;
 }
 
+static DEFINE_PER_CPU(struct cpu_stop_work, migrate_work);
+static DEFINE_PER_CPU(struct migration_arg, migrate_arg);
+
 void migrate_enable(void)
 {
 	struct task_struct *p = current;
@@ -8227,22 +8230,25 @@ void migrate_enable(void)
 
 	WARN_ON(smp_processor_id() != cpu);
 	if (!is_cpu_allowed(p, cpu)) {
-		struct migration_arg arg = { .task = p };
-		struct cpu_stop_work work;
+		struct migration_arg __percpu *arg;
+		struct cpu_stop_work __percpu *work;
 		struct rq_flags rf;
 
+		work = this_cpu_ptr(&migrate_work);
+		arg = this_cpu_ptr(&migrate_arg);
+		WARN_ON_ONCE(!arg->done && !work->disabled && work->arg);
+
+		arg->task = p;
+		arg->done = false;
+
 		rq = task_rq_lock(p, &rf);
 		update_rq_clock(rq);
-		arg.dest_cpu = select_fallback_rq(cpu, p);
+		arg->dest_cpu = select_fallback_rq(cpu, p);
 		task_rq_unlock(rq, p, &rf);
 
 		stop_one_cpu_nowait(task_cpu(p), migration_cpu_stop,
-				    &arg, &work);
+				    arg, work);
 		__schedule(true);
-		if (!work.disabled) {
-			while (!arg.done)
-				cpu_relax();
-		}
 	}
 
 out:
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH RT 2/2] sched: migrate_enable: Remove __schedule() call
  2020-01-24 11:11 [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work Scott Wood
@ 2020-01-24 11:11 ` Scott Wood
  2020-01-24 15:38 ` [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work Steven Rostedt
  2020-02-03 17:37 ` Sebastian Andrzej Siewior
  2 siblings, 0 replies; 4+ messages in thread
From: Scott Wood @ 2020-01-24 11:11 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-rt-users, linux-kernel, Steven Rostedt, Thomas Gleixner,
	Scott Wood

We can rely on preempt_enable() to schedule.  Besides simplifying the
code, this potentially allows sequences such as the following to be
permitted:

migrate_disable();
preempt_disable();
migrate_enable();
preempt_enable();

Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Scott Wood <swood@redhat.com>
---
 kernel/sched/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7713e9c34ad1..ea0536461981 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8248,7 +8248,6 @@ void migrate_enable(void)
 
 		stop_one_cpu_nowait(task_cpu(p), migration_cpu_stop,
 				    arg, work);
-		__schedule(true);
 	}
 
 out:
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work
  2020-01-24 11:11 [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work Scott Wood
  2020-01-24 11:11 ` [PATCH RT 2/2] sched: migrate_enable: Remove __schedule() call Scott Wood
@ 2020-01-24 15:38 ` Steven Rostedt
  2020-02-03 17:37 ` Sebastian Andrzej Siewior
  2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2020-01-24 15:38 UTC (permalink / raw)
  To: Scott Wood
  Cc: Sebastian Andrzej Siewior, linux-rt-users, linux-kernel, Thomas Gleixner

On Fri, 24 Jan 2020 06:11:46 -0500
Scott Wood <swood@redhat.com> wrote:

> Commit e6c287b1512d ("sched: migrate_enable: Use stop_one_cpu_nowait()")
> adds a busy wait to deal with an edge case where the migrated thread
> can resume running on another CPU before the stopper has consumed
> cpu_stop_work.  However, this is done with preemption disabled and can
> potentially lead to deadlock.
> 
> While it is not guaranteed that the cpu_stop_work will be consumed before
> the migrating thread resumes and exits the stack frame, it is guaranteed
> that nothing other than the stopper can run on the old cpu between the
> migrating thread scheduling out and the cpu_stop_work being consumed.
> Thus, we can store cpu_stop_work in per-cpu data without it being
> reused too early.
> 

Makes sense to me.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work
  2020-01-24 11:11 [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work Scott Wood
  2020-01-24 11:11 ` [PATCH RT 2/2] sched: migrate_enable: Remove __schedule() call Scott Wood
  2020-01-24 15:38 ` [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work Steven Rostedt
@ 2020-02-03 17:37 ` Sebastian Andrzej Siewior
  2 siblings, 0 replies; 4+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-02-03 17:37 UTC (permalink / raw)
  To: Scott Wood; +Cc: linux-rt-users, linux-kernel, Steven Rostedt, Thomas Gleixner

On 2020-01-24 06:11:46 [-0500], Scott Wood wrote:
> Commit e6c287b1512d ("sched: migrate_enable: Use stop_one_cpu_nowait()")
> adds a busy wait to deal with an edge case where the migrated thread
> can resume running on another CPU before the stopper has consumed
> cpu_stop_work.  However, this is done with preemption disabled and can
> potentially lead to deadlock.
> 
> While it is not guaranteed that the cpu_stop_work will be consumed before
> the migrating thread resumes and exits the stack frame, it is guaranteed
> that nothing other than the stopper can run on the old cpu between the
> migrating thread scheduling out and the cpu_stop_work being consumed.
> Thus, we can store cpu_stop_work in per-cpu data without it being
> reused too early.
> 
> Fixes: e6c287b1512d ("sched: migrate_enable: Use stop_one_cpu_nowait()")
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Scott Wood <swood@redhat.com>

Yes, perfect, thank you.

Sebastian

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 11:11 [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work Scott Wood
2020-01-24 11:11 ` [PATCH RT 2/2] sched: migrate_enable: Remove __schedule() call Scott Wood
2020-01-24 15:38 ` [PATCH RT 1/2] sched: migrate_enable: Use per-cpu cpu_stop_work Steven Rostedt
2020-02-03 17:37 ` Sebastian Andrzej Siewior

Linux-rt-users archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rt-users/0 linux-rt-users/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rt-users linux-rt-users/ https://lore.kernel.org/linux-rt-users \
		linux-rt-users@vger.kernel.org
	public-inbox-index linux-rt-users

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rt-users


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git