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);
 }