Re: [PATCH v2] arm64: Enable perf events based hard lockup detector

[Sumit Garg <sumit.garg@linaro.org>]

On Wed, 10 Jun 2020 at 07:22, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Sumit Garg (2020-06-04 05:09:11)
> > With the recent feature added to enable perf events to use pseudo NMIs
> > as interrupts on platforms which support GICv3 or later, its now been
> > possible to enable hard lockup detector (or NMI watchdog) on arm64
> > platforms. So enable corresponding support.
> >
> > One thing to note here is that normally lockup detector is initialized
> > just after the early initcalls but PMU on arm64 comes up much later as
> > device_initcall(). So we need to re-initialize lockup detection once
> > PMU has been initialized.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >
> > This patch is dependent on perf NMI patch-set [1].
> >
> > [1] https://patchwork.kernel.org/cover/11047407/
>
> That patch series is from last year. Any progress on it?
>

Alexandru (in Cc) has been working on rebasing and reworking this
patch-set with WIP-pmu-nmi branch [1]. For more information refer to
this thread [2].

[1] http://www.linux-arm.org/git?p=linux-ae.git;a=shortlog;h=refs/heads/WIP-pmu-nmi
[2] https://lkml.org/lkml/2020/5/20/431

> >
> > Changes since RFC:
> > - Rebased on top of Alex's WIP-pmu-nmi branch.
> > - Add comment for safe max. CPU frequency.
> > - Misc. cleanup.
> >
> >  arch/arm64/Kconfig             |  2 ++
> >  arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++++++--
> >  drivers/perf/arm_pmu.c         |  9 +++++++++
> >  include/linux/perf/arm_pmu.h   |  2 ++
> >  4 files changed, 52 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index e109aa5..a37f018 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> >  #define ARMV8_A53_PERFCTR_PREF_LINEFILL                                0xC2
> > @@ -1226,3 +1239,27 @@ void arch_perf_update_userpage(struct perf_event *event,
> >         userpg->time_shift = (u16)shift;
> >         userpg->time_offset = -now;
> >  }
> > +
> > +#ifdef CONFIG_HARDLOCKUP_DETECTOR_PERF
> > +/*
> > + * Safe maximum CPU frequency in case a particular platform doesn't implement
> > + * cpufreq driver. Although, architecture doesn't put any restrictions on
> > + * maximum frequency but 5 GHz seems to be safe maximum given the available
> > + * Arm CPUs in the market which are clocked much less than 5 GHz. On the other
> > + * hand, we can't make it much higher as it would lead to a large hard-lockup
> > + * detection timeout on parts which are running slower (eg. 1GHz on
> > + * Developerbox) and doesn't possess a cpufreq driver.
> > + */
> > +#define SAFE_MAX_CPU_FREQ      5000000000UL // 5 GHz
> > +u64 hw_nmi_get_sample_period(int watchdog_thresh)
>
> Is it ever negative? Odd that this API uses a signed integer.
>

No it isn't expected to be negative.

> > +{
> > +       unsigned int cpu = smp_processor_id();
> > +       unsigned int max_cpu_freq;
> > +
> > +       max_cpu_freq = cpufreq_get_hw_max_freq(cpu);
> > +       if (max_cpu_freq)
> > +               return (u64)max_cpu_freq * 1000 * watchdog_thresh;
> > +       else
> > +               return (u64)SAFE_MAX_CPU_FREQ * watchdog_thresh;
>
> I'd rather see the return unindented and the else dropped.

Okay.

>
> > +}
> > +#endif
> > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> > index f96cfc4..6c25c0d1 100644
> > --- a/drivers/perf/arm_pmu.c
> > +++ b/drivers/perf/arm_pmu.c
> > @@ -718,6 +718,15 @@ static int armpmu_get_cpu_irq(struct arm_pmu *pmu, int cpu)
> >         return per_cpu(hw_events->irq, cpu);
> >  }
> >
> > +bool arm_pmu_irq_is_nmi(void)
> > +{
> > +       const struct pmu_irq_ops *irq_ops;
> > +
> > +       irq_ops = per_cpu(cpu_irq_ops, smp_processor_id());
>
> Can we use this_cpu_ptr()?

this_cpu_ptr() sounds more apt here, will use it instead.

-Sumit

>
> > +
> > +       return irq_ops == &pmunmi_ops || irq_ops == &percpu_pmunmi_ops;
> > +}
> > +
> >  /*
> >   * PMU hardware loses all context when a CPU goes offline.
> >   * When a CPU is hotplugged back in, since some hardware registers are

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel