Re: [PATCH v4 12/17] watchdog/hardlockup: Have the perf hardlockup use __weak functions more cleanly
flat view
From: Petr Mladek <pmladek@suse.com>
Date: 2023-05-12 11:56:21
Also in:
linux-perf-users, sparclinux
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.quoted
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@chromium.org>
I like this change: Reviewed-by: Petr Mladek <pmladek@suse.com> See a comment below.
quoted hunk ↗ jump to hunk
--- 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