* [PATCH] sched: Fix migration_cpu_stop() return value
@ 2014-06-04 10:41 Peter Zijlstra
2014-06-04 11:10 ` Lai Jiangshan
2014-06-04 11:18 ` Kirill Tkhai
0 siblings, 2 replies; 5+ messages in thread
From: Peter Zijlstra @ 2014-06-04 10:41 UTC (permalink / raw)
To: riel, laijs; +Cc: linux-kernel, mingo
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sched: Fix migration_cpu_stop() return value
2014-06-04 10:41 [PATCH] sched: Fix migration_cpu_stop() return value Peter Zijlstra
@ 2014-06-04 11:10 ` Lai Jiangshan
2014-06-04 11:18 ` Kirill Tkhai
1 sibling, 0 replies; 5+ messages in thread
From: Lai Jiangshan @ 2014-06-04 11:10 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: riel, linux-kernel, mingo
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sched: Fix migration_cpu_stop() return value
2014-06-04 10:41 [PATCH] sched: Fix migration_cpu_stop() return value Peter Zijlstra
2014-06-04 11:10 ` Lai Jiangshan
@ 2014-06-04 11:18 ` Kirill Tkhai
2014-06-04 12:03 ` Peter Zijlstra
1 sibling, 1 reply; 5+ messages in thread
From: Kirill Tkhai @ 2014-06-04 11:18 UTC (permalink / raw)
To: Peter Zijlstra, riel, laijs; +Cc: linux-kernel, mingo
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sched: Fix migration_cpu_stop() return value
2014-06-04 11:18 ` Kirill Tkhai
@ 2014-06-04 12:03 ` Peter Zijlstra
2014-06-04 12:29 ` Peter Zijlstra
0 siblings, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2014-06-04 12:03 UTC (permalink / raw)
To: Kirill Tkhai; +Cc: riel, laijs, linux-kernel, mingo
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sched: Fix migration_cpu_stop() return value
2014-06-04 12:03 ` Peter Zijlstra
@ 2014-06-04 12:29 ` Peter Zijlstra
0 siblings, 0 replies; 5+ messages in thread
From: Peter Zijlstra @ 2014-06-04 12:29 UTC (permalink / raw)
To: Kirill Tkhai; +Cc: riel, laijs, linux-kernel, mingo
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-06-04 12:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-04 10:41 [PATCH] sched: Fix migration_cpu_stop() return value Peter Zijlstra
2014-06-04 11:10 ` Lai Jiangshan
2014-06-04 11:18 ` Kirill Tkhai
2014-06-04 12:03 ` Peter Zijlstra
2014-06-04 12:29 ` Peter Zijlstra
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.