On Thu, Mar 11, 2021 at 01:22:07PM -0800, akpm@xxxxxxxxxxxxxxxxxxxx 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().
