Messages in this thread | | | Date | Wed, 22 Nov 2023 14:05:21 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH v2] softlockup: serialized softlockup's log |
| |
On Wed, 22 Nov 2023 18:02:12 +0800 lizhe.67@bytedance.com wrote:
> From: Li Zhe <lizhe.67@bytedance.com> > > If multiple CPUs trigger softlockup at the same time with > 'softlockup_all_cpu_backtrace=0', the softlockup's logs will appear > staggeredly in dmesg, which will affect the viewing of the logs for > developer. Since the code path for outputting softlockup logs is not > a kernel hotspot and the performance requirements for the code are > not strict, locks are used to serialize the softlockup log output to > improve the readability of the logs.
Seems reasonable, but...
> --- a/kernel/watchdog.c > +++ b/kernel/watchdog.c > @@ -28,6 +28,8 @@ > #include <linux/kvm_para.h> > > static DEFINE_MUTEX(watchdog_mutex); > +/* This lock is used to prevent concurrent actions of softlockup output logs */ > +static DEFINE_SPINLOCK(watchdog_output_lock);
It would be a little neater to reduce the scope of this - move the definition into that little code block in watchdog_timer_fn() where it is actually used.
> #if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HARDLOCKUP_DETECTOR_SPARC64) > # define WATCHDOG_HARDLOCKUP_DEFAULT 1 > @@ -514,6 +516,7 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer) > /* Start period for the next softlockup warning. */ > update_report_ts(); > > + spin_lock(&watchdog_output_lock);
The hrtimer callout function is called from [soft]irq context, yes? Doesn't lockdep get upset when we take a spinlock in such a context?
> pr_emerg("BUG: soft lockup - CPU#%d stuck for %us! [%s:%d]\n", > smp_processor_id(), duration, > current->comm, task_pid_nr(current)); > @@ -523,6 +526,7 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer) > show_regs(regs); > else > dump_stack(); > + spin_unlock(&watchdog_output_lock); > > if (softlockup_all_cpu_backtrace) { > trigger_allbutcpu_cpu_backtrace(smp_processor_id());
| |