Re: [RFC 2/2] watchdog: update watchdog_tresh properly

[Michal Hocko]

On Fri 19-07-13 14:05:30, Don Zickus wrote:
> On Fri, Jul 19, 2013 at 06:37:50PM +0200, Michal Hocko wrote:
> > On Fri 19-07-13 12:08:52, Don Zickus wrote:
> > > On Fri, Jul 19, 2013 at 11:04:59AM +0200, Michal Hocko wrote:
> > > > watchdog_tresh controls how often nmi perf event counter checks per-cpu
> > > > hrtimer_interrupts counter and blows up if the counter hasn't changed
> > > > since the last check. The counter is updated by per-cpu watchdog_hrtimer
> > > > hrtimer which is scheduled with 2/5 watchdog_thresh period which
> > > > guarantees that hrtimer is scheduled 2 times per the main period. Both
> > > > hrtimer and perf event are started together when the watchdog is
> > > > enabled.
> > > > 
> > > > So far so good. But...
> > > > 
> > > > But what happens when watchdog_thresh is updated from sysctl handler?
> > > > 
> > > > proc_dowatchdog will set a new sampling period and hrtimer callback
> > > > (watchdog_timer_fn) will use the new value in the next round.
> > > > The problem, however, is that nobody tells the perf event that the
> > > > sampling period has changed so it is ticking with the period configured
> > > > when it has been set up.
> > > > 
> > > > This might result in an ear riping dissonance between perf and hrtimer
> > > > parts if the watchdog_thresh is increased. And even worse it might lead
> > > > to KABOOM if the watchdog is configured to panic on such a spurious
> > > > lockup.
> > > 
> > > Heh.  Good point.
> > > 
> > > What if we keep it simpler.
> > > 
> > > if (old_thresh != watchdog_thresh)
> > >   watchdog_disable_all_cpus()
> > >   wathcdog_enable_all_cpus()
> > 
> > Those would do nothing because of watchdog_disabled checks. It is also
> 
> I guess I am missing something because I don't see the watchdog_disabled
> checks getting in the way. 

You are not missing anything. I should read more carefully.

> If the watchdog is already enabled, then the disable/enable approach
> should work correctly, no?  The watchdog_disabled only blocks from
> re-disabling or re-enabling the watchdog IIUC.

Yes, you are right.
 
> > much more heavy than necessary. I hope we can get a perf API which just
> > update the perf event period directly and we do not have to call even
> > watchdog_nmi_{en,dis}able
> 
> Well the reason for the heaviness is, even with the proposed API you
> suggest, there is a problem with the softlockup's hrtimer.  If you set the
> watchdog_thresh high like say 60 seconds, then the hrtimer fires every 12
> seconds or so (while the nmi watchdog fires every 60 seconds or so).

True. I was so focused on the particular bug I was dealing with that I
didn't consider the other direction. My bad and thanks for pointing it
out.
You even do not have to play with setting the value high and then
low. It is sufficient to set the value to lower than 4 (from default 10)
and you have the same problem.

> Now lets adjust the theshold down to 10 seconds.  The current code doesn't
> reset the hrtimer to the new value until it goes off again (which at worse
> case is 12 seconds).  The API you propose would allow the nmi_watchdog to
> fire in 10 seconds.  As a result you would have a potential to cause a
> panic on a false lockup.
> 
> So if you are going to adjust the nmi_watchdog, you have to adjust the
> softlockup value too.  Hence the heavy-ness.
> 
> Unless you have another solution for the above problem?

We can cancel and restart the hrtimer, I guess. I will look into it on
Monday.

Thanks!
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/