Hi all,
On Mon, Mar 1, 2021 at 4:36 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> So this is entirely unrelated to the csky pull request, and is more of
> a generic "the perf CPU hotplug thing seems a complete mess".
>
> The csky thing is just the latest - of many - that have been bitten by
> the mess, and the one that added yet another hotplug state, and
> finally made me go "Let's at least talk about this"
>
> For csky, the problem is this:
>
> On Sat, Feb 27, 2021 at 7:43 PM <guoren@xxxxxxxxxx> wrote:
> >
> > arch/csky patches for 5.12-rc1
> >
> > - Fixup perf probe failed
>
> and in this case it is 398cb92495cc ("csky: Fixup perf probe failed")
> in my current -git tree.
>
> But it's also
>
> cf6acb8bdb1d ("s390/cpumf: Add support for complete counter set extraction")
> dcb5cdf60a1f ("powerpc/perf/hv-gpci: Add cpu hotplug support")
> 1a8f0886a600 ("powerpc/perf/hv-24x7: Add cpu hotplug support")
> 6b7ce8927b5a ("irqchip: RISC-V per-HART local interrupt controller driver")
> e9b880581d55 ("coresight: cti: Add CPU Hotplug handling to CTI driver")
> e0685fa228fd ("arm64: Retrieve stolen time as paravirtualized guest")
> 6282edb72bed ("clocksource/drivers/exynos_mct: Increase priority
> over ARM arch timer")
> 78f4e932f776 ("x86/microcode, cpuhotplug: Add a microcode loader
> CPU hotplug callback")
> 72c69dcddce1 ("powerpc/perf: Trace imc events detection and cpuhotplug")
> 5861381d4866 ("PM / arch: x86: Rework the
> MSR_IA32_ENERGY_PERF_BIAS handling")
> 69c32972d593 ("drivers/perf: Add Cavium ThunderX2 SoC UNCORE PMU driver")
> ...
>
> and that's not even the complete list.
>
> Does it really make sense to have this kind of silly enumeration of
> many (MANY!) different arch CPU hotplug state indexes, where most of
> them are relevant only to that particular architecture..
>
> No, I don't think this is a _problem_, but it's kind of ugly, wouldn't
> you agree?
>
> Wouldn't it be better to just reserve N different states for the
> architecture hotplug state, and then let each architecture decide how
> they want to order them?
>
> Or better yet, make at least some of them architecture-neutral.
> Because now there are drivers that clearly are very tied to one
> architecture - or SoCs (look at various timer things) - do they really
> want or need their own architecture- or SoC-specific hotplug state?
> IOW, do we really need all of these:
>
> CPUHP_AP_ARM_ARCH_TIMER_STARTING,
> CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
> CPUHP_AP_JCORE_TIMER_STARTING,
> CPUHP_AP_QCOM_TIMER_STARTING,
> CPUHP_AP_TEGRA_TIMER_STARTING,
> CPUHP_AP_ARMADA_TIMER_STARTING,
> CPUHP_AP_MARCO_TIMER_STARTING,
> CPUHP_AP_MIPS_GIC_TIMER_STARTING,
> CPUHP_AP_ARC_TIMER_STARTING,
> CPUHP_AP_RISCV_TIMER_STARTING,
> CPUHP_AP_CLINT_TIMER_STARTING,
> CPUHP_AP_CSKY_TIMER_STARTING,
> CPUHP_AP_HYPERV_TIMER_STARTING,
> CPUHP_AP_KVM_ARM_TIMER_STARTING,
> CPUHP_AP_DUMMY_TIMER_STARTING,
>
> as separate hotplug events?
>
> Whatever. I don't really care deeply, but this just smells a bit to me.
>
> Comments?
We could use CPUHP_AP_ONLINE_DYN to reduce most of the above.
Here is the example of csky:
diff --git a/arch/csky/kernel/perf_event.c b/arch/csky/kernel/perf_event.c
index e5f1842..ccc27c3 100644
--- a/arch/csky/kernel/perf_event.c
+++ b/arch/csky/kernel/perf_event.c
@@ -1319,10 +1319,10 @@ int csky_pmu_device_probe(struct platform_device *pdev,
pr_notice("[perf] PMU request irq fail!\n");
}
- ret = cpuhp_setup_state(CPUHP_AP_PERF_CSKY_ONLINE, "AP_PERF_ONLINE",
+ ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"arch/csky/perf_event:starting",
csky_pmu_starting_cpu,
csky_pmu_dying_cpu);
- if (ret) {
+ if (ret < 0) {
csky_pmu_free_irq();
free_percpu(csky_pmu.hw_events);
return ret;
diff --git a/drivers/clocksource/timer-mp-csky.c
b/drivers/clocksource/timer-mp-csky.c
index 183a995..fc17d77 100644
--- a/drivers/clocksource/timer-mp-csky.c
+++ b/drivers/clocksource/timer-mp-csky.c
@@ -151,11 +151,11 @@ static int __init csky_mptimer_init(struct
device_node *np)
clocksource_register_hz(&csky_clocksource, timer_of_rate(to));
sched_clock_register(sched_clock_read, 32, timer_of_rate(to));
- ret = cpuhp_setup_state(CPUHP_AP_CSKY_TIMER_STARTING,
+ ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"clockevents/csky/timer:starting",
csky_mptimer_starting_cpu,
csky_mptimer_dying_cpu);
- if (ret)
+ if (ret < 0)
return -EINVAL;
return 0;
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index f14adb8..5abcfda 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -134,7 +134,6 @@ enum cpuhp_state {
CPUHP_AP_ARC_TIMER_STARTING,
CPUHP_AP_RISCV_TIMER_STARTING,
CPUHP_AP_CLINT_TIMER_STARTING,
- CPUHP_AP_CSKY_TIMER_STARTING,
CPUHP_AP_HYPERV_TIMER_STARTING,
CPUHP_AP_KVM_STARTING,
CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
@@ -186,7 +185,6 @@ enum cpuhp_state {
CPUHP_AP_PERF_POWERPC_TRACE_IMC_ONLINE,
CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE,
CPUHP_AP_PERF_POWERPC_HV_GPCI_ONLINE,
- CPUHP_AP_PERF_CSKY_ONLINE,
CPUHP_AP_WATCHDOG_ONLINE,
CPUHP_AP_WORKQUEUE_ONLINE,
CPUHP_AP_RCUTREE_ONLINE,
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
