On Fri, Jul 25, 2014 at 01:25:11PM +0200, Andrew Jones wrote:
> > to enable hard lockup detection explicitly.
> >
> > I think changing the 'watchdog_thresh' while 'watchdog_running' is true
> > should
> > _not_ enable hard lockup detection as a side-effect, because a user may
> > have a
> > 'sysctl.conf' entry such as
> >
> > kernel.watchdog_thresh = ...
> >
> > or may only want to change the 'watchdog_thresh' on the fly.
> >
> > I think the following flow of execution could cause such undesired
> > side-effect.
> >
> > proc_dowatchdog
> > if (watchdog_user_enabled && watchdog_thresh) {
> >
> > watchdog_enable_hardlockup_detector
> > hardlockup_detector_enabled = true
> >
> > watchdog_enable_all_cpus
> > if (!watchdog_running) {
> > ...
> > } else if (sample_period_changed)
> > update_timers_all_cpus
> > for_each_online_cpu
> > update_timers
> > watchdog_nmi_disable
> > ...
> > watchdog_nmi_enable
> >
> > watchdog_hardlockup_detector_is_enabled
> > return true
> >
> > enable perf counter for hard lockup
> > detection
> >
> > Regards,
> >
> > Uli
>
> Nice catch. Looks like this will need a v2. Paolo, do we have a
> consensus on the proc echoing? Or should that be revisited in the v2 as
> well?
As discussed privately, how about something like this to handle that case:
(applied on top of these patches)
Cheers,
Don
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 34eca29..027fb6c 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -666,7 +666,12 @@ int proc_dowatchdog(struct ctl_table *table, int write,
* watchdog_*_all_cpus() function takes care of this.
*/
if (watchdog_user_enabled && watchdog_thresh) {
- watchdog_enable_hardlockup_detector(true);
+ /*
+ * Prevent a change in watchdog_thresh accidentally overriding
+ * the enablement of the hardlockup detector.
+ */
+ if (watchdog_user_enabled != old_enabled)
+ watchdog_enable_hardlockup_detector(true);
err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
} else
watchdog_disable_all_cpus();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
