linux-kernel - Re: [PATCH v2] ARM: Don't use complete() during __cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150205142918.GA10634@linux.vnet.ibm.com>
Date:	Thu, 5 Feb 2015 06:29:18 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Arnd Bergmann <arnd@...db.de>,
	Mark Rutland <mark.rutland@....com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>
Subject: Re: [PATCH v2] ARM: Don't use complete() during __cpu_die

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.
> 

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