[PATCH v2] ARM: Don't use complete() during __cpu_die

Paul E. McKenney
2015-02-05T14:29:18Z

On Thu, Feb 05, 2015 at 10:50:35AM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 05, 2015 at 11:14:30AM +0100, Krzysztof Kozlowski wrote:
> > The complete() should not be used on offlined CPU. Rewrite the
> > wait-complete mechanism with wait_on_bit_timeout().
>
> Yuck.
>
> I think that the IPI idea would be far better, and a much smaller patch.
> We can continue using the completions, but instead of running the
> completion on the dying CPU, the dying CPU triggers an IPI which does
> the completion on the requesting CPU.
>
> You're modifying arch/arm/kernel/smp.c, so you just hook it directly
> into the IPI mechanism without any registration required.
>
> We can also kill the second cache flush by the dying CPU - as we're
> not writing to memory anymore by calling complete() after the first
> cache flush, so this will probably make CPU hotplug fractionally faster
> too.
>
> (You'll get some fuzz with this patch as I have the NMI backtrace stuff
> in my kernel.)
>
> Something like this - only build tested so far (waiting for the compile
> to finish...):

Works for me, assuming no hidden uses of RCU in the IPI code. ;-)

Thanx, Paul

> arch/arm/kernel/smp.c | 43 ++++++++++++++++++++++++-------------------
> 1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 194df2f1aa87..c623e27a9c85 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -73,6 +73,9 @@ enum ipi_msg_type {
> IPI_IRQ_WORK,
> IPI_COMPLETION,
> IPI_CPU_BACKTRACE,
> +#ifdef CONFIG_HOTPLUG_CPU
> + IPI_CPU_DEAD,
> +#endif
> };
>
> /* For reliability, we're prepared to waste bits here. */
> @@ -88,6 +91,14 @@ void __init smp_set_ops(struct smp_operations *ops)
> smp_ops = *ops;
> };
>
> +static void (*__smp_cross_call)(const struct cpumask *, unsigned int);
> +
> +void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
> +{
> + if (!__smp_cross_call)
> + __smp_cross_call = fn;
> +}
> +
> static unsigned long get_arch_pgd(pgd_t *pgd)
> {
> phys_addr_t pgdir = virt_to_idmap(pgd);
> @@ -267,19 +278,13 @@ void __ref cpu_die(void)
> flush_cache_louis();
>
> /*
> - * Tell __cpu_die() that this CPU is now safe to dispose of. Once
> - * this returns, power and/or clocks can be removed at any point
> + * Tell __cpu_die() that this CPU is now safe to dispose of. We
> + * do this via an IPI to any online CPU - it doesn't matter, we
> + * just need another CPU to run the completion. Once this IPI
> + * has been sent, power and/or clocks can be removed at any point
> * from this CPU and its cache by platform_cpu_kill().
> */
> - complete(&cpu_died);
> -
> - /*
> - * Ensure that the cache lines associated with that completion are
> - * written out. This covers the case where _this_ CPU is doing the
> - * powering down, to ensure that the completion is visible to the
> - * CPU waiting for this one.
> - */
> - flush_cache_louis();
> + __smp_cross_call(cpumask_of(cpumask_any(cpu_online_mask)), IPI_CPU_DEAD);
>
> /*
> * The actual CPU shutdown procedure is at least platform (if not
> @@ -442,14 +447,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> }
> }
>
> -static void (*__smp_cross_call)(const struct cpumask *, unsigned int);
> -
> -void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
> -{
> - if (!__smp_cross_call)
> - __smp_cross_call = fn;
> -}
> -
> static const char *ipi_types[NR_IPI] __tracepoint_string = {
> #define S(x,s) [x] = s
> S(IPI_WAKEUP, "CPU wakeup interrupts"),
> @@ -648,6 +645,14 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
> irq_exit();
> break;
>
> +#ifdef CONFIG_HOTPLUG_CPU
> + case IPI_CPU_DEAD:
> + irq_enter();
> + complete(&cpu_died);
> + irq_exit();
> + break;
> +#endif
> +
> default:
> pr_crit("CPU%u: Unknown IPI message 0x%x\n",
> cpu, ipinr);
>
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
>