[patch V2 08/24] hwtracing/coresight-etm3x: Use cpuhp_setup_state_nocalls_cpuslocked()

[patch V2 08/24] hwtracing/coresight-etm3x: Use cpuhp_setup_state_nocalls_cpuslocked()

[Thomas Gleixner]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

etm_probe() holds get_online_cpus() while invoking
cpuhp_setup_state_nocalls().

cpuhp_setup_state_nocalls() invokes get_online_cpus() as well. This is
correct, but prevents the conversion of the hotplug locking to a percpu
rwsem.

Use cpuhp_setup_state_nocalls_cpuslocked() to avoid the nested call.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: linux-arm-kernel at lists.infradead.org

---
 drivers/hwtracing/coresight/coresight-etm3x.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/drivers/hwtracing/coresight/coresight-etm3x.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x.c
@@ -803,12 +803,12 @@ static int etm_probe(struct amba_device
 		dev_err(dev, "ETM arch init failed\n");
 
 	if (!etm_count++) {
-		cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
-					  "arm/coresight:starting",
-					  etm_starting_cpu, etm_dying_cpu);
-		ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-						"arm/coresight:online",
-						etm_online_cpu, NULL);
+		cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
+						     "arm/coresight:starting",
+						     etm_starting_cpu, etm_dying_cpu);
+		ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
+							   "arm/coresight:online",
+							   etm_online_cpu, NULL);
 		if (ret < 0)
 			goto err_arch_supported;
 		hp_online = ret;

[patch V2 09/24] hwtracing/coresight-etm4x: Use cpuhp_setup_state_nocalls_cpuslocked()

[Thomas Gleixner]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

etm_probe4() holds get_online_cpus() while invoking
cpuhp_setup_state_nocalls().

cpuhp_setup_state_nocalls() invokes get_online_cpus() as well. This is
correct, but prevents the conversion of the hotplug locking to a percpu
rwsem.

Use cpuhp_setup_state_nocalls_cpuslocked() to avoid the nested call.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: linux-arm-kernel at lists.infradead.org

---
 drivers/hwtracing/coresight/coresight-etm4x.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -990,12 +990,12 @@ static int etm4_probe(struct amba_device
 		dev_err(dev, "ETM arch init failed\n");
 
 	if (!etm4_count++) {
-		cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
-					  "arm/coresight4:starting",
-					  etm4_starting_cpu, etm4_dying_cpu);
-		ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-						"arm/coresight4:online",
-						etm4_online_cpu, NULL);
+		cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
+						     "arm/coresight4:starting",
+						     etm4_starting_cpu, etm4_dying_cpu);
+		ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
+							   "arm/coresight4:online",
+							   etm4_online_cpu, NULL);
 		if (ret < 0)
 			goto err_arch_supported;
 		hp_online = ret;

[patch V2 11/24] ARM/hw_breakpoint: Use cpuhp_setup_state_cpuslocked()

[Thomas Gleixner]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

arch_hw_breakpoint_init() holds get_online_cpus() while registerring the
hotplug callbacks.

cpuhp_setup_state() invokes get_online_cpus() as well. This is correct, but
prevents the conversion of the hotplug locking to a percpu rwsem.

Use cpuhp_setup_state_cpuslocked() to avoid the nested call.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel at lists.infradead.org

---
 arch/arm/kernel/hw_breakpoint.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -1098,8 +1098,9 @@ static int __init arch_hw_breakpoint_ini
 	 * assume that a halting debugger will leave the world in a nice state
 	 * for us.
 	 */
-	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
-				dbg_reset_online, NULL);
+	ret = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
+					   "arm/hw_breakpoint:online",
+					   dbg_reset_online, NULL);
 	unregister_undef_hook(&debug_reg_hook);
 	if (WARN_ON(ret < 0) || !cpumask_empty(&debug_err_mask)) {
 		core_num_brps = 0;

[patch V2 11/24] ARM/hw_breakpoint: Use cpuhp_setup_state_cpuslocked()

[Mark Rutland]

Hi,

On Tue, Apr 18, 2017 at 07:04:53PM +0200, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> arch_hw_breakpoint_init() holds get_online_cpus() while registerring the
> hotplug callbacks.
> 
> cpuhp_setup_state() invokes get_online_cpus() as well. This is correct, but
> prevents the conversion of the hotplug locking to a percpu rwsem.
> 
> Use cpuhp_setup_state_cpuslocked() to avoid the nested call.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-arm-kernel at lists.infradead.org
> 
> ---
>  arch/arm/kernel/hw_breakpoint.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -1098,8 +1098,9 @@ static int __init arch_hw_breakpoint_ini
>  	 * assume that a halting debugger will leave the world in a nice state
>  	 * for us.
>  	 */
> -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
> -				dbg_reset_online, NULL);
> +	ret = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> +					   "arm/hw_breakpoint:online",
> +					   dbg_reset_online, NULL);

Given the callsite, this particular change looks ok to me. So FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

However, as a more general note, the changes make the API feel odd. per
their current names, {get,put}_online_cpus() sound/feel like refcounting
ops, which should be able to nest.

Is there any chance these could get a better name, e.g.
{lock,unlock}_online_cpus(), so as to align with _cpuslocked?

Thanks,
Mark.

[patch V2 11/24] ARM/hw_breakpoint: Use cpuhp_setup_state_cpuslocked()

[Thomas Gleixner]

On Wed, 19 Apr 2017, Mark Rutland wrote:

