Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1241242 > unrolled thread
| Started by | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| First post | 2015-10-07 10:50 +0200 |
| Last post | 2015-10-16 14:10 +0200 |
| Articles | 20 on this page of 31 — 4 participants |
Back to article view | Back to linux.kernel
[RFC][PATCH] sched: Start stopper early Peter Zijlstra <peterz@infradead.org> - 2015-10-07 10:50 +0200
Re: [RFC][PATCH] sched: Start stopper early Oleg Nesterov <oleg@redhat.com> - 2015-10-07 14:40 +0200
Re: [RFC][PATCH] sched: Start stopper early Peter Zijlstra <peterz@infradead.org> - 2015-10-07 14:40 +0200
Re: [RFC][PATCH] sched: Start stopper early Oleg Nesterov <oleg@redhat.com> - 2015-10-07 15:30 +0200
Re: [RFC][PATCH] sched: Start stopper early Oleg Nesterov <oleg@redhat.com> - 2015-10-07 15:30 +0200
Re: Re: [RFC][PATCH] sched: Start stopper early kbuild test robot <lkp@intel.com> - 2015-10-07 15:40 +0200
[PATCH 0/3] (Was: [RFC][PATCH] sched: Start stopper early) Oleg Nesterov <oleg@redhat.com> - 2015-10-08 17:00 +0200
[PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() Oleg Nesterov <oleg@redhat.com> - 2015-10-08 17:00 +0200
Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() Peter Zijlstra <peterz@infradead.org> - 2015-10-14 17:40 +0200
Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() Oleg Nesterov <oleg@redhat.com> - 2015-10-14 21:10 +0200
Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() Peter Zijlstra <peterz@infradead.org> - 2015-10-14 22:40 +0200
Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() Oleg Nesterov <oleg@redhat.com> - 2015-10-15 19:10 +0200
Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() Peter Zijlstra <peterz@infradead.org> - 2015-10-16 13:00 +0200
[PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled Oleg Nesterov <oleg@redhat.com> - 2015-10-08 17:00 +0200
Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled Peter Zijlstra <peterz@infradead.org> - 2015-10-08 17:10 +0200
Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled Oleg Nesterov <oleg@redhat.com> - 2015-10-08 18:10 +0200
Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled Oleg Nesterov <oleg@redhat.com> - 2015-10-08 18:20 +0200
[PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled Oleg Nesterov <oleg@redhat.com> - 2015-10-08 19:10 +0200
Re: [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled Peter Zijlstra <peterz@infradead.org> - 2015-10-09 18:40 +0200
Re: [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled Oleg Nesterov <oleg@redhat.com> - 2015-10-09 18:50 +0200
[PATCH 2/3] stop_machine: introduce __cpu_stop_queue_work() and cpu_stop_queue_two_works() Oleg Nesterov <oleg@redhat.com> - 2015-10-08 17:00 +0200
Re: [RFC][PATCH] sched: Start stopper early Oleg Nesterov <oleg@redhat.com> - 2015-10-08 20:10 +0200
Re: [RFC][PATCH] sched: Start stopper early Oleg Nesterov <oleg@redhat.com> - 2015-10-08 21:00 +0200
[PATCH 0/3] make stopper threads more "selfparking" Oleg Nesterov <oleg@redhat.com> - 2015-10-09 18:10 +0200
[PATCH 2/3] stop_machine: kill cpu_stop_threads->setup() and cpu_stop_unpark() Oleg Nesterov <oleg@redhat.com> - 2015-10-09 18:10 +0200
[PATCH 3/3] sched: start stopper early Oleg Nesterov <oleg@redhat.com> - 2015-10-09 18:10 +0200
Re: [PATCH 3/3] sched: start stopper early Oleg Nesterov <oleg@redhat.com> - 2015-10-09 19:00 +0200
[PATCH 1/3] stop_machine: kill smp_hotplug_thread->pre_unpark, introduce stop_machine_unpark() Oleg Nesterov <oleg@redhat.com> - 2015-10-09 18:10 +0200
Re: [RFC][PATCH] sched: Start stopper early Heiko Carstens <heiko.carstens@de.ibm.com> - 2015-10-16 10:30 +0200
Re: [RFC][PATCH] sched: Start stopper early Peter Zijlstra <peterz@infradead.org> - 2015-10-16 12:00 +0200
Re: [RFC][PATCH] sched: Start stopper early Heiko Carstens <heiko.carstens@de.ibm.com> - 2015-10-16 14:10 +0200
Page 1 of 2 [1] 2 Next page →
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-10-07 10:50 +0200 |
| Subject | [RFC][PATCH] sched: Start stopper early |
| Message-ID | <qgSAa-86i-35@gated-at.bofh.it> |
Hi,
So Heiko reported some 'interesting' fail where stop_two_cpus() got
stuck in multi_cpu_stop() with one cpu waiting for another that never
happens.
It _looks_ like the 'other' cpu isn't running and the current best
theory is that we race on cpu-up and get the stop_two_cpus() call in
before the stopper task is running.
This _is_ possible because we set 'online && active' _before_ we do the
smpboot_unpark thing because of ONLINE notifier order.
The below test patch manually starts the stopper task early.
It boots and hotplugs a cpu on my test box so its not insta broken.
---
kernel/sched/core.c | 7 ++++++-
kernel/stop_machine.c | 5 +++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 1764a0f..9a56ef7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5542,14 +5542,19 @@ static void set_cpu_rq_start_time(void)
rq->age_stamp = sched_clock_cpu(cpu);
}
+extern void cpu_stopper_unpark(unsigned int cpu);
+
static int sched_cpu_active(struct notifier_block *nfb,
unsigned long action, void *hcpu)
{
+ int cpu = (long)hcpu;
+
switch (action & ~CPU_TASKS_FROZEN) {
case CPU_STARTING:
set_cpu_rq_start_time();
return NOTIFY_OK;
case CPU_ONLINE:
+ cpu_stopper_unpark(cpu);
/*
* At this point a starting CPU has marked itself as online via
* set_cpu_online(). But it might not yet have marked itself
@@ -5558,7 +5563,7 @@ static int sched_cpu_active(struct notifier_block *nfb,
* Thus, fall-through and help the starting CPU along.
*/
case CPU_DOWN_FAILED:
- set_cpu_active((long)hcpu, true);
+ set_cpu_active(cpu, true);
return NOTIFY_OK;
default:
return NOTIFY_DONE;
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 12484e5..c674371 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -496,6 +496,11 @@ static struct smp_hotplug_thread cpu_stop_threads = {
.selfparking = true,
};
+void cpu_stopper_unpark(unsigned int cpu)
+{
+ kthread_unpark(per_cpu(cpu_stopper.thread, cpu));
+}
+
static int __init cpu_stop_init(void)
{
unsigned int cpu;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-07 14:40 +0200 |
| Message-ID | <qgWaK-4Te-7@gated-at.bofh.it> |
| In reply to | #1241242 |
On 10/07, Peter Zijlstra wrote: > > So Heiko reported some 'interesting' fail where stop_two_cpus() got > stuck in multi_cpu_stop() with one cpu waiting for another that never > happens. > > It _looks_ like the 'other' cpu isn't running and the current best > theory is that we race on cpu-up and get the stop_two_cpus() call in > before the stopper task is running. > > This _is_ possible because we set 'online && active' Argh. Can't really comment this change right now, but this reminds me that stop_two_cpus() path should not rely on cpu_active() at all. I mean we should not use this check to avoid the deadlock, migrate_swap_stop() can check it itself. And cpu_stop_park()->cpu_stop_signal_done() should be replaced by BUG_ON(). Probably slightly off-topic, but what do you finally think about the old "[PATCH v2 6/6] stop_machine: kill stop_cpus_lock and lg_double_lock/unlock()" we discussed in http://marc.info/?t=143750670300014 ? I won't really insist if you still dislike it, but it seems we both agree that "lg_lock stop_cpus_lock" must die in any case, and after that we can the cleanups mentioned above. And, Peter, I see a lot of interesting emails from you, but currently can't even read them. I hope very much I will read them later and perhaps even reply ;) Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-10-07 14:40 +0200 |
| Message-ID | <qgWaK-4Te-9@gated-at.bofh.it> |
| In reply to | #1241445 |
On Wed, Oct 07, 2015 at 02:30:46PM +0200, Oleg Nesterov wrote: > On 10/07, Peter Zijlstra wrote: > > > > So Heiko reported some 'interesting' fail where stop_two_cpus() got > > stuck in multi_cpu_stop() with one cpu waiting for another that never > > happens. > > > > It _looks_ like the 'other' cpu isn't running and the current best > > theory is that we race on cpu-up and get the stop_two_cpus() call in > > before the stopper task is running. > > > > This _is_ possible because we set 'online && active' > > Argh. Can't really comment this change right now, but this reminds me > that stop_two_cpus() path should not rely on cpu_active() at all. I mean > we should not use this check to avoid the deadlock, migrate_swap_stop() > can check it itself. And cpu_stop_park()->cpu_stop_signal_done() should > be replaced by BUG_ON(). > > Probably slightly off-topic, but what do you finally think about the old > "[PATCH v2 6/6] stop_machine: kill stop_cpus_lock and lg_double_lock/unlock()" > we discussed in http://marc.info/?t=143750670300014 ? > > I won't really insist if you still dislike it, but it seems we both > agree that "lg_lock stop_cpus_lock" must die in any case, and after that > we can the cleanups mentioned above. Yes, I was looking at that, this issue reminded me we still had that issue open. > And, Peter, I see a lot of interesting emails from you, but currently > can't even read them. I hope very much I will read them later and perhaps > even reply ;) Sure, take your time. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-07 15:30 +0200 |
| Message-ID | <qgWX9-65g-29@gated-at.bofh.it> |
| In reply to | #1241446 |
On 10/07, Peter Zijlstra wrote:
>
> On Wed, Oct 07, 2015 at 02:30:46PM +0200, Oleg Nesterov wrote:
> > On 10/07, Peter Zijlstra wrote:
> > >
> > > So Heiko reported some 'interesting' fail where stop_two_cpus() got
> > > stuck in multi_cpu_stop() with one cpu waiting for another that never
> > > happens.
> > >
> > > It _looks_ like the 'other' cpu isn't running and the current best
> > > theory is that we race on cpu-up and get the stop_two_cpus() call in
> > > before the stopper task is running.
> > >
> > > This _is_ possible because we set 'online && active'
> >
> > Argh. Can't really comment this change right now, but this reminds me
> > that stop_two_cpus() path should not rely on cpu_active() at all. I mean
> > we should not use this check to avoid the deadlock, migrate_swap_stop()
> > can check it itself. And cpu_stop_park()->cpu_stop_signal_done() should
> > be replaced by BUG_ON().
> >
> > Probably slightly off-topic, but what do you finally think about the old
> > "[PATCH v2 6/6] stop_machine: kill stop_cpus_lock and lg_double_lock/unlock()"
> > we discussed in http://marc.info/?t=143750670300014 ?
> >
> > I won't really insist if you still dislike it, but it seems we both
> > agree that "lg_lock stop_cpus_lock" must die in any case, and after that
> > we can the cleanups mentioned above.
>
> Yes, I was looking at that, this issue reminded me we still had that
> issue open.
Great, thanks!
But let me add that I tried to confuse you because I forgot what actually
I was going to do... I meant something like the (incomplete) patch below,
and after that we can change stop_two_cpus() to rely on ->enabled and
remove the cpu_active() checks (again, ignoring the fact we do not want
to migrate to inactive CPU). Although I need to recall/recheck this all,
perhaps I missed something...
So while I think we should kill lg_lock in any case, this and the patch
above is absolutely off-topic, we can do this with or without lg_lock
removal.
Oleg.
--- x/kernel/cpu.c
+++ x/kernel/cpu.c
@@ -344,7 +344,7 @@ static int take_cpu_down(void *_param)
/* Give up timekeeping duties */
tick_handover_do_timer();
/* Park the stopper thread */
- kthread_park(current);
+ stop_machine_park(param->hcpu);
return 0;
}
--- x/kernel/stop_machine.c
+++ x/kernel/stop_machine.c
@@ -452,6 +452,15 @@ repeat:
}
}
+void stop_machine_park(int cpu)
+{
+ struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
+
+ spin_lock(&stopper->lock);
+ stopper->enabled = false;
+ spin_unlock(&stopper->lock);
+}
+
extern void sched_set_stop_task(int cpu, struct task_struct *stop);
static void cpu_stop_create(unsigned int cpu)
@@ -468,10 +477,10 @@ static void cpu_stop_park(unsigned int c
/* drain remaining works */
spin_lock_irqsave(&stopper->lock, flags);
list_for_each_entry_safe(work, tmp, &stopper->works, list) {
+ WARN_ON(1);
list_del_init(&work->list);
cpu_stop_signal_done(work->done, false);
}
- stopper->enabled = false;
spin_unlock_irqrestore(&stopper->lock, flags);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-07 15:30 +0200 |
| Message-ID | <qgWX9-65g-43@gated-at.bofh.it> |
| In reply to | #1241487 |
Damn sorry for noise ;)
On 10/07, Oleg Nesterov wrote:
>
> +void stop_machine_park(int cpu)
> +{
> + struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
> +
> + spin_lock(&stopper->lock);
> + stopper->enabled = false;
> + spin_unlock(&stopper->lock);
Of course, it should also do kthread_park(current).
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2015-10-07 15:40 +0200 |
| Message-ID | <qgX6N-6gd-11@gated-at.bofh.it> |
| In reply to | #1241487 |
[Multipart message — attachments visible in raw view] — view raw
Hi Oleg,
[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]
config: x86_64-randconfig-x019-201540 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
kernel/cpu.c: In function 'take_cpu_down':
>> kernel/cpu.c:347:2: error: implicit declaration of function 'stop_machine_park' [-Werror=implicit-function-declaration]
stop_machine_park(param->hcpu);
^
cc1: some warnings being treated as errors
vim +/stop_machine_park +347 kernel/cpu.c
341 return err;
342
343 cpu_notify(CPU_DYING | param->mod, param->hcpu);
344 /* Give up timekeeping duties */
345 tick_handover_do_timer();
346 /* Park the stopper thread */
> 347 stop_machine_park(param->hcpu);
348 return 0;
349 }
350
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-08 17:00 +0200 |
| Subject | [PATCH 0/3] (Was: [RFC][PATCH] sched: Start stopper early) |
| Message-ID | <qhkPM-6FW-23@gated-at.bofh.it> |
| In reply to | #1241242 |
On 10/07, Peter Zijlstra wrote:
>
> So Heiko reported some 'interesting' fail where stop_two_cpus() got
> stuck in multi_cpu_stop() with one cpu waiting for another that never
> happens.
>
> It _looks_ like the 'other' cpu isn't running and the current best
> theory is that we race on cpu-up and get the stop_two_cpus() call in
> before the stopper task is running.
How about this series? Slightly tested.
Note:
- To me this also looks like a preparation for lg lock removal, no
matter how exactly we will do this.
- We can do more cleanups on top of this. Say remove preempt_disable
in stop_two_cpus().
Oleg.
include/linux/stop_machine.h | 1
kernel/cpu.c | 2 -
kernel/stop_machine.c | 83 +++++++++++++++++++++++++++++--------------
3 files changed, 58 insertions(+), 28 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-08 17:00 +0200 |
| Subject | [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() |
| Message-ID | <qhkPM-6FW-35@gated-at.bofh.it> |
| In reply to | #1242471 |
cpu_stop_queue_work() checks stopper->enabled before it queues the
work, but ->enabled == T can only guarantee cpu_stop_signal_done()
if we race with cpu_down().
This is not enough for stop_two_cpus() or stop_machine(), they will
deadlock if multi_cpu_stop() won't be called by one of the target
CPU's. stop_machine/stop_cpus are fine, they rely on stop_cpus_mutex.
But stop_two_cpus() has to check cpu_active() to avoid the same race
with hotplug, and this check is very unobvious and probably not even
correct if we race with cpu_up().
Change cpu_down() pass to clear ->enabled before cpu_stopper_thread()
flushes the pending ->works and returns with KTHREAD_SHOULD_PARK set.
Note also that smpboot_thread_call() calls cpu_stop_unpark() which
sets enabled == T at CPU_ONLINE stage, so this CPU can't go away until
cpu_stopper_thread() is called at least once. This all means that if
cpu_stop_queue_work() succeeds, we know that work->fn() will be called.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
include/linux/stop_machine.h | 1 +
kernel/cpu.c | 2 +-
kernel/stop_machine.c | 23 +++++++++++++----------
3 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
index 414d924..7b76362 100644
--- a/include/linux/stop_machine.h
+++ b/include/linux/stop_machine.h
@@ -33,6 +33,7 @@ void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
struct cpu_stop_work *work_buf);
int stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);
int try_stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);
+void stop_machine_park(int cpu);
#else /* CONFIG_SMP */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 1f4566c..8a7225a 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -342,7 +342,7 @@ static int __ref take_cpu_down(void *_param)
/* Give up timekeeping duties */
tick_handover_do_timer();
/* Park the stopper thread */
- kthread_park(current);
+ stop_machine_park((long)param->hcpu);
return 0;
}
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 12484e5..6a40209 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -452,6 +452,18 @@ repeat:
}
}
+void stop_machine_park(int cpu)
+{
+ struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
+ /*
+ * Lockless. cpu_stopper_thread() will take stopper->lock and flush
+ * the pending works before it parks, until then it is fine to queue
+ * the new works.
+ */
+ stopper->enabled = false;
+ kthread_park(stopper->thread);
+}
+
extern void sched_set_stop_task(int cpu, struct task_struct *stop);
static void cpu_stop_create(unsigned int cpu)
@@ -462,17 +474,8 @@ static void cpu_stop_create(unsigned int cpu)
static void cpu_stop_park(unsigned int cpu)
{
struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
- struct cpu_stop_work *work, *tmp;
- unsigned long flags;
- /* drain remaining works */
- spin_lock_irqsave(&stopper->lock, flags);
- list_for_each_entry_safe(work, tmp, &stopper->works, list) {
- list_del_init(&work->list);
- cpu_stop_signal_done(work->done, false);
- }
- stopper->enabled = false;
- spin_unlock_irqrestore(&stopper->lock, flags);
+ WARN_ON(!list_empty(&stopper->works));
}
static void cpu_stop_unpark(unsigned int cpu)
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-10-14 17:40 +0200 |
| Subject | Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() |
| Message-ID | <qjwjN-2JS-53@gated-at.bofh.it> |
| In reply to | #1242479 |
On Thu, Oct 08, 2015 at 04:51:31PM +0200, Oleg Nesterov wrote: > cpu_stop_queue_work() checks stopper->enabled before it queues the > work, but ->enabled == T can only guarantee cpu_stop_signal_done() > if we race with cpu_down(). > > This is not enough for stop_two_cpus() or stop_machine(), they will > deadlock if multi_cpu_stop() won't be called by one of the target > CPU's. stop_machine/stop_cpus are fine, they rely on stop_cpus_mutex. > But stop_two_cpus() has to check cpu_active() to avoid the same race > with hotplug, and this check is very unobvious and probably not even > correct if we race with cpu_up(). > > Change cpu_down() pass to clear ->enabled before cpu_stopper_thread() > flushes the pending ->works and returns with KTHREAD_SHOULD_PARK set. > > Note also that smpboot_thread_call() calls cpu_stop_unpark() which > sets enabled == T at CPU_ONLINE stage, so this CPU can't go away until > cpu_stopper_thread() is called at least once. This all means that if > cpu_stop_queue_work() succeeds, we know that work->fn() will be called. This hard relies on the fact that cpu_down uses stop machine, right? IIRC part of the hotplug rework Thomas is doing is geared towards breaking away from stop machine. There is nothing fundamental about hot-unplug that requires stop machine. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-14 21:10 +0200 |
| Subject | Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() |
| Message-ID | <qjzB0-7Ge-17@gated-at.bofh.it> |
| In reply to | #1246914 |
On 10/14, Peter Zijlstra wrote: > > On Thu, Oct 08, 2015 at 04:51:31PM +0200, Oleg Nesterov wrote: > > cpu_stop_queue_work() checks stopper->enabled before it queues the > > work, but ->enabled == T can only guarantee cpu_stop_signal_done() > > if we race with cpu_down(). > > > > This is not enough for stop_two_cpus() or stop_machine(), they will > > deadlock if multi_cpu_stop() won't be called by one of the target > > CPU's. stop_machine/stop_cpus are fine, they rely on stop_cpus_mutex. > > But stop_two_cpus() has to check cpu_active() to avoid the same race > > with hotplug, and this check is very unobvious and probably not even > > correct if we race with cpu_up(). > > > > Change cpu_down() pass to clear ->enabled before cpu_stopper_thread() > > flushes the pending ->works and returns with KTHREAD_SHOULD_PARK set. > > > > Note also that smpboot_thread_call() calls cpu_stop_unpark() which > > sets enabled == T at CPU_ONLINE stage, so this CPU can't go away until > > cpu_stopper_thread() is called at least once. This all means that if > > cpu_stop_queue_work() succeeds, we know that work->fn() will be called. > > This hard relies on the fact that cpu_down uses stop machine, right? Not really. > IIRC part of the hotplug rework Thomas is doing is geared towards > breaking away from stop machine. There is nothing fundamental about > hot-unplug that requires stop machine. cpu_down() should park/kill/whatever the percpu stopper thread anyway. And this path should clear ->enabled, it can also flush the pending works. And we need this anyway even if cpu_down() won't use stop_machine(), I think. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-10-14 22:40 +0200 |
| Subject | Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() |
| Message-ID | <qjB06-1bR-5@gated-at.bofh.it> |
| In reply to | #1247087 |
On Wed, Oct 14, 2015 at 09:03:56PM +0200, Oleg Nesterov wrote: > On 10/14, Peter Zijlstra wrote: > > > > On Thu, Oct 08, 2015 at 04:51:31PM +0200, Oleg Nesterov wrote: > > > cpu_stop_queue_work() checks stopper->enabled before it queues the > > > work, but ->enabled == T can only guarantee cpu_stop_signal_done() > > > if we race with cpu_down(). > > > > > > This is not enough for stop_two_cpus() or stop_machine(), they will > > > deadlock if multi_cpu_stop() won't be called by one of the target > > > CPU's. stop_machine/stop_cpus are fine, they rely on stop_cpus_mutex. > > > But stop_two_cpus() has to check cpu_active() to avoid the same race > > > with hotplug, and this check is very unobvious and probably not even > > > correct if we race with cpu_up(). > > > > > > Change cpu_down() pass to clear ->enabled before cpu_stopper_thread() > > > flushes the pending ->works and returns with KTHREAD_SHOULD_PARK set. > > > > > > Note also that smpboot_thread_call() calls cpu_stop_unpark() which > > > sets enabled == T at CPU_ONLINE stage, so this CPU can't go away until > > > cpu_stopper_thread() is called at least once. This all means that if > > > cpu_stop_queue_work() succeeds, we know that work->fn() will be called. > > > > This hard relies on the fact that cpu_down uses stop machine, right? > > Not really. > > > IIRC part of the hotplug rework Thomas is doing is geared towards > > breaking away from stop machine. There is nothing fundamental about > > hot-unplug that requires stop machine. > > cpu_down() should park/kill/whatever the percpu stopper thread anyway. > And this path should clear ->enabled, it can also flush the pending > works. So the proposed patch does: ->enabled=false; park();, which can race with if (->enabled) wake(); smpboot_thread_fn() will not call ->thread_fn() when should_park(), and thus any pending work will not get flushed. It only works now because the stopper task calls park(), which means cpu_stopper_thread() will flush, but that very much relies on the stopper thread calling park in itself. Or I'm just terminally confused.. which is entirely possible. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-15 19:10 +0200 |
| Subject | Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() |
| Message-ID | <qjUcq-4l6-35@gated-at.bofh.it> |
| In reply to | #1247139 |
On 10/14, Peter Zijlstra wrote:
>
> On Wed, Oct 14, 2015 at 09:03:56PM +0200, Oleg Nesterov wrote:
> > On 10/14, Peter Zijlstra wrote:
> > >
> > > On Thu, Oct 08, 2015 at 04:51:31PM +0200, Oleg Nesterov wrote:
> > > >
> > > > Note also that smpboot_thread_call() calls cpu_stop_unpark() which
> > > > sets enabled == T at CPU_ONLINE stage, so this CPU can't go away until
> > > > cpu_stopper_thread() is called at least once. This all means that if
> > > > cpu_stop_queue_work() succeeds, we know that work->fn() will be called.
> > >
> > > This hard relies on the fact that cpu_down uses stop machine, right?
> >
> > Not really.
> >
> > > IIRC part of the hotplug rework Thomas is doing is geared towards
> > > breaking away from stop machine. There is nothing fundamental about
> > > hot-unplug that requires stop machine.
> >
> > cpu_down() should park/kill/whatever the percpu stopper thread anyway.
> > And this path should clear ->enabled, it can also flush the pending
> > works.
>
> So the proposed patch does: ->enabled=false; park();, which can race
> with if (->enabled) wake();
Yes, so I added the comment to explain that this is fine.
> smpboot_thread_fn() will not call ->thread_fn() when should_park(), and
> thus any pending work will not get flushed.
>
> It only works now because the stopper task calls park(), which means
> cpu_stopper_thread() will flush, but that very much relies on the
> stopper thread calling park in itself.
Yes. IOW, this relies on ->selfparking == T which implies "flush before
park".
But even if we change cpu_down() to avoid stop_machine() I think we need
to keep this "selfparking" logic. In a sense that, for example, this code
void func(int cpu, cpu_stop_fn_t fn)
{
get_online_cpus();
if (cpu_online(cpu) {
int ret = stop_one_cpu(cpu, fn, NULL);
BUG_ON(ret == -ENOENT);
}
put_online_cpus();
}
should be correct. Actually this example is not very good, it would
be better to use stop_one_cpu_nowait() but currently it returns "void"
and hmm, it looks buggy ;) I'll send the fix on top of this series if
you accept it.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-10-16 13:00 +0200 |
| Subject | Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() |
| Message-ID | <qkaTV-3YR-49@gated-at.bofh.it> |
| In reply to | #1248016 |
On Thu, Oct 15, 2015 at 07:02:47PM +0200, Oleg Nesterov wrote: > Yes. IOW, this relies on ->selfparking == T which implies "flush before > park". Ah, we have a different reading of 'selfparking'. OK. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-08 17:00 +0200 |
| Subject | [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled |
| Message-ID | <qhkPO-6FW-61@gated-at.bofh.it> |
| In reply to | #1242471 |
Change cpu_stop_queue_two_works() to ensure that both CPU's have
stopper->enabled == T or fail otherwise.
This way stop_two_cpus() no longer needs to check cpu_active() to
avoid the deadlock. This patch doesn't remove these checks, we will
do this later.
Note: we need to take both stopper->lock's at the same time, but this
will also help to remove lglock from stop_machine.c, so I hope this
is fine.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/stop_machine.c | 29 ++++++++++++++++++++---------
1 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 688d6b3..6d85d27 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -219,12 +219,27 @@ static int multi_cpu_stop(void *data)
static int cpu_stop_queue_two_works(int cpu1, struct cpu_stop_work *work1,
int cpu2, struct cpu_stop_work *work2)
{
+ struct cpu_stopper *stopper1 = per_cpu_ptr(&cpu_stopper, cpu1);
+ struct cpu_stopper *stopper2 = per_cpu_ptr(&cpu_stopper, cpu2);
+ int err;
+
lg_double_lock(&stop_cpus_lock, cpu1, cpu2);
- cpu_stop_queue_work(cpu1, work1);
- cpu_stop_queue_work(cpu2, work2);
+ spin_lock_irq(&stopper1->lock);
+ spin_lock_nested(&stopper2->lock, SINGLE_DEPTH_NESTING);
+
+ err = -ENOENT;
+ if (!stopper1->enabled || !stopper2->enabled)
+ goto unlock;
+
+ err = 0;
+ __cpu_stop_queue_work(stopper1, work1);
+ __cpu_stop_queue_work(stopper2, work2);
+unlock:
+ spin_unlock(&stopper2->lock);
+ spin_unlock_irq(&stopper1->lock);
lg_double_unlock(&stop_cpus_lock, cpu1, cpu2);
- return 0;
+ return err;
}
/**
* stop_two_cpus - stops two cpus
@@ -261,12 +276,8 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
set_state(&msdata, MULTI_STOP_PREPARE);
/*
- * If we observe both CPUs active we know _cpu_down() cannot yet have
- * queued its stop_machine works and therefore ours will get executed
- * first. Or its not either one of our CPUs that's getting unplugged,
- * in which case we don't care.
- *
- * This relies on the stopper workqueues to be FIFO.
+ * We do not want to migrate to inactive CPU. FIXME: move this
+ * into the caller.
*/
if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
preempt_enable();
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-10-08 17:10 +0200 |
| Subject | Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled |
| Message-ID | <qhkZs-76Q-31@gated-at.bofh.it> |
| In reply to | #1242490 |
On Thu, Oct 08, 2015 at 04:51:36PM +0200, Oleg Nesterov wrote:
> @@ -261,12 +276,8 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
> set_state(&msdata, MULTI_STOP_PREPARE);
>
> /*
> + * We do not want to migrate to inactive CPU. FIXME: move this
> + * into the caller.
> */
> if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
> preempt_enable();
So we cannot move that into the caller.. because this function sleeps
with wait_for_completion().
Or rather, it would force the caller to use get_online_cpus(), which we
worked really hard to avoid.
Also, I think we still want the patch I proposed which ensures the
stopper thread is active 'early', because the load balancer pretty much
assumes that its available. And when 'online && active' the
load-balancer is fully available.
Not only the numa balancing stop_two_cpus() caller relies on it, but
also the self migration stuff does, and at CPU_ONLINE time the cpu
really is 'free' to run anything.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-08 18:10 +0200 |
| Subject | Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled |
| Message-ID | <qhlVw-8s5-37@gated-at.bofh.it> |
| In reply to | #1242504 |
On 10/08, Peter Zijlstra wrote:
>
> On Thu, Oct 08, 2015 at 04:51:36PM +0200, Oleg Nesterov wrote:
> > @@ -261,12 +276,8 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
> > set_state(&msdata, MULTI_STOP_PREPARE);
> >
> > /*
> > + * We do not want to migrate to inactive CPU. FIXME: move this
> > + * into the caller.
> > */
> > if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
> > preempt_enable();
>
> So we cannot move that into the caller..
Why?
> because this function sleeps
> with wait_for_completion().
>
> Or rather, it would force the caller to use get_online_cpus(), which we
> worked really hard to avoid.
Aaah wait. Sorry for confusion!
I meant "move this into the callback, migrate_swap_stop()".
> Also, I think we still want the patch I proposed which ensures the
> stopper thread is active 'early', because the load balancer pretty much
Perhaps. Although I do not really understand why it is important.
I mean, either way we unpark it at CPU_ONLINE stage, just
sched_cpu_active() has a higher priority.
But this is off-topic in a sense that the main point of this patch
is that stop_two_cpus() no longer needs to abuse cpu_active() checks
to avoid the race with cpu_up/down, we can simply rely on ->enabled.
And again, we need to take both locks to remove "lglock stop_cpus_lock".
So I think your change can be applied after this series too. Or I missed
something?
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-08 18:20 +0200 |
| Subject | Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled |
| Message-ID | <qhm5c-bp-7@gated-at.bofh.it> |
| In reply to | #1242582 |
On 10/08, Oleg Nesterov wrote:
>
> On 10/08, Peter Zijlstra wrote:
> >
> > On Thu, Oct 08, 2015 at 04:51:36PM +0200, Oleg Nesterov wrote:
> > > @@ -261,12 +276,8 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
> > > set_state(&msdata, MULTI_STOP_PREPARE);
> > >
> > > /*
> > > + * We do not want to migrate to inactive CPU. FIXME: move this
> > > + * into the caller.
> > > */
> > > if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
> > > preempt_enable();
> >
> > So we cannot move that into the caller..
>
> Why?
>
> > because this function sleeps
> > with wait_for_completion().
> >
> > Or rather, it would force the caller to use get_online_cpus(), which we
> > worked really hard to avoid.
>
> Aaah wait. Sorry for confusion!
>
> I meant "move this into the callback, migrate_swap_stop()".
Forgot to mention... And note that both these cpu_active() are obviously
racy, CPU_DOWN_PREPARE can make it inactive right after the check.
> > Also, I think we still want the patch I proposed which ensures the
> > stopper thread is active 'early', because the load balancer pretty much
>
> Perhaps. Although I do not really understand why it is important.
> I mean, either way we unpark it at CPU_ONLINE stage, just
> sched_cpu_active() has a higher priority.
>
> But this is off-topic in a sense that the main point of this patch
> is that stop_two_cpus() no longer needs to abuse cpu_active() checks
> to avoid the race with cpu_up/down, we can simply rely on ->enabled.
>
> And again, we need to take both locks to remove "lglock stop_cpus_lock".
>
> So I think your change can be applied after this series too. Or I missed
> something?
So I think this series makes sense anyway and hopefully should fix the
problem with or without your change.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-08 19:10 +0200 |
| Subject | [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled |
| Message-ID | <qhmRA-1ld-7@gated-at.bofh.it> |
| In reply to | #1242490 |
On 10/08, Oleg Nesterov wrote:
>
> + * We do not want to migrate to inactive CPU. FIXME: move this
> + * into the caller.
> */
> if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
> preempt_enable();
Of course, this comment is indeed wrong, thanks. Please see V2.
-------------------------------------------------------------------------------
From 41d6d14e318335212ffac093c09de0b197235b90 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Thu, 8 Oct 2015 16:22:06 +0200
Subject: [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled
Change cpu_stop_queue_two_works() to ensure that both CPU's have
stopper->enabled == T or fail otherwise.
This way stop_two_cpus() no longer needs to check cpu_active() to
avoid the deadlock. This patch doesn't remove these checks, we will
do this later.
Note: we need to take both stopper->lock's at the same time, but this
will also help to remove lglock from stop_machine.c, so I hope this
is fine.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/stop_machine.c | 29 ++++++++++++++++++++---------
1 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 688d6b3..91fbb10 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -219,12 +219,27 @@ static int multi_cpu_stop(void *data)
static int cpu_stop_queue_two_works(int cpu1, struct cpu_stop_work *work1,
int cpu2, struct cpu_stop_work *work2)
{
+ struct cpu_stopper *stopper1 = per_cpu_ptr(&cpu_stopper, cpu1);
+ struct cpu_stopper *stopper2 = per_cpu_ptr(&cpu_stopper, cpu2);
+ int err;
+
lg_double_lock(&stop_cpus_lock, cpu1, cpu2);
- cpu_stop_queue_work(cpu1, work1);
- cpu_stop_queue_work(cpu2, work2);
+ spin_lock_irq(&stopper1->lock);
+ spin_lock_nested(&stopper2->lock, SINGLE_DEPTH_NESTING);
+
+ err = -ENOENT;
+ if (!stopper1->enabled || !stopper2->enabled)
+ goto unlock;
+
+ err = 0;
+ __cpu_stop_queue_work(stopper1, work1);
+ __cpu_stop_queue_work(stopper2, work2);
+unlock:
+ spin_unlock(&stopper2->lock);
+ spin_unlock_irq(&stopper1->lock);
lg_double_unlock(&stop_cpus_lock, cpu1, cpu2);
- return 0;
+ return err;
}
/**
* stop_two_cpus - stops two cpus
@@ -261,12 +276,8 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
set_state(&msdata, MULTI_STOP_PREPARE);
/*
- * If we observe both CPUs active we know _cpu_down() cannot yet have
- * queued its stop_machine works and therefore ours will get executed
- * first. Or its not either one of our CPUs that's getting unplugged,
- * in which case we don't care.
- *
- * This relies on the stopper workqueues to be FIFO.
+ * We do not want to migrate to inactive CPU. FIXME: move this
+ * into migrate_swap_stop() callback.
*/
if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
preempt_enable();
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-10-09 18:40 +0200 |
| Subject | Re: [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled |
| Message-ID | <qhIS7-7BS-39@gated-at.bofh.it> |
| In reply to | #1242616 |
On Thu, Oct 08, 2015 at 07:01:41PM +0200, Oleg Nesterov wrote:
> @@ -261,12 +276,8 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
> set_state(&msdata, MULTI_STOP_PREPARE);
>
> /*
> - * If we observe both CPUs active we know _cpu_down() cannot yet have
> - * queued its stop_machine works and therefore ours will get executed
> - * first. Or its not either one of our CPUs that's getting unplugged,
> - * in which case we don't care.
> - *
> - * This relies on the stopper workqueues to be FIFO.
> + * We do not want to migrate to inactive CPU. FIXME: move this
> + * into migrate_swap_stop() callback.
> */
> if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
> preempt_enable();
I stuck that on top.. I'll have a closer look at the 7 patches later
when I might be more coherent (mad head-ache atm.)
---
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1335,12 +1335,16 @@ static int migrate_swap_stop(void *data)
struct rq *src_rq, *dst_rq;
int ret = -EAGAIN;
+ if (!cpu_active(arg->src_cpu) || !cpu_active(arg->dst_cpu))
+ return -EAGAIN;
+
src_rq = cpu_rq(arg->src_cpu);
dst_rq = cpu_rq(arg->dst_cpu);
double_raw_lock(&arg->src_task->pi_lock,
&arg->dst_task->pi_lock);
double_rq_lock(src_rq, dst_rq);
+
if (task_cpu(arg->dst_task) != arg->dst_cpu)
goto unlock;
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -275,15 +275,6 @@ int stop_two_cpus(unsigned int cpu1, uns
cpu_stop_init_done(&done, 2);
set_state(&msdata, MULTI_STOP_PREPARE);
- /*
- * We do not want to migrate to inactive CPU. FIXME: move this
- * into migrate_swap_stop() callback.
- */
- if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
- preempt_enable();
- return -ENOENT;
- }
-
if (cpu1 > cpu2)
swap(cpu1, cpu2);
if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Oleg Nesterov <oleg@redhat.com> |
|---|---|
| Date | 2015-10-09 18:50 +0200 |
| Subject | Re: [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled |
| Message-ID | <qhJ1L-7NG-7@gated-at.bofh.it> |
| In reply to | #1243523 |
On 10/09, Peter Zijlstra wrote: > > I stuck that on top.. Yes, exactly! Plus I'll send another (minor) cleanup on top of this if everything goes right. > I'll have a closer look at the 7 patches later > when I might be more coherent (mad head-ache atm.) Thanks! Hmm... but please please see another email I'll send in a minute in reply to todays 3/3... Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web