lkml.org 
[lkml]   [2014]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/3] watchdog: control hard lockup detection default
    > ----- Original Message -----
    > From: "Andrew Jones" <drjones@redhat.com>
    > To: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
    > Cc: uobergfe@redhat.com, dzickus@redhat.com, pbonzini@redhat.com, akpm@linux-foundation.org, mingo@redhat.com
    > Sent: Thursday, July 24, 2014 12:13:30 PM
    > Subject: [PATCH 2/3] watchdog: control hard lockup detection default

    [...]

    > The running kernel still has the ability to enable/disable at any
    > time with /proc/sys/kernel/nmi_watchdog us usual. However even
    > when the default has been overridden /proc/sys/kernel/nmi_watchdog
    > will initially show '1'. To truly turn it on one must disable/enable
    > it, i.e.
    > echo 0 > /proc/sys/kernel/nmi_watchdog
    > echo 1 > /proc/sys/kernel/nmi_watchdog

    [...]

    > @@ -626,15 +665,17 @@ int proc_dowatchdog(struct ctl_table *table, int write,
    > * disabled. The 'watchdog_running' variable check in
    > * watchdog_*_all_cpus() function takes care of this.
    > */
    > - if (watchdog_user_enabled && watchdog_thresh)
    > + if (watchdog_user_enabled && watchdog_thresh) {
    > + watchdog_enable_hardlockup_detector(true);
    > err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
    > - else
    > + } else

    [...]


    I just realized a possible issue in the above part of the patch:

    If we would want to give the user the option to override the effect of patch 3/3
    via /proc, I think proc_dowatchdog() should enable hard lockup detection _only_
    in case of a state transition from 'NOT watchdog_running' to 'watchdog_running'.
    |
    if (watchdog_user_enabled && watchdog_thresh) { | need to add this
    if (!watchdog_running) <---------------------------'
    watchdog_enable_hardlockup_detector(true);
    err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
    } else
    ...

    The additional 'if (!watchdog_running)' would _require_ the user to perform the
    sequence of commands

    echo 0 > /proc/sys/kernel/nmi_watchdog
    echo 1 > /proc/sys/kernel/nmi_watchdog

    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


    \
     
     \ /
      Last update: 2014-07-25 11:01    [W:4.681 / U:2.240 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site