> Hi,
> 
> On Tue, Apr 18, 2017 at 07:04:53PM +0200, Thomas Gleixner wrote:
> > From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > 
> > arch_hw_breakpoint_init() holds get_online_cpus() while registerring the
> > hotplug callbacks.
> > 
> > cpuhp_setup_state() invokes get_online_cpus() as well. This is correct, but
> > prevents the conversion of the hotplug locking to a percpu rwsem.
> > 
> > Use cpuhp_setup_state_cpuslocked() to avoid the nested call.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: linux-arm-kernel at lists.infradead.org
> > 
> > ---
> >  arch/arm/kernel/hw_breakpoint.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > --- a/arch/arm/kernel/hw_breakpoint.c
> > +++ b/arch/arm/kernel/hw_breakpoint.c
> > @@ -1098,8 +1098,9 @@ static int __init arch_hw_breakpoint_ini
> >  	 * assume that a halting debugger will leave the world in a nice state
> >  	 * for us.
> >  	 */
> > -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
> > -				dbg_reset_online, NULL);
> > +	ret = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> > +					   "arm/hw_breakpoint:online",
> > +					   dbg_reset_online, NULL);
> 
> Given the callsite, this particular change looks ok to me. So FWIW:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> However, as a more general note, the changes make the API feel odd. per
> their current names, {get,put}_online_cpus() sound/feel like refcounting
> ops, which should be able to nest.
> 
> Is there any chance these could get a better name, e.g.
> {lock,unlock}_online_cpus(), so as to align with _cpuslocked?

Yes, that's a follow up cleanup patch treewide once this hit Linus tree.

Thanks,

	tglx

[patch V2 08/24] hwtracing/coresight-etm3x: Use cpuhp_setup_state_nocalls_cpuslocked()

[Mathieu Poirier]

On 18 April 2017 at 11:04, Thomas Gleixner <tglx@linutronix.de> wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> etm_probe() holds get_online_cpus() while invoking
> cpuhp_setup_state_nocalls().
>
> cpuhp_setup_state_nocalls() invokes get_online_cpus() as well. This is
> correct, but prevents the conversion of the hotplug locking to a percpu
> rwsem.
>
> Use cpuhp_setup_state_nocalls_cpuslocked() to avoid the nested call.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: linux-arm-kernel at lists.infradead.org
>
> ---
>  drivers/hwtracing/coresight/coresight-etm3x.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> --- a/drivers/hwtracing/coresight/coresight-etm3x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c
> @@ -803,12 +803,12 @@ static int etm_probe(struct amba_device
>                 dev_err(dev, "ETM arch init failed\n");
>
>         if (!etm_count++) {
> -               cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
> -                                         "arm/coresight:starting",
> -                                         etm_starting_cpu, etm_dying_cpu);
> -               ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> -                                               "arm/coresight:online",
> -                                               etm_online_cpu, NULL);
> +               cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
> +                                                    "arm/coresight:starting",
> +                                                    etm_starting_cpu, etm_dying_cpu);
> +               ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
> +                                                          "arm/coresight:online",
> +                                                          etm_online_cpu, NULL);
>                 if (ret < 0)
>                         goto err_arch_supported;
>                 hp_online = ret;
>

Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

[patch V2 08/24] hwtracing/coresight-etm3x: Use cpuhp_setup_state_nocalls_cpuslocked()

[Mathieu Poirier]

On Tue, Apr 18, 2017 at 07:04:50PM +0200, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> etm_probe() holds get_online_cpus() while invoking
> cpuhp_setup_state_nocalls().
> 
> cpuhp_setup_state_nocalls() invokes get_online_cpus() as well. This is
> correct, but prevents the conversion of the hotplug locking to a percpu
> rwsem.
> 
> Use cpuhp_setup_state_nocalls_cpuslocked() to avoid the nested call.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: linux-arm-kernel at lists.infradead.org
> 
> ---
>  drivers/hwtracing/coresight/coresight-etm3x.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> --- a/drivers/hwtracing/coresight/coresight-etm3x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c
> @@ -803,12 +803,12 @@ static int etm_probe(struct amba_device
>  		dev_err(dev, "ETM arch init failed\n");
>  
>  	if (!etm_count++) {
> -		cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
> -					  "arm/coresight:starting",
> -					  etm_starting_cpu, etm_dying_cpu);
> -		ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> -						"arm/coresight:online",
> -						etm_online_cpu, NULL);
> +		cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
> +						     "arm/coresight:starting",
> +						     etm_starting_cpu, etm_dying_cpu);
> +		ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
> +							   "arm/coresight:online",
> +							   etm_online_cpu, NULL);

Checkpatch complains about lines being over 80 characters long.  It is better to
keep the above or do as follow and make checkpatch happy?

                cpuhp_setup_state_nocalls_cpuslocked(
                                        PUHP_AP_ARM_CORESIGHT_STARTING,
                                        "arm/coresight:starting",
                                        etm_starting_cpu, etm_dying_cpu);
                ret = cpuhp_setup_state_nocalls_cpuslocked(
                                        CPUHP_AP_ONLINE_DYN,
                                        "arm/coresight:online",
                                        etm_online_cpu, NULL);

Same for coresight-etm4x.c

Thanks,
Mathieu

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Mark Rutland]

Hi,

This series appears to break boot on some arm64 platforms, seen with
next-20170424. More info below.

On Tue, Apr 18, 2017 at 07:04:42PM +0200, Thomas Gleixner wrote:
> get_online_cpus() is used in hot pathes in mainline and even more so in
> RT. That can show up badly under certain conditions because every locker
> contends on a global mutex. RT has it's own homebrewn mitigation which is
> an (badly done) open coded implementation of percpu_rwsems with recursion
> support.
> 
> The proper replacement for that are percpu_rwsems, but that requires to
> remove recursion support.
> 
> The conversion unearthed real locking issues which were previously not
> visible because the get_online_cpus() lockdep annotation was implemented
> with recursion support which prevents lockdep from tracking full dependency
> chains. These potential deadlocks are not related to recursive calls, they
> trigger on the first invocation because lockdep now has the full dependency
> chains available.

Catalin spotted next-20170424 wouldn't boot on a Juno system, where we see the
following splat (repeated forever) when we try to bring up the first secondary
CPU:

