On Thu 2023-05-04 15:13:44, Douglas Anderson wrote:
> The fact that there watchdog_hardlockup_enable(),
> watchdog_hardlockup_disable(), and watchdog_hardlockup_probe() are
> declared __weak means that the configured hardlockup detector can
> define non-weak versions of those functions if it needs to. Instead of
> doing this, the perf hardlockup detector hooked itself into the
> default __weak implementation, which was a bit awkward. Clean this up.
>
> >From comments, it looks as if the original design was done because the
> __weak function were expected to implemented by the architecture and
> not by the configured hardlockup detector. This got awkward when we
> tried to add the buddy lockup detector which was not arch-specific but
> wanted to hook into those same functions.
>
> This is not expected to have any functional impact.
>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
I like this change:
Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
See a comment below.
> --- a/kernel/watchdog_perf.c
> +++ b/kernel/watchdog_perf.c
> @@ -147,12 +151,16 @@ void hardlockup_detector_perf_enable(void)
> }
>
> /**
> - * hardlockup_detector_perf_disable - Disable the local event
> + * watchdog_hardlockup_disable - Disable the local event
> + *
> + * @cpu: The CPU to enable hard lockup on.
> */
> -void hardlockup_detector_perf_disable(void)
> +void watchdog_hardlockup_disable(unsigned int cpu)
> {
> struct perf_event *event = this_cpu_read(watchdog_ev);
>
> + WARN_ON_ONCE(cpu != smp_processor_id());
> +
It makes sense. But it just shows how the code is weird.
@cpu is passed as a parameter and the code expects that it is
running on the given CPU.
It seems that @cpu is passed as a parameter because this is
called from:
+ [CPUHP_AP_WATCHDOG_ONLINE].teardown.single()
+ lockup_detector_offline_cpu()
+ watchdog_disable()
and the CPU hotplug API passes @cpu parameter.
IMHO, the clean solution would be to use per_cpu*() instead
of this_cpu*() API everywhere in this code path.
But it is yet another cleanup. It seems to be out-of-scope of
this patchset.
> if (event) {
> perf_event_disable(event);
> this_cpu_write(watchdog_ev, NULL);
Best Regards,
Petr
