Re: [PATCH] watchdog: don't run proc_watchdog_update if new value is same as old

[Josh Hunt]

On 03/16/2016 04:21 AM, Ulrich Obergfell wrote:
> Josh,
>
> I haven't tried to reproduce the soft lockups with kernel 4.1, but I
> believe I found an explanation in terms of how your test case breaks
> the watchdog mechanism in kernel 4.1:
>
> The soft lockup detector is implemented via two components (per-cpu).
>
> - The watchdog_timer_fn() function.
>    This function runs periodically (sample_period) in the context of a
>    high-resolution timer on CPU N. It wakes up the 'watchdog/N' thread
>    and checks watchdog_touch_ts[N] to determine whether the thread has
>    run within the soft lockup detection interval (watchdog_thresh * 2).
>
> - The watchdog() function.
>    This function is executed in the context of the 'watchdog/N' thread.
>    It updates the watchdog_touch_ts[N] time stamp to signal that it got
>    a chance to run. The thread has a high realtime priority. If it does
>    not get a chance to run, this is indicative that 'something else' is
>    hogging CPU N.
>
>
> The while :; do echo 1 > /proc/sys/kernel/nmi_watchdog; sleep .1; done
> loop triggers the following flow of execution in the watchdog code at
> a frequency that is much higher than sample_period.
>
>    proc_nmi_watchdog
>      proc_watchdog_common
>        proc_watchdog_update
>          watchdog_enable_all_cpus
>            update_watchdog_all_cpus
>              for_each_online_cpu
>                  update_watchdog
>                    smp_call_function_single(restart_watchdog_hrtimer)
>
> Calling the restart_watchdog_hrtimer() function at such a high rate has
> the following effects.
>
> - The high-resolution timer gets canceled and restarted before it ever
>    gets a chance to expire. Hence, the watchdog_timer_fn() function does
>    not get a chance to wake up the 'watchdog/N' thread. So this prevents
>    the thread from running within the soft lockup detection interval.
>
> - Since watchdog_timer_fn() does not wake up the 'watchdog/N' thread,
>    the watchdog_touch_ts[N] time stamp is not being updated.
>
> The  sleep 30 && kill %1  command terminates the above while loop after
> 30 seconds, so the high-resolution timer no longer gets canceled. When
> the timer expires, watchdog_timer_fn() detects that 'watchdog/N' has not
> run within the soft lockup detection interval.
>
> Your patch 'masks' the above issue because proc_watchdog_update() is not
> called if the parameter value is not changed.
>
>
> The mechanism that picks up changes of watchdog parameters 'on the fly'
> was rewritten in kernel 4.3 (see https://lkml.org/lkml/2015/8/1/64 and
> http://marc.info/?l=linux-kernel&m=143894132129981&w=2).
>
> $ git log --pretty=oneline v4.2..v4.3 -- kernel/watchdog.c | head -5
> ec6a90661a0d6ce1461d05c7a58a0a151154e14a watchdog: rename watchdog_suspend() and watchdog_resume()
> 999bbe49ea0118b70ddf3f5d679f51dc7a97ae55 watchdog: use suspend/resume interface in fixup_ht_bug()
> d4bdd0b21c7652a8271f873cc755486b255c1bbd watchdog: use park/unpark functions in update_watchdog_all_cpus()
> 8c073d27d7ad293bf734cc8475689413afadab81 watchdog: introduce watchdog_suspend() and watchdog_resume()
> 81a4beef91ba4a9e8ad6054ca9933dff7e25ff28 watchdog: introduce watchdog_park_threads() and watchdog_unpark_threads()
>
> The new version of the update_watchdog_all_cpus() function now parks and
> unparks all 'watchdog/N' threads. If any watchdog parameter in /proc was
> changed, the new value gets picked up by the threads during unpark when
> watchdog_enable() is executed.
>
> The following is particularly relevant to your test case.
>
> - watchdog_disable() cancels the high-resolution timer during park
>
> - watchdog_enable() starts the high-resolution timer during unpark
>    and updates watchdog_touch_ts[N] by calling __touch_watchdog()
>
> I believe this explains why you can only reproduce the soft lockup with
> kernel v4.1 but not with v4.5.
>
>
> Your patch "don't run proc_watchdog_update if new value is same as old"
> looks OK to me. I have no objections.
>
>
> Many Thanks,
>
> Uli

Uli

Thanks for the detailed explanation!

As you mention my patch will mask this problem for 4.1 which is why I 
wanted to get it into stable. Do you think there is any way to mitigate 
this issue for the stable kernels (4.1 to 4.4) if the user changes the 
values doing something like:

foo=1; while :; do echo $foo > /proc/sys/kernel/nmi_watchdog; foo=$(( ! 
$foo )); sleep .1; done & sleep 30 && kill %1

?

I realize this isn't a real-world use-case (I hope :)), but shows there 
is still a way to cause the box to soft lockup with this code path.

I ask b/c these kernels are both longterm stable releases and so this 
issue will likely live for a while in the wild.

Josh