Messages in this thread | | | Date | Wed, 21 Apr 2010 23:46:12 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 2/6] [watchdog] convert touch_softlockup_watchdog to touch_watchdog |
| |
On Wed, Apr 21, 2010 at 05:31:42PM -0400, Don Zickus wrote: > On Wed, Apr 21, 2010 at 10:46:01PM +0200, Frederic Weisbecker wrote: > > On Tue, Apr 20, 2010 at 11:23:59AM -0400, Don Zickus wrote: > > > Just a scripted conversion to remove touch_softlockup_watchdog. > > > > > > Also converts the once case of touch_all_softlockup_watchdogs to > > > touch_all_watchdogs. > > > > > > This is done as part of the removal of the old softlockup code and > > > transition to the new softlockup code. > > > > > > Signed-off-by: Don Zickus <dzickus@redhat.com> > > > > > > In fact I worry a bit about this unification of watchdog touching. > > When we touch the softlockup watchdog, do we also want to touch > > the nmi watchdog? > > > > Most of the time, I think we don't want to. We usually touch the > > softlockup detector because we know we are abnormally delaying > > the softlockup kthread from being scheduled, and if we are in such > > situation, it means we are doing something in a sensitive context: > > typically the kind of context favorable to create hardlockups... > > > > But the opposite is right: if we touch the nmi watchdog: it means we > > are abnormally delaying irqs, which means we also are abnormally > > delaying the softlockup kthread from being scheduled, so if we > > touch the nmi watchdog, we also want to touch the softlockup > > detector. > > > > Hence I guess we want to keep the current state: > > > > - touch_nmi_watchdog() = touch softlockup and nmi watchdogs > > - touch_softlockup_watchdog() = only touch softlockup watchdog > > Hmm ok I see what you are saying. A little tweak and I have this > compiled-tested only patch that I think satisifies you. > > I didn't really touch the touch_nmi_watchdog() code in the kernel, so it > still calls a stub function in kernel/watchdog.c. Add a boolean to that > path and I think it accomplishes the logic you are looking for. > > Cheers, > Don
Yeah looks good.
Could you send this patch with a changelog and your sign-off?
Thanks!
> > > diff --git a/kernel/watchdog.c b/kernel/watchdog.c > index 9898c7c..c1a89ac 100644 > --- a/kernel/watchdog.c > +++ b/kernel/watchdog.c > @@ -31,6 +31,7 @@ int watchdog_enabled; > int __read_mostly softlockup_thresh = 60; > > static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts); > +static DEFINE_PER_CPU(bool, watchdog_nmi_touch); > static DEFINE_PER_CPU(struct task_struct *, softlockup_watchdog); > static DEFINE_PER_CPU(struct hrtimer, watchdog_hrtimer); > static DEFINE_PER_CPU(bool, watchdog_touch_sync); > @@ -147,6 +148,7 @@ void touch_watchdog_sync(void) > > void touch_nmi_watchdog(void) > { > + __get_cpu_var(watchdog_nmi_touch) = true; > touch_watchdog(); > } > EXPORT_SYMBOL(touch_nmi_watchdog); > @@ -203,11 +205,10 @@ void watchdog_overflow_callback(struct perf_event *event, int nmi, > struct pt_regs *regs) > { > int this_cpu = smp_processor_id(); > - unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu); > char warn = per_cpu(watchdog_warn, this_cpu); > > - if (touch_ts == 0) { > - __touch_watchdog(); > + if (__get_cpu_var(watchdog_nmi_touch) == true) { > + __get_cpu_var(watchdog_nmi_touch) = false; > return; > } >
| |