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

[Mark Rutland <mark.rutland@arm.com>]

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