arm64: watchdog_hld: provide arm_pmu_irq_is_nmi stub
Commit Message
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.
Fixes: 7e61b33831bc ("arm64: enable perf events based hard lockup detector")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
---
arch/arm64/kernel/watchdog_hld.c | 10 ----------
include/linux/nmi.h | 1 -
include/linux/perf/arm_pmu.h | 7 ++++---
kernel/watchdog_perf.c | 11 ++++++++++-
4 files changed, 14 insertions(+), 15 deletions(-)
Comments
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
@@ -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();
-}
@@ -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) && \
@@ -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__ */
@@ -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;
}