Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1241242 > unrolled thread

[RFC][PATCH] sched: Start stopper early

Started byPeter Zijlstra <peterz@infradead.org>
First post2015-10-07 10:50 +0200
Last post2015-10-16 14:10 +0200
Articles 20 on this page of 31 — 4 participants

Back to article view | Back to linux.kernel


Contents

  [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 →


#1241242 — [RFC][PATCH] sched: Start stopper early

FromPeter Zijlstra <peterz@infradead.org>
Date2015-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]


#1241445

FromOleg Nesterov <oleg@redhat.com>
Date2015-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]


#1241446

FromPeter Zijlstra <peterz@infradead.org>
Date2015-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]


#1241487

FromOleg Nesterov <oleg@redhat.com>
Date2015-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]


#1241492

FromOleg Nesterov <oleg@redhat.com>
Date2015-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]


#1241500

Fromkbuild test robot <lkp@intel.com>
Date2015-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]


#1242471 — [PATCH 0/3] (Was: [RFC][PATCH] sched: Start stopper early)

FromOleg Nesterov <oleg@redhat.com>
Date2015-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]


#1242479 — [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park()

FromOleg Nesterov <oleg@redhat.com>
Date2015-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]


#1246914 — Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park()

FromPeter Zijlstra <peterz@infradead.org>
Date2015-10-14 17:40 +0200
SubjectRe: [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]


#1247087 — Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park()

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-14 21:10 +0200
SubjectRe: [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]


#1247139 — Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park()

FromPeter Zijlstra <peterz@infradead.org>
Date2015-10-14 22:40 +0200
SubjectRe: [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]


#1248016 — Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park()

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-15 19:10 +0200
SubjectRe: [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]


#1248639 — Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park()

FromPeter Zijlstra <peterz@infradead.org>
Date2015-10-16 13:00 +0200
SubjectRe: [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]


#1242490 — [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled

FromOleg Nesterov <oleg@redhat.com>
Date2015-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]


#1242504 — Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled

FromPeter Zijlstra <peterz@infradead.org>
Date2015-10-08 17:10 +0200
SubjectRe: [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]


#1242582 — Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-08 18:10 +0200
SubjectRe: [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]


#1242586 — Re: [PATCH 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-08 18:20 +0200
SubjectRe: [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]


#1242616 — [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled

FromOleg Nesterov <oleg@redhat.com>
Date2015-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]


#1243523 — Re: [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled

FromPeter Zijlstra <peterz@infradead.org>
Date2015-10-09 18:40 +0200
SubjectRe: [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]


#1243528 — Re: [PATCH v2 3/3] stop_machine: change cpu_stop_queue_two_works() to rely on stopper->enabled

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-09 18:50 +0200
SubjectRe: [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