Re: [RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

On Mon, Oct 26, 2015 at 03:24:36PM +0100, Michael Holzheu wrote:
> On Fri, 16 Oct 2015 14:01:25 +0200
> Heiko Carstens  wrote:
> 
> > On Fri, Oct 16, 2015 at 11:57:06AM +0200, Peter Zijlstra wrote:
> > > On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> > > > So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> > > > 
> > > > [ FWIW, I will be offline for the next two weeks ]
> > > 
> > > So the series from Oleg would be good to try; I can make a git tree for
> > > you, or otherwise stuff the lot into a single patch.
> > > 
> > > Should I be talking to someone else whilst you're having down time?
> > 
> > Yes Michael Holzheu (on cc), can take care of this.
> 
> I tested Peter's "tip/master" and "tip/sched/core". With the following
> commit our issue seems to be fixed:
> 
> 2b621a085a ("stop_machine: Change cpu_stop_queue_two_works() to rely
> on stopper->enabled")
> 
> When do you plan to merge the patch series in the mainline kernel?

They're slated for the next merge window. Thanks for testing!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Michael Holzheu]

On Fri, 16 Oct 2015 14:01:25 +0200
Heiko Carstens  wrote:

> On Fri, Oct 16, 2015 at 11:57:06AM +0200, Peter Zijlstra wrote:
> > On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> > > So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> > > 
> > > [ FWIW, I will be offline for the next two weeks ]
> > 
> > So the series from Oleg would be good to try; I can make a git tree for
> > you, or otherwise stuff the lot into a single patch.
> > 
> > Should I be talking to someone else whilst you're having down time?
> 
> Yes Michael Holzheu (on cc), can take care of this.

I tested Peter's "tip/master" and "tip/sched/core". With the following
commit our issue seems to be fixed:

2b621a085a ("stop_machine: Change cpu_stop_queue_two_works() to rely
on stopper->enabled")

When do you plan to merge the patch series in the mainline kernel?

Regards,
Michael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

On Mon, Oct 26, 2015 at 03:24:36PM +0100, Michael Holzheu wrote:
> On Fri, 16 Oct 2015 14:01:25 +0200
> Heiko Carstens  wrote:
> 
> > On Fri, Oct 16, 2015 at 11:57:06AM +0200, Peter Zijlstra wrote:
> > > On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> > > > So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> > > > 
> > > > [ FWIW, I will be offline for the next two weeks ]
> > > 
> > > So the series from Oleg would be good to try; I can make a git tree for
> > > you, or otherwise stuff the lot into a single patch.
> > > 
> > > Should I be talking to someone else whilst you're having down time?
> > 
> > Yes Michael Holzheu (on cc), can take care of this.
> 
> I tested Peter's "tip/master" and "tip/sched/core". With the following
> commit our issue seems to be fixed:
> 
> 2b621a085a ("stop_machine: Change cpu_stop_queue_two_works() to rely
> on stopper->enabled")
> 
> When do you plan to merge the patch series in the mainline kernel?

They're slated for the next merge window. Thanks for testing!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Michael Holzheu]

On Fri, 16 Oct 2015 14:01:25 +0200
Heiko Carstens  wrote:

> On Fri, Oct 16, 2015 at 11:57:06AM +0200, Peter Zijlstra wrote:
> > On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> > > So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> > > 
> > > [ FWIW, I will be offline for the next two weeks ]
> > 
> > So the series from Oleg would be good to try; I can make a git tree for
> > you, or otherwise stuff the lot into a single patch.
> > 
> > Should I be talking to someone else whilst you're having down time?
> 
> Yes Michael Holzheu (on cc), can take care of this.

I tested Peter's "tip/master" and "tip/sched/core". With the following
commit our issue seems to be fixed:

2b621a085a ("stop_machine: Change cpu_stop_queue_two_works() to rely
on stopper->enabled")

When do you plan to merge the patch series in the mainline kernel?

Regards,
Michael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Heiko Carstens]

On Fri, Oct 16, 2015 at 11:57:06AM +0200, Peter Zijlstra wrote:
> On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> > So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> > 
> > [ FWIW, I will be offline for the next two weeks ]
> 
> So the series from Oleg would be good to try; I can make a git tree for
> you, or otherwise stuff the lot into a single patch.
> 
> Should I be talking to someone else whilst you're having down time?

Yes Michael Holzheu (on cc), can take care of this.

Thanks,
Heiko

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> 
> [ FWIW, I will be offline for the next two weeks ]

So the series from Oleg would be good to try; I can make a git tree for
you, or otherwise stuff the lot into a single patch.

Should I be talking to someone else whilst you're having down time?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Heiko Carstens]

On Wed, Oct 07, 2015 at 10:41:10AM +0200, Peter Zijlstra wrote:
> 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));
> +}
> +

So, actually this doesn't fix the bug and it _seems_ to be reproducible.

[ FWIW, I will be offline for the next two weeks ]

The bug was reproduced with your patch applied to 4.2.0 (+ couple of
unrelated internal patches).

In addition I cherry-picked these two upstream commits:
dd9d3843755d "sched: Fix cpu_active_mask/cpu_online_mask race"
02cb7aa923ec "stop_machine: Move 'cpu_stopper_task' and
  'stop_cpus_work' into 'struct cpu_stopper'"

The new dump again shows one cpu looping in multi_cpu_stop() triggered by
stop_two_cpus(), and the second one will never enter multi_cpu_stop() since
the corresponding cpu_stop_work was never enqueued:

The two cpu_stop_work on the stack of the process that invocated
stop_two_cpus() look like this:

crash> struct cpu_stop_work 0x8ad8afa78
struct cpu_stop_work {
  list = {
next = 0x8ad8afa78, 
prev = 0x8ad8afa78
  }, 
  fn = 0x2091b0 , 
  arg = 0x8ad8afac8, 
  done = 0x8ad8afaf0
}

crash> struct cpu_stop_work 0x8ad8afaa0
struct cpu_stop_work {
  list = {
next = 0x0, < NULL indicates it was never enqueued
prev = 0x0
  }, 
  fn = 0x2091b0 , 
  arg = 0x8ad8afac8, 
  done = 0x8ad8afaf0
}

The corresponding struct cpu_stop_done below indicates that at least for
one of them cpu_stop_signal_done() was called (nr_todo == 1). So the idea
is still that this happened when cpu_stop_queue_work() was being called,
but the corresponding stopper was not enabled.

crash> struct -x cpu_stop_done 0008ad8afaf0
struct cpu_stop_done {
  nr_todo = {
counter = 0x1
  },
  executed = 0x0,
  ret = 0x0,
  completion = {
done = 0x0,
wait = {
  lock = {
{
  rlock = {
raw_lock = {
  lock = 0x0
},
break_lock = 0x0,
magic = 0xdead4ead,
owner_cpu = 0x,
owner = 0x,
dep_map = {
  key = 0x1e901e0 <__key.5629>,
  class_cache = {0x188fec0 , 0x0},
  name = 0xb40d0c ">wait",
  cpu = 0xb,
  ip = 0x94e5b2
}
  },
  {
__padding = "\000\000\000\000\000\000\000\000 
ޭN\255\377\377\377\377\377\377\377\377\377\377\377\377",
dep_map = {
  key = 0x1e901e0 <__key.5629>,
  class_cache = {0x188fec0 , 0x0},
  name = 0xb40d0c ">wait",
  cpu = 0xb,
  ip = 0x94e5b2
}
  }
}
  },
  task_list = {

Re: [RFC][PATCH] sched: Start stopper early

[Heiko Carstens]

On Wed, Oct 07, 2015 at 10:41:10AM +0200, Peter Zijlstra wrote:
> 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));
> +}
> +

So, actually this doesn't fix the bug and it _seems_ to be reproducible.

[ FWIW, I will be offline for the next two weeks ]

The bug was reproduced with your patch applied to 4.2.0 (+ couple of
unrelated internal patches).

In addition I cherry-picked these two upstream commits:
dd9d3843755d "sched: Fix cpu_active_mask/cpu_online_mask race"
02cb7aa923ec "stop_machine: Move 'cpu_stopper_task' and
  'stop_cpus_work' into 'struct cpu_stopper'"

The new dump again shows one cpu looping in multi_cpu_stop() triggered by
stop_two_cpus(), and the second one will never enter multi_cpu_stop() since
the corresponding cpu_stop_work was never enqueued:

The two cpu_stop_work on the stack of the process that invocated
stop_two_cpus() look like this:

crash> struct cpu_stop_work 0x8ad8afa78
struct cpu_stop_work {
  list = {
next = 0x8ad8afa78, 
prev = 0x8ad8afa78
  }, 
  fn = 0x2091b0 , 
  arg = 0x8ad8afac8, 
  done = 0x8ad8afaf0
}

crash> struct cpu_stop_work 0x8ad8afaa0
struct cpu_stop_work {
  list = {
next = 0x0, < NULL indicates it was never enqueued
prev = 0x0
  }, 
  fn = 0x2091b0 , 
  arg = 0x8ad8afac8, 
  done = 0x8ad8afaf0
}

The corresponding struct cpu_stop_done below indicates that at least for
one of them cpu_stop_signal_done() was called (nr_todo == 1). So the idea
is still that this happened when cpu_stop_queue_work() was being called,
but the corresponding stopper was not enabled.

crash> struct -x cpu_stop_done 0008ad8afaf0
struct cpu_stop_done {
  nr_todo = {
counter = 0x1
  },
  executed = 0x0,
  ret = 0x0,
  completion = {
done = 0x0,
wait = {
  lock = {
{
  rlock = {
raw_lock = {
  lock = 0x0
},
break_lock = 0x0,
magic = 0xdead4ead,
owner_cpu = 0x,
owner = 0x,
dep_map = {
  key = 0x1e901e0 <__key.5629>,
  class_cache = {0x188fec0 , 0x0},
  name = 0xb40d0c ">wait",
  cpu = 0xb,
  ip = 0x94e5b2
}
  },
  {
__padding = "\000\000\000\000\000\000\000\000 
ޭN\255\377\377\377\377\377\377\377\377\377\377\377\377",
dep_map = {
  key = 0x1e901e0 <__key.5629>,
  class_cache = {0x188fec0 , 0x0},
  name = 0xb40d0c ">wait",
  cpu = 0xb,
  ip = 0x94e5b2
}
  }
   

Re: [RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> 
> [ FWIW, I will be offline for the next two weeks ]

So the series from Oleg would be good to try; I can make a git tree for
you, or otherwise stuff the lot into a single patch.

Should I be talking to someone else whilst you're having down time?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Heiko Carstens]

On Fri, Oct 16, 2015 at 11:57:06AM +0200, Peter Zijlstra wrote:
> On Fri, Oct 16, 2015 at 10:22:12AM +0200, Heiko Carstens wrote:
> > So, actually this doesn't fix the bug and it _seems_ to be reproducible.
> > 
> > [ FWIW, I will be offline for the next two weeks ]
> 
> So the series from Oleg would be good to try; I can make a git tree for
> you, or otherwise stuff the lot into a single patch.
> 
> Should I be talking to someone else whilst you're having down time?

Yes Michael Holzheu (on cc), can take care of this.

Thanks,
Heiko

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

On 10/08, Oleg Nesterov wrote:
>
> To avoid the confusion, let me repeat that I am not arguing with
> this change, perhaps it makes sense too.
>
> But unless I missed something it is not really correct and can't
> fix the problem. So I still think the series I sent should be
> applied first.

...

> But note that kthread_unpark() will only wake the stopper thread up.
>
> cpu_stopper->enabled is still false, and it will be false until
> smpboot_unpark_thread() calls ->pre_unpark() later. And this means
> that stop_two_cpus()->cpu_stop_queue_work() can silently fail until
> then. So I don't this patch can fix the problem.

So I'd suggest something like the patch below (uncompiled/untested)
on top of the series I sent.

Note also that (at least imo) it looks like a cleanup, and even
->selfparking becomes more consistent.

What do you think?

Oleg.


--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -40,7 +40,6 @@ struct smp_hotplug_thread {
void(*cleanup)(unsigned int cpu, bool 
online);
void(*park)(unsigned int cpu);
void(*unpark)(unsigned int cpu);
-   void(*pre_unpark)(unsigned int cpu);
boolselfparking;
const char  *thread_comm;
 };
diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
index 7b76362..0adedca 100644
--- a/include/linux/stop_machine.h
+++ b/include/linux/stop_machine.h
@@ -34,6 +34,7 @@ void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, 
void *arg,
 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);
+void stop_machine_unpark(int cpu);
 
 #else  /* CONFIG_SMP */
 
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -221,9 +221,8 @@ static void smpboot_unpark_thread(struct smp_hotplug_thread 
*ht, unsigned int cp
 {
struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
 
-   if (ht->pre_unpark)
-   ht->pre_unpark(cpu);
-   kthread_unpark(tsk);
+   if (!ht->selfparking)
+   kthread_unpark(tsk);
 }
 
 void smpboot_unpark_threads(unsigned int cpu)
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -513,6 +513,14 @@ static void cpu_stop_unpark(unsigned int cpu)
spin_unlock_irq(>lock);
 }
 
+void stop_machine_unpark(int cpu)
+{
+   struct cpu_stopper *stopper = _cpu(cpu_stopper, cpu);
+
+   cpu_stop_unpark(cpu);
+   kthread_unpark(stopper->thread);
+}
+
 static struct smp_hotplug_thread cpu_stop_threads = {
.store  = _stopper.thread,
.thread_should_run  = cpu_stop_should_run,
@@ -521,7 +529,6 @@ static struct smp_hotplug_thread cpu_stop_threads = {
.create = cpu_stop_create,
.setup  = cpu_stop_unpark,
.park   = cpu_stop_park,
-   .pre_unpark = cpu_stop_unpark,
.selfparking= true,
 };
 
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5515,11 +5515,14 @@ static void set_cpu_rq_start_time(void)
 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:
+   stop_machine_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
@@ -5528,7 +5531,7 @@ static int sched_cpu_active(struct notif
 * 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;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

To avoid the confusion, let me repeat that I am not arguing with
this change, perhaps it makes sense too.

But unless I missed something it is not really correct and can't
fix the problem. So I still think the series I sent should be
applied first.

On 10/07, Peter Zijlstra wrote:
>  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));
> +}

But note that kthread_unpark() will only wake the stopper thread up.

cpu_stopper->enabled is still false, and it will be false until
smpboot_unpark_thread() calls ->pre_unpark() later. And this means
that stop_two_cpus()->cpu_stop_queue_work() can silently fail until
then. So I don't this patch can fix the problem.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oleg Nesterov]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oleg Nesterov]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

To avoid the confusion, let me repeat that I am not arguing with
this change, perhaps it makes sense too.

But unless I missed something it is not really correct and can't
fix the problem. So I still think the series I sent should be
applied first.

On 10/07, Peter Zijlstra wrote:
>  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));
> +}

But note that kthread_unpark() will only wake the stopper thread up.

cpu_stopper->enabled is still false, and it will be false until
smpboot_unpark_thread() calls ->pre_unpark() later. And this means
that stop_two_cpus()->cpu_stop_queue_work() can silently fail until
then. So I don't this patch can fix the problem.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

On 10/08, Oleg Nesterov wrote:
>
> To avoid the confusion, let me repeat that I am not arguing with
> this change, perhaps it makes sense too.
>
> But unless I missed something it is not really correct and can't
> fix the problem. So I still think the series I sent should be
> applied first.

...

> But note that kthread_unpark() will only wake the stopper thread up.
>
> cpu_stopper->enabled is still false, and it will be false until
> smpboot_unpark_thread() calls ->pre_unpark() later. And this means
> that stop_two_cpus()->cpu_stop_queue_work() can silently fail until
> then. So I don't this patch can fix the problem.

So I'd suggest something like the patch below (uncompiled/untested)
on top of the series I sent.

Note also that (at least imo) it looks like a cleanup, and even
->selfparking becomes more consistent.

What do you think?

Oleg.


--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -40,7 +40,6 @@ struct smp_hotplug_thread {
void(*cleanup)(unsigned int cpu, bool 
online);
void(*park)(unsigned int cpu);
void(*unpark)(unsigned int cpu);
-   void(*pre_unpark)(unsigned int cpu);
boolselfparking;
const char  *thread_comm;
 };
diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
index 7b76362..0adedca 100644
--- a/include/linux/stop_machine.h
+++ b/include/linux/stop_machine.h
@@ -34,6 +34,7 @@ void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, 
void *arg,
 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);
+void stop_machine_unpark(int cpu);
 
 #else  /* CONFIG_SMP */
 
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -221,9 +221,8 @@ static void smpboot_unpark_thread(struct smp_hotplug_thread 
*ht, unsigned int cp
 {
struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
 
-   if (ht->pre_unpark)
-   ht->pre_unpark(cpu);
-   kthread_unpark(tsk);
+   if (!ht->selfparking)
+   kthread_unpark(tsk);
 }
 
 void smpboot_unpark_threads(unsigned int cpu)
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -513,6 +513,14 @@ static void cpu_stop_unpark(unsigned int cpu)
spin_unlock_irq(>lock);
 }
 
+void stop_machine_unpark(int cpu)
+{
+   struct cpu_stopper *stopper = _cpu(cpu_stopper, cpu);
+
+   cpu_stop_unpark(cpu);
+   kthread_unpark(stopper->thread);
+}
+
 static struct smp_hotplug_thread cpu_stop_threads = {
.store  = _stopper.thread,
.thread_should_run  = cpu_stop_should_run,
@@ -521,7 +529,6 @@ static struct smp_hotplug_thread cpu_stop_threads = {
.create = cpu_stop_create,
.setup  = cpu_stop_unpark,
.park   = cpu_stop_park,
-   .pre_unpark = cpu_stop_unpark,
.selfparking= true,
 };
 
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5515,11 +5515,14 @@ static void set_cpu_rq_start_time(void)
 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:
+   stop_machine_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
@@ -5528,7 +5531,7 @@ static int sched_cpu_active(struct notif
 * 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;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: [RFC][PATCH] sched: Start stopper early

[kbuild test robot]

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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

Damn sorry for noise ;)

On 10/07, Oleg Nesterov wrote:
>
> +void stop_machine_park(int cpu)
> +{
> + struct cpu_stopper *stopper = _cpu(cpu_stopper, cpu);
> +
> + spin_lock(>lock);
> + stopper->enabled = false;
> + spin_unlock(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

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 = _cpu(cpu_stopper, cpu);
+
+   spin_lock(>lock);
+   stopper->enabled = false;
+   spin_unlock(>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(>lock, flags);
list_for_each_entry_safe(work, tmp, >works, list) {
+   WARN_ON(1);
list_del_init(>list);
cpu_stop_signal_done(work->done, false);
}
-   stopper->enabled = false;
spin_unlock_irqrestore(>lock, flags);
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Re: [RFC][PATCH] sched: Start stopper early

[kbuild test robot]

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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Peter Zijlstra]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

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 = _cpu(cpu_stopper, cpu);
+
+   spin_lock(>lock);
+   stopper->enabled = false;
+   spin_unlock(>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(>lock, flags);
list_for_each_entry_safe(work, tmp, >works, list) {
+   WARN_ON(1);
list_del_init(>list);
cpu_stop_signal_done(work->done, false);
}
-   stopper->enabled = false;
spin_unlock_irqrestore(>lock, flags);
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH] sched: Start stopper early

[Oleg Nesterov]

Damn sorry for noise ;)

On 10/07, Oleg Nesterov wrote:
>
> +void stop_machine_park(int cpu)
> +{
> + struct cpu_stopper *stopper = _cpu(cpu_stopper, cpu);
> +
> + spin_lock(>lock);
> + stopper->enabled = false;
> + spin_unlock(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/