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

Mark Rutland
2017-04-27T09:58:26Z

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