Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports

[Petr Mladek <pmladek@suse.com>]

On Sat 2021-03-20 09:32:43, Andrew Morton wrote:
> On Fri, 19 Mar 2021 13:14:40 +0100 Petr Mladek <pmladek@suse.com> wrote:
> 
> > this patch can be put on top of the patchset fixing/cleaning softlockup
> > watchdog code. Feel free to squash it into the other patch fixing
> > the barriers.
> > 
> > Or should I resend the entire patchset again, please?
> 
> That's OK - I'll queue this as a fix against "watchdog: fix barriers
> when printing backtraces from all CPUs".
> 
> However once the two patches are merged into one for upstreaming, the
> changelog doesn't seem to be fully correct and complete.  Please check
> and suggest a replacement if needed?  The joined patches presently 
> look like this:
> 
> From: Petr Mladek <pmladek@suse.com>
> Subject: watchdog: fix barriers when printing backtraces from all CPUs
> 
> Any parallel softlockup reports are skipped when one CPU is already
> printing backtraces from all CPUs.
> 
> The exclusive rights are synchronized using one bit in
> soft_lockup_nmi_warn.  There is also one memory barrier that does not make
> much sense.
> 
> Use two barriers on the right location to prevent mixing two reports.

Please, use the following:

<proposal>
Subject: watchdog: Clean up locking when printing backtraces from all CPUs

Any parallel softlockup reports are skipped when one CPU is already
printing backtraces from all CPUs.

The exclusive rights are synchronized using one bit in
soft_lockup_nmi_warn. There is one explicit barrier. The other
is implicitly provided by test_and_set_bit().

Use *_bit_lock() API to better express the intention. It provides
the needed barriers out of box.
</proposal>

Thanks a lot for taking care of it.

Best Regards,
Petr

> 
> Link: https://lkml.kernel.org/r/20210311122130.6788-6-pmladek@suse.com
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Laurence Oberman <loberman@redhat.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Vincent Whitchurch <vincent.whitchurch@axis.com>
> 
>  kernel/watchdog.c |   17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> --- a/kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus
> +++ a/kernel/watchdog.c
> @@ -409,11 +409,12 @@ static enum hrtimer_restart watchdog_tim
>  		if (kvm_check_and_clear_guest_paused())
>  			return HRTIMER_RESTART;
>  
> +		/*
> +		 * Prevent multiple soft-lockup reports if one cpu is already
> +		 * engaged in dumping all cpu back traces.
> +		 */
>  		if (softlockup_all_cpu_backtrace) {
> -			/* Prevent multiple soft-lockup reports if one cpu is already
> -			 * engaged in dumping cpu back traces
> -			 */
> -			if (test_and_set_bit(0, &soft_lockup_nmi_warn))
> +			if (test_and_set_bit_lock(0, &soft_lockup_nmi_warn))
>  				return HRTIMER_RESTART;
>  		}
>  
> @@ -431,14 +432,8 @@ static enum hrtimer_restart watchdog_tim
>  			dump_stack();
>  
>  		if (softlockup_all_cpu_backtrace) {
> -			/* Avoid generating two back traces for current
> -			 * given that one is already made above
> -			 */
>  			trigger_allbutself_cpu_backtrace();
> -
> -			clear_bit(0, &soft_lockup_nmi_warn);
> -			/* Barrier to sync with other cpus */
> -			smp_mb__after_atomic();
> +			clear_bit_unlock(0, &soft_lockup_nmi_warn);
>  		}
>  
>  		add_taint(TAINT_SOFTLOCKUP, LOCKDEP_STILL_OK);
> _
>