| Message ID | 20230522114922.1052421-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: watchdog_hld: provide arm_pmu_irq_is_nmi stub | expand |
Hi, On Mon, May 22, 2023 at 4:49 AM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > The newly added arch_perf_nmi_is_available() function fails to build > when CONFIG_ARM_PMU is disabled: > > arch/arm64/kernel/watchdog_hld.c: In function 'arch_perf_nmi_is_available': > arch/arm64/kernel/watchdog_hld.c:35:16: error: implicit declaration of function 'arm_pmu_irq_is_nmi' [-Werror=implicit-function-declaration] > 35 | return arm_pmu_irq_is_nmi(); > > As it turns out, there is only one caller for that function anyway, > in the same file as the __weak definition, and this can only be called > if CONFIG_ARM_PMU is also enabled. > > I tried a number of variants, but everything ended up with more > complexity from having both the __weak function and one or more > added #ifdef. Keeping it in watchdog_perf.c is a small layering > violation but otherwise the most robust. Sorry for the breakage! The intention here is that turning on CONFIG_HARDLOCKUP_DETECTOR_PERF doesn't make any sense if the PMU is not enabled. In that sense, maybe a better option would be to just fix this in Kconfig? What about going into `arch/arm64/Kconfig` and instead of: select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI We do: select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && HW_PERF_EVENTS It looks like HW_PERF_EVENTS is a synonym for ARM_PMU and that's the same symbol used to include the needed definition. Making that change seems to fix the compile error and has the added benefit that enabling CONFIG_HARDLOCKUP_DETECTOR will automatically choose the "buddy" backend when CONFIG_ARM_PMU isn't turned on. -Doug
Hi, On Mon, May 22, 2023 at 7:31 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, May 22, 2023 at 4:49 AM Arnd Bergmann <arnd@kernel.org> wrote: > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > The newly added arch_perf_nmi_is_available() function fails to build > > when CONFIG_ARM_PMU is disabled: > > > > arch/arm64/kernel/watchdog_hld.c: In function 'arch_perf_nmi_is_available': > > arch/arm64/kernel/watchdog_hld.c:35:16: error: implicit declaration of function 'arm_pmu_irq_is_nmi' [-Werror=implicit-function-declaration] > > 35 | return arm_pmu_irq_is_nmi(); > > > > As it turns out, there is only one caller for that function anyway, > > in the same file as the __weak definition, and this can only be called > > if CONFIG_ARM_PMU is also enabled. > > > > I tried a number of variants, but everything ended up with more > > complexity from having both the __weak function and one or more > > added #ifdef. Keeping it in watchdog_perf.c is a small layering > > violation but otherwise the most robust. > > Sorry for the breakage! > > The intention here is that turning on CONFIG_HARDLOCKUP_DETECTOR_PERF > doesn't make any sense if the PMU is not enabled. In that sense, maybe > a better option would be to just fix this in Kconfig? What about going > into `arch/arm64/Kconfig` and instead of: > > select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI > > We do: > > select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && > HAVE_PERF_EVENTS_NMI && HW_PERF_EVENTS > > It looks like HW_PERF_EVENTS is a synonym for ARM_PMU and that's the > same symbol used to include the needed definition. > > Making that change seems to fix the compile error and has the added > benefit that enabling CONFIG_HARDLOCKUP_DETECTOR will automatically > choose the "buddy" backend when CONFIG_ARM_PMU isn't turned on. Breadcrumbs: since I didn't see a patch this morning and I'd love to get this resolved, I've posted the Kconfig fix: https://lore.kernel.org/r/20230523073952.1.I60217a63acc35621e13f10be16c0cd7c363caf8c@changeid Assuming people think that's OK, it should land instead of ${SUBJECT} patch. -Doug
On Tue, May 23, 2023, at 17:47, Doug Anderson wrote: > On Mon, May 22, 2023 at 7:31 AM Doug Anderson <dianders@chromium.org> wrote: >> On Mon, May 22, 2023 at 4:49 AM Arnd Bergmann <arnd@kernel.org> wrote: > > Breadcrumbs: since I didn't see a patch this morning and I'd love to > get this resolved, I've posted the Kconfig fix: > > https://lore.kernel.org/r/20230523073952.1.I60217a63acc35621e13f10be16c0cd7c363caf8c@changeid > > Assuming people think that's OK, it should land instead of ${SUBJECT} patch. Looks good to me, I've replaced my patch with yours now in my randconfig build setup, I assume it's fine but I'll let you know if another regression comes up. Thanks for addressing it, Arnd
diff --git a/arch/arm64/kernel/watchdog_hld.c b/arch/arm64/kernel/watchdog_hld.c index dcd25322127c..3d948e5c1c1e 100644 --- a/arch/arm64/kernel/watchdog_hld.c +++ b/arch/arm64/kernel/watchdog_hld.c @@ -24,13 +24,3 @@ u64 hw_nmi_get_sample_period(int watchdog_thresh) return (u64)max_cpu_freq * watchdog_thresh; } - -bool __init arch_perf_nmi_is_available(void) -{ - /* - * hardlockup_detector_perf_init() will success even if Pseudo-NMI turns off, - * however, the pmu interrupts will act like a normal interrupt instead of - * NMI and the hardlockup detector would be broken. - */ - return arm_pmu_irq_is_nmi(); -} diff --git a/include/linux/nmi.h b/include/linux/nmi.h index d23902a2fd49..1fabf8c35d27 100644 --- a/include/linux/nmi.h +++ b/include/linux/nmi.h @@ -212,7 +212,6 @@ static inline bool trigger_single_cpu_backtrace(int cpu) #ifdef CONFIG_HARDLOCKUP_DETECTOR_PERF u64 hw_nmi_get_sample_period(int watchdog_thresh); -bool arch_perf_nmi_is_available(void); #endif #if defined(CONFIG_HARDLOCKUP_CHECK_TIMESTAMP) && \ diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h index 5b00f5cb4cf9..cbdd3533d843 100644 --- a/include/linux/perf/arm_pmu.h +++ b/include/linux/perf/arm_pmu.h @@ -12,10 +12,11 @@ #include <linux/perf_event.h> #include <linux/platform_device.h> #include <linux/sysfs.h> -#include <asm/cputype.h> #ifdef CONFIG_ARM_PMU +#include <asm/cputype.h> + /* * The ARMv7 CPU PMU supports up to 32 event counters. */ @@ -171,8 +172,6 @@ void kvm_host_pmu_init(struct arm_pmu *pmu); #define kvm_host_pmu_init(x) do { } while(0) #endif -bool arm_pmu_irq_is_nmi(void); - /* Internal functions only for core arm_pmu code */ struct arm_pmu *armpmu_alloc(void); void armpmu_free(struct arm_pmu *pmu); @@ -184,6 +183,8 @@ void armpmu_free_irq(int irq, int cpu); #endif /* CONFIG_ARM_PMU */ +bool arm_pmu_irq_is_nmi(void); + #define ARMV8_SPE_PDEV_NAME "arm,spe-v1" #endif /* __ARM_PMU_H__ */ diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c index 8ea00c4a24b2..ee7d3dcfdda2 100644 --- a/kernel/watchdog_perf.c +++ b/kernel/watchdog_perf.c @@ -19,6 +19,7 @@ #include <asm/irq_regs.h> #include <linux/perf_event.h> +#include <linux/perf/arm_pmu.h> static DEFINE_PER_CPU(struct perf_event *, watchdog_ev); static DEFINE_PER_CPU(struct perf_event *, dead_event); @@ -234,8 +235,16 @@ void __init hardlockup_detector_perf_restart(void) } } -bool __weak __init arch_perf_nmi_is_available(void) +static bool __init arch_perf_nmi_is_available(void) { + /* + * hardlockup_detector_perf_init() will success even if Pseudo-NMI turns off, + * however, the pmu interrupts will act like a normal interrupt instead of + * NMI and the hardlockup detector would be broken. + */ + if (IS_ENABLED(CONFIG_ARM_PMU)) + return arm_pmu_irq_is_nmi(); + return true; }