Re: + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Mar 11, 2021 at 01:22:07PM -0800, akpm@linux-foundation.org wrote:
> --- a/kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus
> +++ a/kernel/watchdog.c
> @@ -409,12 +409,18 @@ 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))
>  				return HRTIMER_RESTART;
> +			/*
> +			 * Make sure that reports are serialized. Start
> +			 * printing after getting the exclusive rights.
> +			 */
> +			smp_mb__after_atomic();

test_and_set_bit() is a value returning atomic and therefore already
implies a full smp_mb() on both ends.

>  		}
>  
>  		/* Start period for the next softlockup warning. */
> @@ -431,14 +437,14 @@ 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();
> -
> +			/*
> +			 * Make sure that everything is printed before
> +			 * another CPU is allowed to report lockup again.
> +			 */

This is not something smp_mb() ensures. smp_mb() is only concerned with
memory access ordering, not completion, and certainly not some random
IO.

IOW, that comment is complete bollocks.

> +			smp_mb__before_atomic();
> +			/* Allow a further report. */
>  			clear_bit(0, &soft_lockup_nmi_warn);


That said, this looks like a test-and-set lock and clear_bit unlock like
situation. In fact it doesn't seem to use anything other than bit0 of
that word.

FWIW we have test_and_set_bit_lock() / clear_bit_unlock().