Hi all,

On Mon, Mar 1, 2021 at 4:36 AM Linus Torvalds
[off-list ref] 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 [off-list ref] wrote:

quoted

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/