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

[Ulrich Obergfell]

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


----- Original Message -----
From: "Josh Hunt" <johunt@xxxxxxxxxx>
To: "Don Zickus" <dzickus@xxxxxxxxxx>
Cc: akpm@xxxxxxxxxxxxxxxxxxxx, uobergfe@xxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
Sent: Tuesday, March 15, 2016 5:02:31 AM
Subject: Re: [PATCH] watchdog: don't run proc_watchdog_update if new value is same as old

On 03/14/2016 11:29 AM, Don Zickus wrote:
>
> Hi Josh,
>
> I believe Uli thought the below patch might fix it.
>
> Cheers,
> Don

Don

It looks like I was incorrect when I said 4.5 was getting the soft 
lockup. I originally found this problem on 4.1.19 and saw both the 
problem my patch solves and the soft lockups there. I thought when I 
checked 4.5 that I saw both issues there as well, but going back and 
checking now that is not the case. I only see the issue my patch 
resolves on 4.5.

With that info my changelog is incorrect now as it states I saw a soft 
lockup on the head. I will submit a v2 of my patch with the updated 
changelog. I'll also cc stable this time as I'd like to see this fix end 
up there as well.

As for the soft lockups showing up on 4.1, I tried Uli's patch and it 
did not help. After that I did a git bisect to figure out when the soft 
lockup was fixed and it appears to be resolved after one of the commits 
in this series:

commit 81a4beef91ba4a9e8ad6054ca9933dff7e25ff28
Author: Ulrich Obergfell <uobergfe@xxxxxxxxxx>
Date:   Fri Sep 4 15:45:15 2015 -0700

     watchdog: introduce watchdog_park_threads() and 
watchdog_unpark_threads()

I didn't identify the exact commit.

It would be nice to resolve the soft lockup for the stable folks since 
4.1 and 4.4 are longterm stable releases and would see this problem.

I did not have time to debug it any more outside of this bisection 
today. If you have something you'd like me to try which may work for the 
stable kernels I'm happy to test it.

For the record I'm able to reproduce the soft lockup on 4.1 doing:

while :; do echo 1 > /proc/sys/kernel/nmi_watchdog; sleep .1; done & 
sleep 30 && kill %1 && sleep 5

Josh