[PATCH] sched: Fix migration_cpu_stop() return value

[PATCH] sched: Fix migration_cpu_stop() return value

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]

A while ago I did a similar patch for some debugging, but looking at it
again today I realized we should probably fix this anyway.

---
Subject: sched: Fix migration_cpu_stop() return value

There are a number of migration_cpu_stop() users; and some actually care
about the success of the migration. So report this.

In particular migrate_task_to() as used from task_numa_migrate()
actually tests this return value.

Also change set_cpus_allowed_ptr() to propagate this return value, since
it already returns other errors.

Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/sched/core.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4633,11 +4633,13 @@ int set_cpus_allowed_ptr(struct task_str
 	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
 	if (p->on_rq) {
 		struct migration_arg arg = { p, dest_cpu };
+
 		/* Need help from migration thread: drop lock and wait. */
 		task_rq_unlock(rq, p, &flags);
-		stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
+		ret = stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
 		tlb_migrate_finish(p->mm);
-		return 0;
+
+		return ret;
 	}
 out:
 	task_rq_unlock(rq, p, &flags);
@@ -4747,19 +4749,24 @@ void sched_setnuma(struct task_struct *p
  * migration_cpu_stop - this will be executed by a highprio stopper thread
  * and performs thread migration by bumping thread off CPU then
  * 'pushing' onto another runqueue.
+ *
+ * Returns 0 on success, -EAGAIN on failure to migrate.
  */
 static int migration_cpu_stop(void *data)
 {
 	struct migration_arg *arg = data;
+	int ret = 0;
 
 	/*
 	 * The original target cpu might have gone down and we might
 	 * be on another cpu but it doesn't matter.
 	 */
 	local_irq_disable();
-	__migrate_task(arg->task, raw_smp_processor_id(), arg->dest_cpu);
+	if (!__migrate_task(arg->task, raw_smp_processor_id(), arg->dest_cpu))
+		ret = -EAGAIN;
 	local_irq_enable();
-	return 0;
+
+	return ret;
 }
 
 #ifdef CONFIG_HOTPLUG_CPU

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sched: Fix migration_cpu_stop() return value

[Lai Jiangshan]

On 06/04/2014 06:41 PM, Peter Zijlstra wrote:
> A while ago I did a similar patch for some debugging, but looking at it
> again today I realized we should probably fix this anyway.
> 
> ---
> Subject: sched: Fix migration_cpu_stop() return value
> 
> There are a number of migration_cpu_stop() users; and some actually care
> about the success of the migration. So report this.
> 
> In particular migrate_task_to() as used from task_numa_migrate()
> actually tests this return value.
> 
> Also change set_cpus_allowed_ptr() to propagate this return value, since
> it already returns other errors.
> 
> Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> ---
>  kernel/sched/core.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4633,11 +4633,13 @@ int set_cpus_allowed_ptr(struct task_str
>  	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
>  	if (p->on_rq) {
>  		struct migration_arg arg = { p, dest_cpu };
> +
>  		/* Need help from migration thread: drop lock and wait. */
>  		task_rq_unlock(rq, p, &flags);
> -		stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
> +		ret = stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
>  		tlb_migrate_finish(p->mm);
> -		return 0;
> +
> +		return ret;
>  	}
>  out:
>  	task_rq_unlock(rq, p, &flags);
> @@ -4747,19 +4749,24 @@ void sched_setnuma(struct task_struct *p
>   * migration_cpu_stop - this will be executed by a highprio stopper thread
>   * and performs thread migration by bumping thread off CPU then
>   * 'pushing' onto another runqueue.
> + *
> + * Returns 0 on success, -EAGAIN on failure to migrate.
>   */
>  static int migration_cpu_stop(void *data)
>  {
>  	struct migration_arg *arg = data;
> +	int ret = 0;
>  
>  	/*
>  	 * The original target cpu might have gone down and we might
>  	 * be on another cpu but it doesn't matter.
>  	 */
>  	local_irq_disable();
> -	__migrate_task(arg->task, raw_smp_processor_id(), arg->dest_cpu);
> +	if (!__migrate_task(arg->task, raw_smp_processor_id(), arg->dest_cpu))
> +		ret = -EAGAIN;


Current __migrate_task() returns 0 in these cases:
  !cpu_active(dest_cpu):
	hotplug subsystem's responsibility to migrate it

  !cpumask_test_cpu(dest_cpu, tsk_cpus_allowed(p)):
	newer set_cpus_allowed_ptr()'s responsibility

In these cases, current set_cpus_allowed_ptr() doesn't have the responsibility,
I don't think -EAGAIN is proper here since "-EAGAIN" asks the caller to do it again.


>  	local_irq_enable();
> -	return 0;
> +
> +	return ret;
>  }
>  
>  #ifdef CONFIG_HOTPLUG_CPU

Re: [PATCH] sched: Fix migration_cpu_stop() return value

[Kirill Tkhai]

Hi, Peter,

04.06.2014, 14:41, "Peter Zijlstra" <peterz@infradead.org>:
>  A while ago I did a similar patch for some debugging, but looking at it
>  again today I realized we should probably fix this anyway.
>
>  ---
>  Subject: sched: Fix migration_cpu_stop() return value
>
>  There are a number of migration_cpu_stop() users; and some actually care
>  about the success of the migration. So report this.
>
>  In particular migrate_task_to() as used from task_numa_migrate()
>  actually tests this return value.
>
>  Also change set_cpus_allowed_ptr() to propagate this return value, since
>  it already returns other errors.
>
>  Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
>  Cc: Ingo Molnar <mingo@redhat.com>
>  Signed-off-by: Peter Zijlstra <peterz@infradead.org>
>  ---
>   kernel/sched/core.c |   15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
(snipped everything because of bad email editor)

In set_cpus_allowed_ptr() p->on_rq branch can not fail.

We've changed affinity and released rq's lock, so task can migrate
on allowed cpu only (even if migration_cpu_stop fails).

And it's a little ambiguously how user should react on this EAGAIN.

Regards,
Kirill.

Re: [PATCH] sched: Fix migration_cpu_stop() return value

[Peter Zijlstra]

On Wed, Jun 04, 2014 at 03:18:45PM +0400, Kirill Tkhai wrote:
> Hi, Peter,
> 
> 04.06.2014, 14:41, "Peter Zijlstra" <peterz@infradead.org>:
> >  A while ago I did a similar patch for some debugging, but looking at it
> >  again today I realized we should probably fix this anyway.
> >
> >  ---
> >  Subject: sched: Fix migration_cpu_stop() return value
> >
> >  There are a number of migration_cpu_stop() users; and some actually care
> >  about the success of the migration. So report this.
> >
> >  In particular migrate_task_to() as used from task_numa_migrate()
> >  actually tests this return value.
> >
> >  Also change set_cpus_allowed_ptr() to propagate this return value, since
> >  it already returns other errors.
> >
> >  Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
> >  Cc: Ingo Molnar <mingo@redhat.com>
> >  Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> >  ---
> >   kernel/sched/core.c |   15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> (snipped everything because of bad email editor)
> 
> In set_cpus_allowed_ptr() p->on_rq branch can not fail.
> 
> We've changed affinity and released rq's lock, so task can migrate
> on allowed cpu only (even if migration_cpu_stop fails).
> 
> And it's a little ambiguously how user should react on this EAGAIN.

Try again? So one reason it might fail is because the task got migrated
in between the stop_cpu_call(migration_cpu_stop) call getting to
__migrate_task().

Esp. if you look at migrate_task_to() its fairly easy to fail this.
Currently it reports success, even though we completely failed to
migrate.

On -EAGAIN, re-evaluate the target and try again (later). Like for the
numa case, we'll try again on the next task_numa_migrate() call if its
still relevant.

Re: [PATCH] sched: Fix migration_cpu_stop() return value

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

On Wed, Jun 04, 2014 at 02:03:38PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 04, 2014 at 03:18:45PM +0400, Kirill Tkhai wrote:
> > Hi, Peter,
> > 
> > 04.06.2014, 14:41, "Peter Zijlstra" <peterz@infradead.org>:
> > >  A while ago I did a similar patch for some debugging, but looking at it
> > >  again today I realized we should probably fix this anyway.
> > >
> > >  ---
> > >  Subject: sched: Fix migration_cpu_stop() return value
> > >
> > >  There are a number of migration_cpu_stop() users; and some actually care
> > >  about the success of the migration. So report this.
> > >
> > >  In particular migrate_task_to() as used from task_numa_migrate()
> > >  actually tests this return value.
> > >
> > >  Also change set_cpus_allowed_ptr() to propagate this return value, since
> > >  it already returns other errors.
> > >
> > >  Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
> > >  Cc: Ingo Molnar <mingo@redhat.com>
> > >  Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> > >  ---
> > >   kernel/sched/core.c |   15 +++++++++++----
> > >   1 file changed, 11 insertions(+), 4 deletions(-)
> > (snipped everything because of bad email editor)
> > 
> > In set_cpus_allowed_ptr() p->on_rq branch can not fail.
> > 
> > We've changed affinity and released rq's lock, so task can migrate
> > on allowed cpu only (even if migration_cpu_stop fails).
> > 
> > And it's a little ambiguously how user should react on this EAGAIN.
> 
> Try again? So one reason it might fail is because the task got migrated
> in between the stop_cpu_call(migration_cpu_stop) call getting to
> __migrate_task().
> 
> Esp. if you look at migrate_task_to() its fairly easy to fail this.
> Currently it reports success, even though we completely failed to
> migrate.
> 
> On -EAGAIN, re-evaluate the target and try again (later). Like for the
> numa case, we'll try again on the next task_numa_migrate() call if its
> still relevant.

Argh, I just realized that the one case the numa code is interested in
is actually reported as success (task_cpu() != cpu).

I'll go make a new patch..

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]