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

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

[Joshua Hunt]

While working on a script to restore all sysctl params before a series of
tests I found that writing any value into the
/proc/sys/kernel/{nmi_watchdog,soft_watchdog,watchdog,watchdog_thresh}
causes them to call proc_watchdog_update(). Not only that, but when I
wrote to these proc files in a loop I could easily trigger a soft lockup.

[  955.756196] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
[  955.765994] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
[  955.774619] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
[  955.783182] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
[  959.788319] NMI watchdog: BUG: soft lockup - CPU#4 stuck for 30s! [swapper/4:0]
[  959.788325] NMI watchdog: BUG: soft lockup - CPU#5 stuck for 30s! [swapper/5:0]

There doesn't appear to be a reason for doing this work other every time a
write occurs, so only do the work when the values change.

Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 kernel/watchdog.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index b3ace6e..9acb29f 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -923,6 +923,9 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 		 * both lockup detectors are disabled if proc_watchdog_update()
 		 * returns an error.
 		 */
+		if (old == new)
+			goto out;
+
 		err = proc_watchdog_update();
 	}
 out:
@@ -967,7 +970,7 @@ int proc_soft_watchdog(struct ctl_table *table, int write,
 int proc_watchdog_thresh(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	int err, old;
+	int err, old, new;
 
 	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
@@ -987,6 +990,10 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 	/*
 	 * Update the sample period. Restore on failure.
 	 */
+	new = ACCESS_ONCE(watchdog_thresh);
+	if (old == new)
+		goto out;
+
 	set_sample_period();
 	err = proc_watchdog_update();
 	if (err) {
-- 
1.7.9.5

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

[Don Zickus]

On Sat, Mar 12, 2016 at 06:50:26PM -0500, Joshua Hunt wrote:
> While working on a script to restore all sysctl params before a series of
> tests I found that writing any value into the
> /proc/sys/kernel/{nmi_watchdog,soft_watchdog,watchdog,watchdog_thresh}
> causes them to call proc_watchdog_update(). Not only that, but when I
> wrote to these proc files in a loop I could easily trigger a soft lockup.
> 
> [  955.756196] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> [  955.765994] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> [  955.774619] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> [  955.783182] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> [  959.788319] NMI watchdog: BUG: soft lockup - CPU#4 stuck for 30s! [swapper/4:0]
> [  959.788325] NMI watchdog: BUG: soft lockup - CPU#5 stuck for 30s! [swapper/5:0]
> 
> There doesn't appear to be a reason for doing this work other every time a
> write occurs, so only do the work when the values change.

Hi Josh,

Thanks for the patch.  I have no objections to it, but Uli and myself were
interested in the reason for the softlockups.  Uli is going to provide a
test patch to see if his theory is correct.  That way we fix the underlying
issue and then apply your patch on top. Make sense?

Cheers,
Don

> 
> Signed-off-by: Josh Hunt <johunt@akamai.com>
> ---
>  kernel/watchdog.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index b3ace6e..9acb29f 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -923,6 +923,9 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
>  		 * both lockup detectors are disabled if proc_watchdog_update()
>  		 * returns an error.
>  		 */
> +		if (old == new)
> +			goto out;
> +
>  		err = proc_watchdog_update();
>  	}
>  out:
> @@ -967,7 +970,7 @@ int proc_soft_watchdog(struct ctl_table *table, int write,
>  int proc_watchdog_thresh(struct ctl_table *table, int write,
>  			 void __user *buffer, size_t *lenp, loff_t *ppos)
>  {
> -	int err, old;
> +	int err, old, new;
>  
>  	get_online_cpus();
>  	mutex_lock(&watchdog_proc_mutex);
> @@ -987,6 +990,10 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
>  	/*
>  	 * Update the sample period. Restore on failure.
>  	 */
> +	new = ACCESS_ONCE(watchdog_thresh);
> +	if (old == new)
> +		goto out;
> +
>  	set_sample_period();
>  	err = proc_watchdog_update();
>  	if (err) {
> -- 
> 1.7.9.5
> 

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

[Josh Hunt]

On 03/14/2016 09:34 AM, Don Zickus wrote:
> On Sat, Mar 12, 2016 at 06:50:26PM -0500, Joshua Hunt wrote:
>> While working on a script to restore all sysctl params before a series of
>> tests I found that writing any value into the
>> /proc/sys/kernel/{nmi_watchdog,soft_watchdog,watchdog,watchdog_thresh}
>> causes them to call proc_watchdog_update(). Not only that, but when I
>> wrote to these proc files in a loop I could easily trigger a soft lockup.
>>
>> [  955.756196] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
>> [  955.765994] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
>> [  955.774619] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
>> [  955.783182] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
>> [  959.788319] NMI watchdog: BUG: soft lockup - CPU#4 stuck for 30s! [swapper/4:0]
>> [  959.788325] NMI watchdog: BUG: soft lockup - CPU#5 stuck for 30s! [swapper/5:0]
>>
>> There doesn't appear to be a reason for doing this work other every time a
>> write occurs, so only do the work when the values change.
>
> Hi Josh,
>
> Thanks for the patch.  I have no objections to it, but Uli and myself were
> interested in the reason for the softlockups.  Uli is going to provide a
> test patch to see if his theory is correct.  That way we fix the underlying
> issue and then apply your patch on top. Make sense?

Yep. Sounds good. I meant to mention I didn't diagnose the soft-lockup. 
If you provide a patch I'm happy to test. I can also attempt to debug 
that part more if needed.

Josh

>
> Cheers,
> Don
>
>>
>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>> ---
>>   kernel/watchdog.c |    9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> index b3ace6e..9acb29f 100644
>> --- a/kernel/watchdog.c
>> +++ b/kernel/watchdog.c
>> @@ -923,6 +923,9 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
>>   		 * both lockup detectors are disabled if proc_watchdog_update()
>>   		 * returns an error.
>>   		 */
>> +		if (old == new)
>> +			goto out;
>> +
>>   		err = proc_watchdog_update();
>>   	}
>>   out:
>> @@ -967,7 +970,7 @@ int proc_soft_watchdog(struct ctl_table *table, int write,
>>   int proc_watchdog_thresh(struct ctl_table *table, int write,
>>   			 void __user *buffer, size_t *lenp, loff_t *ppos)
>>   {
>> -	int err, old;
>> +	int err, old, new;
>>
>>   	get_online_cpus();
>>   	mutex_lock(&watchdog_proc_mutex);
>> @@ -987,6 +990,10 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
>>   	/*
>>   	 * Update the sample period. Restore on failure.
>>   	 */
>> +	new = ACCESS_ONCE(watchdog_thresh);
>> +	if (old == new)
>> +		goto out;
>> +
>>   	set_sample_period();
>>   	err = proc_watchdog_update();
>>   	if (err) {
>> --
>> 1.7.9.5
>>

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

[Don Zickus]

On Mon, Mar 14, 2016 at 09:45:26AM -0500, Josh Hunt wrote:
> On 03/14/2016 09:34 AM, Don Zickus wrote:
> >On Sat, Mar 12, 2016 at 06:50:26PM -0500, Joshua Hunt wrote:
> >>While working on a script to restore all sysctl params before a series of
> >>tests I found that writing any value into the
> >>/proc/sys/kernel/{nmi_watchdog,soft_watchdog,watchdog,watchdog_thresh}
> >>causes them to call proc_watchdog_update(). Not only that, but when I
> >>wrote to these proc files in a loop I could easily trigger a soft lockup.
> >>
> >>[  955.756196] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  955.765994] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  955.774619] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  955.783182] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  959.788319] NMI watchdog: BUG: soft lockup - CPU#4 stuck for 30s! [swapper/4:0]
> >>[  959.788325] NMI watchdog: BUG: soft lockup - CPU#5 stuck for 30s! [swapper/5:0]
> >>
> >>There doesn't appear to be a reason for doing this work other every time a
> >>write occurs, so only do the work when the values change.
> >
> >Hi Josh,
> >
> >Thanks for the patch.  I have no objections to it, but Uli and myself were
> >interested in the reason for the softlockups.  Uli is going to provide a
> >test patch to see if his theory is correct.  That way we fix the underlying
> >issue and then apply your patch on top. Make sense?
> 
> Yep. Sounds good. I meant to mention I didn't diagnose the soft-lockup. If
> you provide a patch I'm happy to test. I can also attempt to debug that part
> more if needed.

Hi Josh,

I believe Uli thought the below patch might fix it.

Cheers,
Don

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index b3ace6e..dd298d2 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -517,12 +517,12 @@ static void watchdog_enable(unsigned int cpu)
 	/* Enable the perf event */
 	watchdog_nmi_enable(cpu);
 
+	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
 	/* done here because hrtimer_start can only pin to smp_processor_id() */
 	hrtimer_start(hrtimer, ns_to_ktime(sample_period),
 		      HRTIMER_MODE_REL_PINNED);
 
 	/* initialize timestamp */
-	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
 	__touch_watchdog();
 }
 
@@ -530,8 +530,8 @@ static void watchdog_disable(unsigned int cpu)
 {
 	struct hrtimer *hrtimer = raw_cpu_ptr(&watchdog_hrtimer);
 
-	watchdog_set_prio(SCHED_NORMAL, 0);
 	hrtimer_cancel(hrtimer);
+	watchdog_set_prio(SCHED_NORMAL, 0);
 	/* disable the perf event */
 	watchdog_nmi_disable(cpu);
 }

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

[Josh Hunt]

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@redhat.com>
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

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

[Don Zickus]

On Mon, Mar 14, 2016 at 11:02:31PM -0500, Josh Hunt wrote:
> 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@redhat.com>
> 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


Thanks for the feedback Josh!  Good to know the softlockup was fixed
already. :-)

Cheers,
Don

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@akamai.com>
To: "Don Zickus" <dzickus@redhat.com>
Cc: akpm@linux-foundation.org, uobergfe@redhat.com, linux-kernel@vger.kernel.org
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@redhat.com>
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

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

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

[Ulrich Obergfell]

Josh,

in https://lkml.org/lkml/2016/3/15/1 you stated that the soft lockup
messages do not occur with kernel v4.5. Hence, I believe this should
not be reproducible with kernel v4.4 either. The relevant changes in
update_watchdog_all_cpus() were introduced in kernel v4.3 by patches
that I mentioned in my previous reply. I see that

  watchdog: use park/unpark functions in update_watchdog_all_cpus()

and related patches are included in this change log

  https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/log/kernel/watchdog.c?id=refs/tags/v4.4.6

In terms of kernel v4.1, it seems that the issue is mitigated by the
access rights of the watchdog parameters in /proc/sys/kernel as only
a privileged user should be able to write to, for example

   /proc/sys/kernel/nmi_watchdog

Also, based on the analysis in my previous reply, I think these soft
lockup messages are 'false positives' as the repeated cancel/restart
of watchdog_timer_fn() prevents the 'watchdog/N' thread from running
(i.e. I think the thread is not prevented from running by something
actually hogging CPU N).


Regards,

Uli


----- Original Message -----
From: "Josh Hunt" <johunt@akamai.com>
To: "Ulrich Obergfell" <uobergfe@redhat.com>
Cc: "Don Zickus" <dzickus@redhat.com>, akpm@linux-foundation.org, linux-kernel@vger.kernel.org
Sent: Thursday, March 17, 2016 5:08:03 PM
Subject: Re: [PATCH] watchdog: don't run proc_watchdog_update if new value is same as old

[...]

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.

[...]