On Sun, Apr 11 2021 at 21:21, Paul E. McKenney wrote:
> On Sun, Apr 11, 2021 at 09:46:12AM -0700, Paul E. McKenney wrote:
>> So I need to is inline clocksource_verify_percpu_wq()
>> into clocksource_verify_percpu() and then move the call to
>> clocksource_verify_percpu() to __clocksource_watchdog_kthread(), right
>> before the existing call to list_del_init(). Will do!
>
> Except that this triggers the WARN_ON_ONCE() in smp_call_function_single()
> due to interrupts being disabled across that list_del_init().
>
> Possibilities include:
>
> 1. Figure out why interrupts must be disabled only sometimes while
> holding watchdog_lock, in the hope that they need not be across
> the entire critical section for __clocksource_watchdog_kthread().
> As in:
>
> local_irq_restore(flags);
> clocksource_verify_percpu(cs);
> local_irq_save(flags);
>
> Trying this first with lockdep enabled. Might be spectacular.
Yes, it's a possible deadlock against the watchdog timer firing ...
The reason for irqsave is again historical AFAICT and nobody bothered to
clean it up. spin_lock_bh() should be sufficient to serialize against
the watchdog timer, though I haven't looked at all possible scenarios.
> 2. Invoke clocksource_verify_percpu() from its original
> location in clocksource_watchdog(), just before the call to
> __clocksource_unstable(). This relies on the fact that
> clocksource_watchdog() acquires watchdog_lock without
> disabling interrupts.
That should be fine, but this might cause the softirq to 'run' for a
very long time which is not pretty either.
Aside of that, do we really need to check _all_ online CPUs? What you
are trying to figure out is whether the wreckage is CPU local or global,
right?
Wouldn't a shirt-sleeve approach of just querying _one_ CPU be good
enough? Either the other CPU has the same wreckage, then it's global or
it hasn't which points to a per CPU local issue.
Sure it does not catch the case where a subset (>1) of all CPUs is
affected, but I'm not seing how that really buys us anything.
Thanks,
tglx
