2013-05-03 22:35:48

by Russ Anderson

[permalink] [raw]
Subject: [PATCH] cpu: Speedup disable_nonboot_cpus()

The routine disable_nonboot_cpus() shuts down cpus sequentially
using for_each_online_cpu(cpu) to call cpu_down() one cpu at
a time. cpu_down() calls __stop_machine() which stops all
the cpus while it disables one. Then it re-enables the remaining
cpus, only to do it all over again for the next cpu. The
result is that it takes 16 minutes on a 1024 cpu system to
disable 1023 cpus.

This patch changes disable_nonboot_cpus() to pass a bitmask
of cpus to cpu_down() and modifies cpu_down() to only call
__stop_machine() once.

On a 1024 cpu system this reduces the time it takes to disable
all but one cpu from 16 minutes down to 4 minutes.

Signed-off-by: Russ Anderson <[email protected]>
To: Andrew Morton <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: Russ Anderson <[email protected]>

---
kernel/cpu.c | 104 ++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 61 insertions(+), 43 deletions(-)

Index: linux/kernel/cpu.c
===================================================================
--- linux.orig/kernel/cpu.c 2013-05-03 09:56:31.145508321 -0500
+++ linux/kernel/cpu.c 2013-05-03 17:01:20.652959400 -0500
@@ -241,13 +241,13 @@ static inline void check_for_tasks(int c

struct take_cpu_down_param {
unsigned long mod;
- void *hcpu;
};

/* Take this CPU down. */
static int __ref take_cpu_down(void *_param)
{
struct take_cpu_down_param *param = _param;
+ void *hcpu = (void *)(long)smp_processor_id();
int err;

/* Ensure this CPU doesn't handle any more interrupts. */
@@ -255,21 +255,21 @@ static int __ref take_cpu_down(void *_pa
if (err < 0)
return err;

- cpu_notify(CPU_DYING | param->mod, param->hcpu);
+ cpu_notify(CPU_DYING | param->mod, hcpu);
/* Park the stopper thread */
kthread_park(current);
return 0;
}

/* Requires cpu_add_remove_lock to be held */
-static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
+static int __ref _cpu_down(const cpumask_t *cpus_to_offline, int tasks_frozen)
{
- int err, nr_calls = 0;
+ int err = 0, cpu = 0, nr_calls = 0;
void *hcpu = (void *)(long)cpu;
+ cpumask_var_t cpus_offlined;
unsigned long mod = tasks_frozen ? CPU_TASKS_FROZEN : 0;
struct take_cpu_down_param tcd_param = {
.mod = mod,
- .hcpu = hcpu,
};

if (num_online_cpus() == 1)
@@ -278,46 +278,67 @@ static int __ref _cpu_down(unsigned int
if (!cpu_online(cpu))
return -EINVAL;

+ if (!alloc_cpumask_var(&cpus_offlined, GFP_KERNEL))
+ return -ENOMEM;
+
cpu_hotplug_begin();
+ cpumask_copy(cpus_offlined, cpus_to_offline);

- err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
- if (err) {
- nr_calls--;
- __cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
- printk("%s: attempt to take down CPU %u failed\n",
+ for_each_cpu_mask(cpu, *cpus_to_offline) {
+ if (!cpu_online(cpu))
+ continue;
+ hcpu = (void *)(long)cpu;
+ err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
+ if (err) {
+ nr_calls--;
+ __cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
+ pr_err("%s: attempt to take down CPU %u failed\n",
__func__, cpu);
- goto out_release;
+ goto out_release;
+ }
+ smpboot_park_threads(cpu);
}
- smpboot_park_threads(cpu);

- err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
+ err = __stop_machine(take_cpu_down, &tcd_param, cpus_to_offline);
if (err) {
/* CPU didn't die: tell everyone. Can't complain. */
- smpboot_unpark_threads(cpu);
- cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
+ for_each_cpu_mask(cpu, *cpus_to_offline) {
+ hcpu = (void *)(long)cpu;
+ smpboot_unpark_threads(cpu);
+ cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
+ }
goto out_release;
}
- BUG_ON(cpu_online(cpu));

/*
* The migration_call() CPU_DYING callback will have removed all
* runnable tasks from the cpu, there's only the idle task left now
* that the migration thread is done doing the stop_machine thing.
- *
- * Wait for the stop thread to go away.
*/
- while (!idle_cpu(cpu))
- cpu_relax();
-
- /* This actually kills the CPU. */
- __cpu_die(cpu);
+ for_each_cpu_mask(cpu, *cpus_offlined) {
+ BUG_ON(cpu_online(cpu));

- /* CPU is completely dead: tell everyone. Too late to complain. */
- cpu_notify_nofail(CPU_DEAD | mod, hcpu);
-
- check_for_tasks(cpu);
+ /*
+ * Wait for the stop thread to go away.
+ */
+ while (!idle_cpu(cpu))
+ cpu_relax();
+
+ /*
+ * This actually kills the CPU.
+ */
+ __cpu_die(cpu);
+
+ /*
+ * CPU is completely dead: tell everyone. Too late to complain.
+ */
+ hcpu = (void *)(long)cpu;
+ cpu_notify_nofail(CPU_DEAD | mod, hcpu);
+ check_for_tasks(cpu);
+ }

out_release:
+ free_cpumask_var(cpus_offlined);
cpu_hotplug_done();
if (!err)
cpu_notify_nofail(CPU_POST_DEAD | mod, hcpu);
@@ -327,6 +348,7 @@ out_release:
int __ref cpu_down(unsigned int cpu)
{
int err;
+ cpumask_var_t cpumask;

cpu_maps_update_begin();

@@ -335,7 +357,11 @@ int __ref cpu_down(unsigned int cpu)
goto out;
}

- err = _cpu_down(cpu, 0);
+ if (!alloc_cpumask_var(&cpumask, GFP_KERNEL))
+ return -ENOMEM;
+ cpumask_set_cpu(cpu, cpumask);
+ err = _cpu_down(cpumask, 0);
+ free_cpumask_var(cpumask);

out:
cpu_maps_update_done();
@@ -459,7 +485,7 @@ static cpumask_var_t frozen_cpus;

int disable_nonboot_cpus(void)
{
- int cpu, first_cpu, error = 0;
+ int first_cpu, error = 0;

cpu_maps_update_begin();
first_cpu = cpumask_first(cpu_online_mask);
@@ -467,21 +493,13 @@ int disable_nonboot_cpus(void)
* We take down all of the non-boot CPUs in one shot to avoid races
* with the userspace trying to use the CPU hotplug at the same time
*/
- cpumask_clear(frozen_cpus);
-
printk("Disabling non-boot CPUs ...\n");
- for_each_online_cpu(cpu) {
- if (cpu == first_cpu)
- continue;
- error = _cpu_down(cpu, 1);
- if (!error)
- cpumask_set_cpu(cpu, frozen_cpus);
- else {
- printk(KERN_ERR "Error taking CPU%d down: %d\n",
- cpu, error);
- break;
- }
- }
+ cpumask_copy(frozen_cpus, cpu_online_mask);
+ cpumask_clear_cpu(first_cpu, frozen_cpus); /* all but one cpu*/
+
+ error = _cpu_down(frozen_cpus, 1);
+ if (error)
+ pr_err("Error %d stopping cpus\n", error);

if (!error) {
BUG_ON(num_online_cpus() > 1);
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc [email protected]


2013-05-21 21:17:20

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] cpu: Speedup disable_nonboot_cpus()

On Fri, 3 May 2013 17:35:44 -0500 Russ Anderson <[email protected]> wrote:

> The routine disable_nonboot_cpus() shuts down cpus sequentially
> using for_each_online_cpu(cpu) to call cpu_down() one cpu at
> a time. cpu_down() calls __stop_machine() which stops all
> the cpus while it disables one. Then it re-enables the remaining
> cpus, only to do it all over again for the next cpu. The
> result is that it takes 16 minutes on a 1024 cpu system to
> disable 1023 cpus.
>
> This patch changes disable_nonboot_cpus() to pass a bitmask
> of cpus to cpu_down() and modifies cpu_down() to only call
> __stop_machine() once.
>
> On a 1024 cpu system this reduces the time it takes to disable
> all but one cpu from 16 minutes down to 4 minutes.

That's still a helluva long time. What's the kernel *doing* for
all that time?

> --- linux.orig/kernel/cpu.c 2013-05-03 09:56:31.145508321 -0500
> +++ linux/kernel/cpu.c 2013-05-03 17:01:20.652959400 -0500
>
> ...
>
> @@ -255,21 +255,21 @@ static int __ref take_cpu_down(void *_pa
> if (err < 0)
> return err;
>
> - cpu_notify(CPU_DYING | param->mod, param->hcpu);
> + cpu_notify(CPU_DYING | param->mod, hcpu);
> /* Park the stopper thread */
> kthread_park(current);
> return 0;
> }
>
> /* Requires cpu_add_remove_lock to be held */
> -static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
> +static int __ref _cpu_down(const cpumask_t *cpus_to_offline, int tasks_frozen)

_cpu_down() is now misnamed - it downs multiple CPUs.

> {
> - int err, nr_calls = 0;
> + int err = 0, cpu = 0, nr_calls = 0;
> void *hcpu = (void *)(long)cpu;
> + cpumask_var_t cpus_offlined;
> unsigned long mod = tasks_frozen ? CPU_TASKS_FROZEN : 0;
> struct take_cpu_down_param tcd_param = {
> .mod = mod,
> - .hcpu = hcpu,
> };
>
> if (num_online_cpus() == 1)
> @@ -278,46 +278,67 @@ static int __ref _cpu_down(unsigned int
> if (!cpu_online(cpu))
> return -EINVAL;
>
> + if (!alloc_cpumask_var(&cpus_offlined, GFP_KERNEL))
> + return -ENOMEM;
> +
> cpu_hotplug_begin();
> + cpumask_copy(cpus_offlined, cpus_to_offline);
>
> - err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
> - if (err) {
> - nr_calls--;
> - __cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
> - printk("%s: attempt to take down CPU %u failed\n",
> + for_each_cpu_mask(cpu, *cpus_to_offline) {
> + if (!cpu_online(cpu))
> + continue;
> + hcpu = (void *)(long)cpu;
> + err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
> + if (err) {
> + nr_calls--;
> + __cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
> + pr_err("%s: attempt to take down CPU %u failed\n",
> __func__, cpu);
> - goto out_release;
> + goto out_release;
> + }
> + smpboot_park_threads(cpu);
> }
> - smpboot_park_threads(cpu);
>
> - err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
> + err = __stop_machine(take_cpu_down, &tcd_param, cpus_to_offline);
> if (err) {
> /* CPU didn't die: tell everyone. Can't complain. */

This comment is now inaccurate. "One or more of the CPUs didn't die"?.

I'm not sure what "Can't complain" means. Perhaps expand on this while
you're there?

> - smpboot_unpark_threads(cpu);
> - cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
> + for_each_cpu_mask(cpu, *cpus_to_offline) {
> + hcpu = (void *)(long)cpu;
> + smpboot_unpark_threads(cpu);
> + cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);

Is this accurate? What happens if we asked stop_machine() to down 100
CPUs but it failed at CPU #50? We now tell listeners that we failed to
down all 100 CPUs? That's not true.

> + }
> goto out_release;
> }
> - BUG_ON(cpu_online(cpu));
>
> /*
> * The migration_call() CPU_DYING callback will have removed all
> * runnable tasks from the cpu, there's only the idle task left now
> * that the migration thread is done doing the stop_machine thing.
> - *
> - * Wait for the stop thread to go away.
> */
> - while (!idle_cpu(cpu))
> - cpu_relax();
> -
> - /* This actually kills the CPU. */
> - __cpu_die(cpu);
> + for_each_cpu_mask(cpu, *cpus_offlined) {
> + BUG_ON(cpu_online(cpu));
>
> - /* CPU is completely dead: tell everyone. Too late to complain. */
> - cpu_notify_nofail(CPU_DEAD | mod, hcpu);
> -
> - check_for_tasks(cpu);
> + /*
> + * Wait for the stop thread to go away.
> + */
> + while (!idle_cpu(cpu))
> + cpu_relax();
> +
> + /*
> + * This actually kills the CPU.
> + */
> + __cpu_die(cpu);
> +
> + /*
> + * CPU is completely dead: tell everyone. Too late to complain.
> + */
> + hcpu = (void *)(long)cpu;
> + cpu_notify_nofail(CPU_DEAD | mod, hcpu);
> + check_for_tasks(cpu);
> + }
>
> out_release:
> + free_cpumask_var(cpus_offlined);
> cpu_hotplug_done();

Swap the above two lines and we reduced the locked region by an
unmeasurable amount!

> if (!err)
> cpu_notify_nofail(CPU_POST_DEAD | mod, hcpu);
>
> ...
>

2013-05-21 22:25:35

by Russ Anderson

[permalink] [raw]
Subject: Re: [PATCH] cpu: Speedup disable_nonboot_cpus()

On Tue, May 21, 2013 at 02:17:17PM -0700, Andrew Morton wrote:
> On Fri, 3 May 2013 17:35:44 -0500 Russ Anderson <[email protected]> wrote:
>
> > The routine disable_nonboot_cpus() shuts down cpus sequentially
> > using for_each_online_cpu(cpu) to call cpu_down() one cpu at
> > a time. cpu_down() calls __stop_machine() which stops all
> > the cpus while it disables one. Then it re-enables the remaining
> > cpus, only to do it all over again for the next cpu. The
> > result is that it takes 16 minutes on a 1024 cpu system to
> > disable 1023 cpus.
> >
> > This patch changes disable_nonboot_cpus() to pass a bitmask
> > of cpus to cpu_down() and modifies cpu_down() to only call
> > __stop_machine() once.
> >
> > On a 1024 cpu system this reduces the time it takes to disable
> > all but one cpu from 16 minutes down to 4 minutes.
>
> That's still a helluva long time. What's the kernel *doing* for
> all that time?

Since __raw_notifier_call_chain() isn't thread safe, it is still
called one cpu at a time. See "__cpu_notify(CPU_DOWN_PREPARE" below.
So we still go sequentially for each cpu through the call chain.

These are the two heaviest hitters on the call chain.

sched_init_smp ->
cpuset_cpu_inactive ->
cpuset_update_active_cpus ->
partition_sched_domains(1, NULL, NULL)
schedule_work(&cpuset_hotplug_work);
cpuset_hotplug_workfn
mutex_lock(&cpuset_mutex);
partition_sched_domains()

partition_sched_domains ->
unregister_sched_domain_sysctl() ->
unregister_sysctl_table
register_sched_domain_sysctl
for_each_possible_cpu()
register_sysctl_table


uncore_cpu_nb -> uncore_cpu_notifier
uncore_event_exit_cpu
for_each_online_cpu()

The partition_sched_domains() code gets called for each cpu,
each time destroying and building new domains.
register_sched_domain_sysctl() calls for_each_possible_cpu()
each time.

Likewise uncore_event_exit_cpu() calls for_each_online_cpu()
for each cpu.

Neither of those scale well.


> > --- linux.orig/kernel/cpu.c 2013-05-03 09:56:31.145508321 -0500
> > +++ linux/kernel/cpu.c 2013-05-03 17:01:20.652959400 -0500
> >
> > ...
> >
> > @@ -255,21 +255,21 @@ static int __ref take_cpu_down(void *_pa
> > if (err < 0)
> > return err;
> >
> > - cpu_notify(CPU_DYING | param->mod, param->hcpu);
> > + cpu_notify(CPU_DYING | param->mod, hcpu);
> > /* Park the stopper thread */
> > kthread_park(current);
> > return 0;
> > }
> >
> > /* Requires cpu_add_remove_lock to be held */
> > -static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
> > +static int __ref _cpu_down(const cpumask_t *cpus_to_offline, int tasks_frozen)
>
> _cpu_down() is now misnamed - it downs multiple CPUs.

OK, I'll change it.

> > {
> > - int err, nr_calls = 0;
> > + int err = 0, cpu = 0, nr_calls = 0;
> > void *hcpu = (void *)(long)cpu;
> > + cpumask_var_t cpus_offlined;
> > unsigned long mod = tasks_frozen ? CPU_TASKS_FROZEN : 0;
> > struct take_cpu_down_param tcd_param = {
> > .mod = mod,
> > - .hcpu = hcpu,
> > };
> >
> > if (num_online_cpus() == 1)
> > @@ -278,46 +278,67 @@ static int __ref _cpu_down(unsigned int
> > if (!cpu_online(cpu))
> > return -EINVAL;
> >
> > + if (!alloc_cpumask_var(&cpus_offlined, GFP_KERNEL))
> > + return -ENOMEM;
> > +
> > cpu_hotplug_begin();
> > + cpumask_copy(cpus_offlined, cpus_to_offline);
> >
> > - err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
> > - if (err) {
> > - nr_calls--;
> > - __cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
> > - printk("%s: attempt to take down CPU %u failed\n",
> > + for_each_cpu_mask(cpu, *cpus_to_offline) {
> > + if (!cpu_online(cpu))
> > + continue;
> > + hcpu = (void *)(long)cpu;
> > + err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
> > + if (err) {
> > + nr_calls--;
> > + __cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
> > + pr_err("%s: attempt to take down CPU %u failed\n",
> > __func__, cpu);
> > - goto out_release;
> > + goto out_release;
> > + }
> > + smpboot_park_threads(cpu);
> > }
> > - smpboot_park_threads(cpu);
> >
> > - err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
> > + err = __stop_machine(take_cpu_down, &tcd_param, cpus_to_offline);
> > if (err) {
> > /* CPU didn't die: tell everyone. Can't complain. */
>
> This comment is now inaccurate. "One or more of the CPUs didn't die"?.
>
> I'm not sure what "Can't complain" means. Perhaps expand on this while
> you're there?

OK.

> > - smpboot_unpark_threads(cpu);
> > - cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
> > + for_each_cpu_mask(cpu, *cpus_to_offline) {
> > + hcpu = (void *)(long)cpu;
> > + smpboot_unpark_threads(cpu);
> > + cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
>
> Is this accurate? What happens if we asked stop_machine() to down 100
> CPUs but it failed at CPU #50? We now tell listeners that we failed to
> down all 100 CPUs? That's not true.

I'll look closer at that.

> > + }
> > goto out_release;
> > }
> > - BUG_ON(cpu_online(cpu));
> >
> > /*
> > * The migration_call() CPU_DYING callback will have removed all
> > * runnable tasks from the cpu, there's only the idle task left now
> > * that the migration thread is done doing the stop_machine thing.
> > - *
> > - * Wait for the stop thread to go away.
> > */
> > - while (!idle_cpu(cpu))
> > - cpu_relax();
> > -
> > - /* This actually kills the CPU. */
> > - __cpu_die(cpu);
> > + for_each_cpu_mask(cpu, *cpus_offlined) {
> > + BUG_ON(cpu_online(cpu));
> >
> > - /* CPU is completely dead: tell everyone. Too late to complain. */
> > - cpu_notify_nofail(CPU_DEAD | mod, hcpu);
> > -
> > - check_for_tasks(cpu);
> > + /*
> > + * Wait for the stop thread to go away.
> > + */
> > + while (!idle_cpu(cpu))
> > + cpu_relax();
> > +
> > + /*
> > + * This actually kills the CPU.
> > + */
> > + __cpu_die(cpu);
> > +
> > + /*
> > + * CPU is completely dead: tell everyone. Too late to complain.
> > + */
> > + hcpu = (void *)(long)cpu;
> > + cpu_notify_nofail(CPU_DEAD | mod, hcpu);
> > + check_for_tasks(cpu);
> > + }
> >
> > out_release:
> > + free_cpumask_var(cpus_offlined);
> > cpu_hotplug_done();
>
> Swap the above two lines and we reduced the locked region by an
> unmeasurable amount!
>
> > if (!err)
> > cpu_notify_nofail(CPU_POST_DEAD | mod, hcpu);
> >
> > ...
> >

--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc [email protected]