[    0.213406] smp: Bringing up secondary CPUs ...
[    0.250326] CPU features: enabling workaround for ARM erratum 832075
[    0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
[    0.250337] Modules linked in:
[    0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
[    0.250349] Hardware name: ARM Juno development board (r1) (DT)
[    0.250353] Call trace:
[    0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
[    0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
[    0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
[    0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
[    0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
[    0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
[    0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
[    0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
[    0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
[    0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
[    0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
[    0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
[    0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
[    0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
[    0.250444] [<000000008093d1b4>] 0x8093d1b4

I can reproduce this with the current head of the linux-tip smp/hotplug
branch (commit 77c60400c82bd993), with arm64 defconfig on a Juno R1
system.

When we bring the secondary CPU online, we detect an erratum that wasn't
present on the boot CPU, and try to enable a static branch we use to
track the erratum. The call to static_branch_enable() blows up as above.

I see that we now have static_branch_disable_cpuslocked(), but we don't
have an equivalent for enable. I'm not sure what we should be doing
here.

Thanks,
Mark.

> The following patch series addresses this by
> 
>  - Cleaning up places which call get_online_cpus() nested
> 
>  - Replacing a few instances with cpu_hotplug_disable() to prevent circular
>    locking dependencies.
> 
> The series depends on
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched/core
>   plus
>     Linus tree merged in to avoid conflicts
> 
> It's available in git from
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.hotplug
> 
> Changes since V1:
> 
>   - Fixed fallout reported by kbuild bot
>   - Repaired the recursive call in perf
>   - Repaired the interaction with jumplabels (Peter Zijlstra)
>   - Renamed _locked to _cpuslocked
>   - Picked up Acked-bys
> 
> Thanks,
> 
> 	tglx
> 
> -------
>  arch/arm/kernel/hw_breakpoint.c               |    5 
>  arch/mips/kernel/jump_label.c                 |    2 
>  arch/powerpc/kvm/book3s_hv.c                  |    8 -
>  arch/powerpc/platforms/powernv/subcore.c      |    3 
>  arch/s390/kernel/time.c                       |    2 
>  arch/x86/events/core.c                        |    1 
>  arch/x86/events/intel/cqm.c                   |   12 -
>  arch/x86/kernel/cpu/mtrr/main.c               |    2 
>  b/arch/sparc/kernel/jump_label.c              |    2 
>  b/arch/tile/kernel/jump_label.c               |    2 
>  b/arch/x86/events/intel/core.c                |    4 
>  b/arch/x86/kernel/jump_label.c                |    2 
>  b/kernel/jump_label.c                         |   31 ++++-
>  drivers/acpi/processor_driver.c               |    4 
>  drivers/cpufreq/cpufreq.c                     |    9 -
>  drivers/hwtracing/coresight/coresight-etm3x.c |   12 -
>  drivers/hwtracing/coresight/coresight-etm4x.c |   12 -
>  drivers/pci/pci-driver.c                      |   47 ++++---
>  include/linux/cpu.h                           |    2 
>  include/linux/cpuhotplug.h                    |   29 ++++
>  include/linux/jump_label.h                    |    3 
>  include/linux/padata.h                        |    3 
>  include/linux/pci.h                           |    1 
>  include/linux/stop_machine.h                  |   26 +++-
>  kernel/cpu.c                                  |  157 ++++++++------------------
>  kernel/events/core.c                          |    9 -
>  kernel/padata.c                               |   39 +++---
>  kernel/stop_machine.c                         |    7 -
>  28 files changed, 228 insertions(+), 208 deletions(-)
> 
> 
> 

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Sebastian Siewior]

On 2017-04-25 17:10:37 [+0100], Mark Rutland wrote:
> Hi,
Hi,

> When we bring the secondary CPU online, we detect an erratum that wasn't
> present on the boot CPU, and try to enable a static branch we use to
> track the erratum. The call to static_branch_enable() blows up as above.

this (cpus_set_cap()) seems only to be used used in CPU up part.

> I see that we now have static_branch_disable_cpuslocked(), but we don't
> have an equivalent for enable. I'm not sure what we should be doing
> here.

We should introduce static_branch_enable_cpuslocked(). Does this work
for you (after s/static_branch_enable/static_branch_enable_cpuslocked/
in cpus_set_cap()) ?:

>From fe004351e4649da037d5165247e89ab976fe4a26 Mon Sep 17 00:00:00 2001
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Tue, 25 Apr 2017 18:43:00 +0200
Subject: [PATCH] jump_label: Provide
 static_key_[enable|/slow_inc]_cpuslocked()

Provide static_key_[enable|slow_inc]_cpuslocked() variant that
don't take cpu_hotplug_lock().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/jump_label.h |  7 +++++++
 kernel/jump_label.c        | 10 ++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index d7b17d1ab875..c80d8b1279b5 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -164,6 +164,7 @@ extern void static_key_slow_dec_cpuslocked(struct static_key *key);
 extern void jump_label_apply_nops(struct module *mod);
 extern int static_key_count(struct static_key *key);
 extern void static_key_enable(struct static_key *key);
+extern void static_key_enable_cpuslocked(struct static_key *key);
 extern void static_key_disable(struct static_key *key);
 extern void static_key_disable_cpuslocked(struct static_key *key);
 
@@ -252,6 +253,11 @@ static inline void static_key_enable(struct static_key *key)
 		static_key_slow_inc(key);
 }
 
+static inline void static_key_enable_cpuslocked(struct static_key *key)
+{
+	static_key_enable(key);
+}
+
 static inline void static_key_disable(struct static_key *key)
 {
 	int count = static_key_count(key);
@@ -429,6 +435,7 @@ extern bool ____wrong_branch_error(void);
  */
 
 #define static_branch_enable(x)			static_key_enable(&(x)->key)
+#define static_branch_enable_cpuslocked(x)	static_key_enable_cpuslocked(&(x)->key)
 #define static_branch_disable(x)		static_key_disable(&(x)->key)
 #define static_branch_disable_cpuslocked(x)	static_key_disable_cpuslocked(&(x)->key)
 
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d71124ee3b14..6343f4c7e27f 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -90,6 +90,16 @@ void static_key_enable(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_enable);
 
+void static_key_enable_cpuslocked(struct static_key *key)
+{
+	int count = static_key_count(key);
+
+	WARN_ON_ONCE(count < 0 || count > 1);
+
+	if (!count)
+		static_key_slow_inc_cpuslocked(key);
+}
+
 void static_key_disable(struct static_key *key)
 {
 	int count = static_key_count(key);
-- 
2.11.0

> Thanks,
> Mark.

Sebastian

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Mark Rutland]

On Tue, Apr 25, 2017 at 07:28:38PM +0200, Sebastian Siewior wrote:
> On 2017-04-25 17:10:37 [+0100], Mark Rutland wrote:
> > When we bring the secondary CPU online, we detect an erratum that wasn't
> > present on the boot CPU, and try to enable a static branch we use to
> > track the erratum. The call to static_branch_enable() blows up as above.
> 
> this (cpus_set_cap()) seems only to be used used in CPU up part.
> 
> > I see that we now have static_branch_disable_cpuslocked(), but we don't
> > have an equivalent for enable. I'm not sure what we should be doing
> > here.
> 
> We should introduce static_branch_enable_cpuslocked(). Does this work
> for you (after s/static_branch_enable/static_branch_enable_cpuslocked/
> in cpus_set_cap()) ?:

The patch you linked worked for me, given the below patch for arm64 to
make use of static_branch_enable_cpuslocked().

Catalin/Will, are you happy for this to go via the tip tree with the
other hotplug locking changes?

Thanks,
Mark.

---->8----
>From 03c98baf0a9fcdfb87145bfb3f108d49a721dad6 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Wed, 26 Apr 2017 09:46:47 +0100
Subject: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()

Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
the existing {get,put}_online_cpus() logic, this can't nest.
Unfortunately, in arm64's secondary boot path we can end up nesting via
static_branch_enable() in cpus_set_cap() when we detect an erratum.

This leads to a stream of messages as below, where the secondary
attempts to schedule befroe it has been fully onlined. As the CPU
orchsetrating the onlining holds the rswem, this hangs the system.

[    0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
[    0.250337] Modules linked in:
[    0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
[    0.250349] Hardware name: ARM Juno development board (r1) (DT)
[    0.250353] Call trace:
[    0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
[    0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
[    0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
[    0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
[    0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
[    0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
[    0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
[    0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
[    0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
[    0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
[    0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
[    0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
[    0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
[    0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
[    0.250444] [<000000008093d1b4>] 0x8093d1b4

We only call cpus_set_cap() in the secondary boot path, where we know
that the rwsem is held by the thread orchestrating the onlining. Thus,
we can use the new static_branch_enable_cpuslocked() in cpus_set_cap(),
avoiding the above.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki Poulose <suzuki,poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index f31c48d..349b5cd 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
 			num, ARM64_NCAPS);
 	} else {
 		__set_bit(num, cpu_hwcaps);
-		static_branch_enable(&cpu_hwcap_keys[num]);
+		static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
 	}
 }
 
-- 
1.9.1

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Suzuki K Poulose]

On 26/04/17 09:59, Mark Rutland wrote:
> On Tue, Apr 25, 2017 at 07:28:38PM +0200, Sebastian Siewior wrote:
>> On 2017-04-25 17:10:37 [+0100], Mark Rutland wrote:
>>> When we bring the secondary CPU online, we detect an erratum that wasn't
>>> present on the boot CPU, and try to enable a static branch we use to
>>> track the erratum. The call to static_branch_enable() blows up as above.
>>
>> this (cpus_set_cap()) seems only to be used used in CPU up part.
>>
>>> I see that we now have static_branch_disable_cpuslocked(), but we don't
>>> have an equivalent for enable. I'm not sure what we should be doing
>>> here.
>>
>> We should introduce static_branch_enable_cpuslocked(). Does this work
>> for you (after s/static_branch_enable/static_branch_enable_cpuslocked/
>> in cpus_set_cap()) ?:
>
> The patch you linked worked for me, given the below patch for arm64 to
> make use of static_branch_enable_cpuslocked().
>
> Catalin/Will, are you happy for this to go via the tip tree with the
> other hotplug locking changes?
>
> Thanks,
> Mark.
>
> ---->8----
> From 03c98baf0a9fcdfb87145bfb3f108d49a721dad6 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Wed, 26 Apr 2017 09:46:47 +0100
> Subject: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()
>
> Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
> the existing {get,put}_online_cpus() logic, this can't nest.
> Unfortunately, in arm64's secondary boot path we can end up nesting via
> static_branch_enable() in cpus_set_cap() when we detect an erratum.
>
> This leads to a stream of messages as below, where the secondary
> attempts to schedule befroe it has been fully onlined. As the CPU

nit: s/befroe/before/

> orchsetrating the onlining holds the rswem, this hangs the system.
s/orchsetrating/orchestrating/

>
> [    0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
> [    0.250337] Modules linked in:
> [    0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
> [    0.250349] Hardware name: ARM Juno development board (r1) (DT)
> [    0.250353] Call trace:
> [    0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
> [    0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
> [    0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
> [    0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
> [    0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
> [    0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
> [    0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
> [    0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
> [    0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
> [    0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
> [    0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
> [    0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
> [    0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
> [    0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
> [    0.250444] [<000000008093d1b4>] 0x8093d1b4
>
> We only call cpus_set_cap() in the secondary boot path, where we know
> that the rwsem is held by the thread orchestrating the onlining. Thus,
> we can use the new static_branch_enable_cpuslocked() in cpus_set_cap(),
> avoiding the above.

Correction, we could call cpus_set_cap() from setup_cpu_features()
for updating the system global capabilities, where we may not hold the
cpu_hotplug_lock. So we could end up calling static_branch_enable_cpuslocked()
without actually holding the lock. Should we do a cpu_hotplug_begin/done in
setup_cpu_feature_capabilities ? I agree it doesn't look that nice. Thoughts ?

Suzuki

> 		
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki Poulose <suzuki,poulose@arm.com>
> ---
>  arch/arm64/include/asm/cpufeature.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index f31c48d..349b5cd 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
>  			num, ARM64_NCAPS);
>  	} else {
>  		__set_bit(num, cpu_hwcaps);
> -		static_branch_enable(&cpu_hwcap_keys[num]);
> +		static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
>  	}
>  }
>
>

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Mark Rutland]

On Wed, Apr 26, 2017 at 10:40:47AM +0100, Suzuki K Poulose wrote:
> On 26/04/17 09:59, Mark Rutland wrote:

> >We only call cpus_set_cap() in the secondary boot path, where we know
> >that the rwsem is held by the thread orchestrating the onlining. Thus,
> >we can use the new static_branch_enable_cpuslocked() in cpus_set_cap(),
> >avoiding the above.
> 
> Correction, we could call cpus_set_cap() from setup_cpu_features()
> for updating the system global capabilities, where we may not hold the
> cpu_hotplug_lock. 

Argh, yes, I missed that when scanning.

> So we could end up calling static_branch_enable_cpuslocked()
> without actually holding the lock. Should we do a cpu_hotplug_begin/done in
> setup_cpu_feature_capabilities ? I agree it doesn't look that nice. Thoughts ?

I agree that's hideous, but it looks like the only choice given the
hotplug rwsem cahnges. :/

I can spin a v2 with that and the typo fixes.

Thanks,
Mark.

> 
> Suzuki
> 
> >		
> >Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> >Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> >Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >Cc: Will Deacon <will.deacon@arm.com>
> >Cc: Suzuki Poulose <suzuki,poulose@arm.com>
> >---
> > arch/arm64/include/asm/cpufeature.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> >index f31c48d..349b5cd 100644
> >--- a/arch/arm64/include/asm/cpufeature.h
> >+++ b/arch/arm64/include/asm/cpufeature.h
> >@@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
> > 			num, ARM64_NCAPS);
> > 	} else {
> > 		__set_bit(num, cpu_hwcaps);
> >-		static_branch_enable(&cpu_hwcap_keys[num]);
> >+		static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
> > 	}
> > }
> >
> >
> 

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Sebastian Siewior]

On 2017-04-26 11:32:36 [+0100], Mark Rutland wrote:
> > So we could end up calling static_branch_enable_cpuslocked()
> > without actually holding the lock. Should we do a cpu_hotplug_begin/done in
> > setup_cpu_feature_capabilities ? I agree it doesn't look that nice. Thoughts ?
> 
> I agree that's hideous, but it looks like the only choice given the
> hotplug rwsem cahnges. :/

would work for you to provide a locked and unlocked version?

> I can spin a v2 with that and the typo fixes.
> 
> Thanks,
> Mark.

Sebastian

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Mark Rutland]

On Thu, Apr 27, 2017 at 10:27:20AM +0200, Sebastian Siewior wrote:
> On 2017-04-26 11:32:36 [+0100], Mark Rutland wrote:
> > > So we could end up calling static_branch_enable_cpuslocked()
> > > without actually holding the lock. Should we do a cpu_hotplug_begin/done in
> > > setup_cpu_feature_capabilities ? I agree it doesn't look that nice. Thoughts ?
> > 
> > I agree that's hideous, but it looks like the only choice given the
> > hotplug rwsem cahnges. :/
> 
> would work for you to provide a locked and unlocked version?

Maybe. Today we have:

// rwsem unlocked
start_kernel()
->smp_prepare_boot_cpu()
-->update_cpu_errata_workarounds()
--->update_cpu_capabilities()

// rwsem locked (by other CPU)
secondary_start_kernel()
->check_local_cpu_capabilities()
-->update_cpu_errata_workarounds()
--->update_cpu_capabilities() 

With the common chain:

update_cpu_capabilities()
->cpus_set_cap()
-->static_branch_enable()

... so we could add a update_cpu_capabilities{,_cpuslocked}(), and say
that cpus_set_cap() expects the hotplug rswem to be locked, as per the
below diff.

Thoughts?

Mark.

---->8----

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index f31c48d..7341579 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
 			num, ARM64_NCAPS);
 	} else {
 		__set_bit(num, cpu_hwcaps);
-		static_branch_enable(&cpu_hwcap_keys[num]);
+		static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
 	}
 }
 
@@ -217,8 +217,22 @@ static inline bool id_aa64pfr0_32bit_el0(u64 pfr0)
 
 void __init setup_cpu_features(void);
 
-void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
-			    const char *info);
+void __update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
+			       const char *info, bool cpuslocked);
+static inline void
+update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
+			  const char *info)
+{
+	__update_cpu_capabilities(caps, info, false);
+}
+
+static inline void
+update_cpu_capabilities_cpuslocked(const struct arm64_cpu_capabilities *caps,
+			  const char *info)
+{
+	__update_cpu_capabilities(caps, info, true);
+}
+
 void enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps);
 void check_local_cpu_capabilities(void);
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index abda8e8..ae8ddc1 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -956,8 +956,8 @@ static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps)
 			cap_set_elf_hwcap(hwcaps);
 }
 
-void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
-			    const char *info)
+void __update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
+			       const char *info, bool cpuslocked)
 {
 	for (; caps->matches; caps++) {
 		if (!caps->matches(caps, caps->def_scope))
@@ -965,7 +965,14 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
 
 		if (!cpus_have_cap(caps->capability) && caps->desc)
 			pr_info("%s %s\n", info, caps->desc);
-		cpus_set_cap(caps->capability);
+
+		if (cpuslocked) {
+			cpus_set_cap(caps->capability);
+		} else {
+			get_online_cpus();
+			cpus_set_cap(caps->capability);
+			put_online_cpus();
+		}
 	}
 }
 

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Thomas Gleixner]

On Thu, 27 Apr 2017, Mark Rutland wrote:

> On Thu, Apr 27, 2017 at 10:27:20AM +0200, Sebastian Siewior wrote:
> > On 2017-04-26 11:32:36 [+0100], Mark Rutland wrote:
> > > > So we could end up calling static_branch_enable_cpuslocked()
> > > > without actually holding the lock. Should we do a cpu_hotplug_begin/done in
> > > > setup_cpu_feature_capabilities ? I agree it doesn't look that nice. Thoughts ?
> > > 
> > > I agree that's hideous, but it looks like the only choice given the
> > > hotplug rwsem cahnges. :/
> > 
> > would work for you to provide a locked and unlocked version?
> 
> Maybe. Today we have:
> 
> // rwsem unlocked
> start_kernel()
> ->smp_prepare_boot_cpu()
> -->update_cpu_errata_workarounds()
> --->update_cpu_capabilities()
> 
> // rwsem locked (by other CPU)
> secondary_start_kernel()
> ->check_local_cpu_capabilities()
> -->update_cpu_errata_workarounds()
> --->update_cpu_capabilities() 
> 
> With the common chain:
> 
> update_cpu_capabilities()
> ->cpus_set_cap()
> -->static_branch_enable()
> 
> ... so we could add a update_cpu_capabilities{,_cpuslocked}(), and say
> that cpus_set_cap() expects the hotplug rswem to be locked, as per the
> below diff.

You just can take the rwsen in smp_prepare_boot_cpu(), so you don't need
that conditional thingy at all. Hmm?

Thanks,

	tglx

[patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem

[Mark Rutland]

On Thu, Apr 27, 2017 at 12:01:57PM +0200, Thomas Gleixner wrote:
> On Thu, 27 Apr 2017, Mark Rutland wrote:
> 
> > On Thu, Apr 27, 2017 at 10:27:20AM +0200, Sebastian Siewior wrote:
> > > On 2017-04-26 11:32:36 [+0100], Mark Rutland wrote:
> > > > > So we could end up calling static_branch_enable_cpuslocked()
> > > > > without actually holding the lock. Should we do a cpu_hotplug_begin/done in
> > > > > setup_cpu_feature_capabilities ? I agree it doesn't look that nice. Thoughts ?
> > > > 
> > > > I agree that's hideous, but it looks like the only choice given the
> > > > hotplug rwsem cahnges. :/
> > > 
> > > would work for you to provide a locked and unlocked version?
> > 
> > Maybe. Today we have:
> > 
> > // rwsem unlocked
> > start_kernel()
> > ->smp_prepare_boot_cpu()
> > -->update_cpu_errata_workarounds()
> > --->update_cpu_capabilities()
> > 
> > // rwsem locked (by other CPU)
> > secondary_start_kernel()
> > ->check_local_cpu_capabilities()
> > -->update_cpu_errata_workarounds()
> > --->update_cpu_capabilities() 
> > 
> > With the common chain:
> > 
> > update_cpu_capabilities()
> > ->cpus_set_cap()
> > -->static_branch_enable()
> > 
> > ... so we could add a update_cpu_capabilities{,_cpuslocked}(), and say
> > that cpus_set_cap() expects the hotplug rswem to be locked, as per the
> > below diff.
> 
> You just can take the rwsen in smp_prepare_boot_cpu(), so you don't need
> that conditional thingy at all. Hmm?

True.

Given it's a bit further up the callchain, it's probably worth a
comment, but it will work.

I'll spin a v3 to that effect shortly.

Thanks,
Mark.

[PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked() (was: Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem)

[Mark Rutland]

Hi Catalin/Will,

The below addresses a boot failure Catalin spotted in next-20170424,
based on Sebastian's patch [1]. I've given it a spin on Juno R1, where I
can reproduce the issue prior to applying this patch.

I believe this would need to go via tip, as the issue is a result of
change in the tip smp/hotplug branch, and the fix depends on
infrastructure introduced there.

Are you happy with the fix, and for it to go via the tip tree?

Thanks,
Mark.

[1] https://lkml.kernel.org/r/20170425172838.mr3kyccsdteyjso5 at linutronix.de
[2] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=smp/hotplug

---->8----
>From 6cdb503b060f74743769c9f601c35f985d3c58eb Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Wed, 26 Apr 2017 09:46:47 +0100
Subject: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()

Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
the existing {get,put}_online_cpus() logic, this can't nest.
Unfortunately, in arm64's secondary boot path we can end up nesting via
static_branch_enable() in cpus_set_cap() when we detect an erratum.

This leads to a stream of messages as below, where the secondary
attempts to schedule before it has been fully onlined. As the CPU
orchestrating the onlining holds the rswem, this hangs the system.

[    0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
[    0.250337] Modules linked in:
[    0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
[    0.250349] Hardware name: ARM Juno development board (r1) (DT)
[    0.250353] Call trace:
[    0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
[    0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
[    0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
[    0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
[    0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
[    0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
[    0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
[    0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
[    0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
[    0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
[    0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
[    0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
[    0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
[    0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
[    0.250444] [<000000008093d1b4>] 0x8093d1b4

We call cpus_set_cap() from update_cpu_capabilities(), which is called
from the secondary boot path (where the CPU orchestrating the onlining
holds the hotplug rwsem), and in the primary boot path, where this is
not held.

This patch makes cpus_set_cap() use static_branch_enable_cpuslocked(),
and updates the primary CPU boot path to hold the rwsem so as to keep
the *_cpuslocked() code happy.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki Poulose <suzuki,poulose@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/cpufeature.h | 2 +-
 arch/arm64/kernel/smp.c             | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index f31c48d..349b5cd 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
 			num, ARM64_NCAPS);
 	} else {
 		__set_bit(num, cpu_hwcaps);
-		static_branch_enable(&cpu_hwcap_keys[num]);
+		static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
 	}
 }
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 9b10365..c2ce9aa 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -447,11 +447,13 @@ void __init smp_prepare_boot_cpu(void)
 	cpuinfo_store_boot_cpu();
 	save_boot_cpu_run_el();
 	/*
-	 * Run the errata work around checks on the boot CPU, once we have
-	 * initialised the cpu feature infrastructure from
-	 * cpuinfo_store_boot_cpu() above.
+	 * Run the errata work around checks on the boot CPU, now that
+	 * cpuinfo_store_boot_cpu() has set things up. We hold the percpu rwsem
+	 * to keep the workaround setup code happy.
 	 */
+	get_online_cpus();
 	update_cpu_errata_workarounds();
+	put_online_cpus();
 }
 
 static u64 __init of_get_cpu_mpidr(struct device_node *dn)
-- 
1.9.1

[PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked() (was: Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem)

[Suzuki K Poulose]

On Thu, Apr 27, 2017 at 04:48:06PM +0100, Mark Rutland wrote:
> Hi Catalin/Will,
> 
> The below addresses a boot failure Catalin spotted in next-20170424,
> based on Sebastian's patch [1]. I've given it a spin on Juno R1, where I
> can reproduce the issue prior to applying this patch.
> 
> I believe this would need to go via tip, as the issue is a result of
> change in the tip smp/hotplug branch, and the fix depends on
> infrastructure introduced there.
> 
> Are you happy with the fix, and for it to go via the tip tree?
> 
> Thanks,
> Mark.
> 
> [1] https://lkml.kernel.org/r/20170425172838.mr3kyccsdteyjso5 at linutronix.de
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=smp/hotplug
> 
> ---->8----
> From 6cdb503b060f74743769c9f601c35f985d3c58eb Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Wed, 26 Apr 2017 09:46:47 +0100
> Subject: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()
> 
> Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
> the existing {get,put}_online_cpus() logic, this can't nest.
> Unfortunately, in arm64's secondary boot path we can end up nesting via
> static_branch_enable() in cpus_set_cap() when we detect an erratum.
> 
> This leads to a stream of messages as below, where the secondary
> attempts to schedule before it has been fully onlined. As the CPU
> orchestrating the onlining holds the rswem, this hangs the system.
> 
> [    0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
> [    0.250337] Modules linked in:
> [    0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
> [    0.250349] Hardware name: ARM Juno development board (r1) (DT)
> [    0.250353] Call trace:
> [    0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
> [    0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
> [    0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
> [    0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
> [    0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
> [    0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
> [    0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
> [    0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
> [    0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
> [    0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
> [    0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
> [    0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
> [    0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
> [    0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
> [    0.250444] [<000000008093d1b4>] 0x8093d1b4
> 
> We call cpus_set_cap() from update_cpu_capabilities(), which is called
> from the secondary boot path (where the CPU orchestrating the onlining
> holds the hotplug rwsem), and in the primary boot path, where this is
> not held.
> 
> This patch makes cpus_set_cap() use static_branch_enable_cpuslocked(),
> and updates the primary CPU boot path to hold the rwsem so as to keep
> the *_cpuslocked() code happy.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki Poulose <suzuki,poulose@arm.com>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/include/asm/cpufeature.h | 2 +-
>  arch/arm64/kernel/smp.c             | 8 +++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index f31c48d..349b5cd 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
>  			num, ARM64_NCAPS);
>  	} else {
>  		__set_bit(num, cpu_hwcaps);
> -		static_branch_enable(&cpu_hwcap_keys[num]);
> +		static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
>  	}
>  }
>  
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 9b10365..c2ce9aa 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -447,11 +447,13 @@ void __init smp_prepare_boot_cpu(void)
>  	cpuinfo_store_boot_cpu();
>  	save_boot_cpu_run_el();
>  	/*
> -	 * Run the errata work around checks on the boot CPU, once we have
> -	 * initialised the cpu feature infrastructure from
> -	 * cpuinfo_store_boot_cpu() above.
> +	 * Run the errata work around checks on the boot CPU, now that
> +	 * cpuinfo_store_boot_cpu() has set things up. We hold the percpu rwsem
> +	 * to keep the workaround setup code happy.
>  	 */
> +	get_online_cpus();
>  	update_cpu_errata_workarounds();
> +	put_online_cpus();

We need similar locking for updat_cpu_capabilities() called
from setup_cpu_feature_capabilities(). i.e., the following fixup is required.

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 94b8f7f..62bdab4 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -966,6 +966,7 @@ static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps)
 			cap_set_elf_hwcap(hwcaps);
 }
 
+/* Should be called with CPU hotplug lock held */
 void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
 			    const char *info)
 {
@@ -1092,7 +1093,9 @@ void check_local_cpu_capabilities(void)
 
 static void __init setup_feature_capabilities(void)
 {
-	update_cpu_capabilities(arm64_features, "detected feature:");
+	get_online_cpus();
+	update_cpu_capabilities(arm6_features, "detected feature:");
+	put_online_cpus();
 	enable_cpu_capabilities(arm64_features);
 }
 
-- 
---

Also, I think having update_cpu_errata_workarounds() called with and without
the CPU hotplug lock makes it a bit confusing without proper explanation.
How about the following patch which makes things a bit more reader friendly ?

---8>---

>From f3b0809224e4915197d3ae4a38ebe7f210e74abf Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Thu, 27 Apr 2017 16:48:06 +0100
Subject: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()

Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
the existing {get,put}_online_cpus() logic, this can't nest.
Unfortunately, in arm64's secondary boot path we can end up nesting via
static_branch_enable() in cpus_set_cap() when we detect an erratum.

This leads to a stream of messages as below, where the secondary
attempts to schedule before it has been fully onlined. As the CPU
orchestrating the onlining holds the rswem, this hangs the system.

[    0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
[    0.250337] Modules linked in:
[    0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
[    0.250349] Hardware name: ARM Juno development board (r1) (DT)
[    0.250353] Call trace:
[    0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
[    0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
[    0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
[    0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
[    0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
[    0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
[    0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
[    0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
[    0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
[    0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
[    0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
[    0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
[    0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
[    0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
[    0.250444] [<000000008093d1b4>] 0x8093d1b4

We call cpus_set_cap() from update_cpu_capabilities(), which is called
from the secondary boot path (where the CPU orchestrating the onlining
holds the hotplug rwsem), and in the primary boot path, where this is
not held.

This patch makes cpus_set_cap() use static_branch_enable_cpuslocked(),
and updates all the callers of update_cpu_capabilities() consistent with
the change.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki Poulose <suzuki,poulose@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |  5 +++--
 arch/arm64/kernel/cpu_errata.c      | 13 ++++++++++++-
 arch/arm64/kernel/cpufeature.c      |  5 ++++-
 arch/arm64/kernel/smp.c             |  7 +++----
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index e7f84a7..2a832c6 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
 			num, ARM64_NCAPS);
 	} else {
 		__set_bit(num, cpu_hwcaps);
-		static_branch_enable(&cpu_hwcap_keys[num]);
+		static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
 	}
 }
 
@@ -222,7 +222,8 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
 void enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps);
 void check_local_cpu_capabilities(void);
 
-void update_cpu_errata_workarounds(void);
+void update_secondary_cpu_errata_workarounds(void);
+void update_boot_cpu_errata_workarounds(void);
 void __init enable_errata_workarounds(void);
 void verify_local_cpu_errata_workarounds(void);
 
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 2ed2a76..f2d889b 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -190,9 +190,20 @@ void verify_local_cpu_errata_workarounds(void)
 		}
 }
 
-void update_cpu_errata_workarounds(void)
+/*
+ * Secondary CPUs are booted with the waker holding the
+ * CPU hotplug lock, hence we don't need to lock it here again.
+ */
+void update_secondary_cpu_errata_workarounds(void)
+{
+	update_cpu_capabilities(arm64_errata, "enabling workaround for");
+}
+
+void update_boot_cpu_errata_workarounds(void)
 {
+	get_online_cpus();
 	update_cpu_capabilities(arm64_errata, "enabling workaround for");
+	put_online_cpus();
 }
 
 void __init enable_errata_workarounds(void)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 94b8f7f..62bdab4 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -966,6 +966,7 @@ static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps)
 			cap_set_elf_hwcap(hwcaps);
 }
 
+/* Should be called with CPU hotplug lock held */
 void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
 			    const char *info)
 {
@@ -1092,7 +1093,9 @@ void check_local_cpu_capabilities(void)
 
 static void __init setup_feature_capabilities(void)
 {
-	update_cpu_capabilities(arm64_features, "detected feature:");
+	get_online_cpus();
+	update_cpu_capabilities(arm6_features, "detected feature:");
+	put_online_cpus();
 	enable_cpu_capabilities(arm64_features);
 }
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 6e0e16a..51ba91f 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -450,11 +450,10 @@ void __init smp_prepare_boot_cpu(void)
 	cpuinfo_store_boot_cpu();
 	save_boot_cpu_run_el();
 	/*
-	 * Run the errata work around checks on the boot CPU, once we have
-	 * initialised the cpu feature infrastructure from
-	 * cpuinfo_store_boot_cpu() above.
+	 * Run the errata work around checks on the boot CPU, now that
+	 * cpuinfo_store_boot_cpu() has set things up.
 	 */
-	update_cpu_errata_workarounds();
+	update_boot_cpu_errata_workarounds();
 }
 
 static u64 __init of_get_cpu_mpidr(struct device_node *dn)
-- 
2.7.4

[PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()

[Suzuki K Poulose]

On 27/04/17 17:35, Suzuki K Poulose wrote:
> rom f3b0809224e4915197d3ae4a38ebe7f210e74abf Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Thu, 27 Apr 2017 16:48:06 +0100
> Subject: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()
>

Build break alert. There are some issues with patch below.

>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki Poulose <suzuki,poulose@arm.com>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/cpufeature.h |  5 +++--
>  arch/arm64/kernel/cpu_errata.c      | 13 ++++++++++++-
>  arch/arm64/kernel/cpufeature.c      |  5 ++++-
>  arch/arm64/kernel/smp.c             |  7 +++----
>  4 files changed, 22 insertions(+), 8 deletions(-)
>


>  void __init enable_errata_workarounds(void)
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 94b8f7f..62bdab4 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -966,6 +966,7 @@ static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps)
>  			cap_set_elf_hwcap(hwcaps);
>  }
>
> +/* Should be called with CPU hotplug lock held */
>  void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
>  			    const char *info)
>  {
> @@ -1092,7 +1093,9 @@ void check_local_cpu_capabilities(void)
>
>  static void __init setup_feature_capabilities(void)
>  {
> -	update_cpu_capabilities(arm64_features, "detected feature:");
> +	get_online_cpus();
> +	update_cpu_capabilities(arm6_features, "detected feature:");

s/arm6_features/arm64_features

And we need the following hunk:

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 62bdab4..19c359a 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1086,7 +1086,7 @@ void check_local_cpu_capabilities(void)
          * advertised capabilities.
          */
         if (!sys_caps_initialised)
-               update_cpu_errata_workarounds();
+               update_secondary_cpu_errata_workarounds();
         else
                 verify_local_cpu_capabilities();
  }

Sorry about that.

Suzuki

[PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()

[Mark Rutland]

On Thu, Apr 27, 2017 at 06:03:35PM +0100, Suzuki K Poulose wrote:
> On 27/04/17 17:35, Suzuki K Poulose wrote:
> >@@ -1092,7 +1093,9 @@ void check_local_cpu_capabilities(void)
> >
> > static void __init setup_feature_capabilities(void)
> > {
> >-	update_cpu_capabilities(arm64_features, "detected feature:");
> >+	get_online_cpus();
> >+	update_cpu_capabilities(arm6_features, "detected feature:");
> 
> s/arm6_features/arm64_features
> 
> And we need the following hunk:
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 62bdab4..19c359a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1086,7 +1086,7 @@ void check_local_cpu_capabilities(void)
>          * advertised capabilities.
>          */
>         if (!sys_caps_initialised)
> -               update_cpu_errata_workarounds();
> +               update_secondary_cpu_errata_workarounds();
>         else
>                 verify_local_cpu_capabilities();
>  }
> 
> Sorry about that.

No worries; thanks for the fixups.

With those this is working for me, so I'll send this and Sebastian's
patch (with Ccs) as a new series.

> 
> Suzuki