Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630738 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Mark Rutland <mark.rutland@arm.com> - 2017-04-25 18:20 +0200
    Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Sebastian Siewior <bigeasy@linutronix.de> - 2017-04-25 19:30 +0200
      Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Mark Rutland <mark.rutland@arm.com> - 2017-04-26 11:10 +0200
        Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2017-04-26 12:00 +0200
          Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Mark Rutland <mark.rutland@arm.com> - 2017-04-26 12:40 +0200
            Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Sebastian Siewior <bigeasy@linutronix.de> - 2017-04-27 10:30 +0200
              Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Mark Rutland <mark.rutland@arm.com> - 2017-04-27 12:00 +0200
                Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Thomas Gleixner <tglx@linutronix.de> - 2017-04-27 12:10 +0200
                  Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a  percpu_rwsem Mark Rutland <mark.rutland@arm.com> - 2017-04-27 14:40 +0200
                    [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 <mark.rutland@arm.com> - 2017-04-27 17:50 +0200
                      Re: [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 <Suzuki.Poulose@arm.com> - 2017-04-27 18:40 +0200
                        Re: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked() Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2017-04-27 19:10 +0200
                          Re: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked() Mark Rutland <mark.rutland@arm.com> - 2017-04-27 19:20 +0200

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

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(-)
> 
> 
> 

[toc] | [next] | [standalone]

#1630820

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

[toc] | [prev] | [next] | [standalone]

#1631266

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

[toc] | [prev] | [next] | [standalone]

#1631331

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]);
>  	}
>  }
>
>

[toc] | [prev] | [next] | [standalone]

#1631365

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]);
> > 	}
> > }
> >
> >
> 

[toc] | [prev] | [next] | [standalone]

#1631963

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

[toc] | [prev] | [next] | [standalone]

#1632007

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();
+		}
 	}
 }
 

[toc] | [prev] | [next] | [standalone]

#1632011

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

[toc] | [prev] | [next] | [standalone]

#1632071

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1632267 — Re: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()

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

[toc] | [prev] | [next] | [standalone]

#1632270 — Re: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web