lkml.org 
[lkml]   [2015]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v9 2/3] watchdog: add watchdog_cpumask sysctl to assist nohz
    cc'ing Andrew

    On Mon, Apr 27, 2015 at 04:27:16PM -0400, Chris Metcalf wrote:
    > I've been out on vacation the last ten days, but picking this up
    > again now.
    >
    > I'll wait a bit before putting out a v10, and also address Uli's additional
    > emails. Meanwhile, who is the right person to eventually pick up this patchset
    > and push it up to Linus? Frederic, Don, Thomas, akpm? v9 is here:

    I usually resubmit watchdog changes with my signoff to Andrew. But would
    just my ACK be ok, Andrew?

    Cheers,
    Don

    >
    > https://lkml.org/lkml/2015/4/17/697
    >
    > And I haven't heard any feedback on my fix to /proc/self/stat etc. to
    > avoid showing the PARKED threads in "R" state (patch 3/3 from that series).
    >
    > Thanks for any guidance.
    >
    >
    > On 04/22/2015 11:21 AM, Don Zickus wrote:
    > >On Wed, Apr 22, 2015 at 07:02:31AM -0400, Ulrich Obergfell wrote:
    > >>Chris,
    > >>
    > >>in https://lkml.org/lkml/2015/4/17/616 you stated:
    > >>
    > >> ">> + alloc_cpumask_var(&watchdog_cpumask_for_smpboot, GFP_KERNEL);
    > >> >
    > >> > alloc_cpumask_var could fail?
    > >>
    > >> Good catch; if I get a failure I'll just return early without trying to
    > >> start the watchdog, since clearly things are too memory-constrained
    > >> to enable that functionality anyway."
    > >>
    > >>Let's assume that (in spite of the memory constraints) the kernel would still
    > >>be able to make progress and get to a point where the system will be usable.
    > >>In this corner case, the following code would leave a NULL pointer behind in
    > >>watchdog_cpumask and in watchdog_cpumask_bits which could subsequently lead
    > >>to a crash.
    > >>
    > >> void __init lockup_detector_init(void)
    > >> {
    > >> set_sample_period();
    > >>+ if (!alloc_cpumask_var(&watchdog_cpumask, GFP_KERNEL)) {
    > >>+ pr_err("Failed to allocate cpumask for watchdog");
    > >>+ return;
    > >>+ }
    > >>+ watchdog_cpumask_bits = cpumask_bits(watchdog_cpumask);
    > >>
    > >>For example, proc_watchdog_cpumask() and the change that your patch introduces
    > >>in watchdog_enable_all_cpus() are not protected against a possible NULL pointer.
    > >>I think the code needs to be made safer.
    > >Or we could just statically allocate it
    > >
    > >static DECLARE_BITMAP(watchdog_cpumask, NR_CPUS) __read_mostly;
    > >
    > >Cheers,
    > >Don
    >
    > I think Don's suggestion is best here. It's too intrusive to try to check
    > for the out-of-memory condition everywhere in the code, just to guard
    > against the possibility that a system that is already out of memory while
    > starting the watchdog still has users trying to fiddle with the
    > /proc/sys/kernel/watchdog* knobs.
    >
    > The diff against v9 is just this (plus changing watchdog_cpumask to
    > &watchdog_cpumask in a bunch of places):
    >
    > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
    > index 8875717b6616..ec742f38c90d 100644
    > --- a/kernel/watchdog.c
    > +++ b/kernel/watchdog.c
    > @@ -57,8 +57,8 @@ int __read_mostly sysctl_softlockup_all_cpu_backtrace;
    > #else
    > #define sysctl_softlockup_all_cpu_backtrace 0
    > #endif
    > -static cpumask_var_t watchdog_cpumask;
    > -unsigned long *watchdog_cpumask_bits;
    > +static struct cpumask __read_mostly;
    > +unsigned long *watchdog_cpumask_bits = cpumask_bits(watchdog_cpumask);
    > /* Helper for online, unparked cpus. */
    > #define for_each_watchdog_cpu(cpu) \
    > @@ -913,12 +913,6 @@ void __init lockup_detector_init(void)
    > {
    > set_sample_period();
    > - if (!alloc_cpumask_var(&watchdog_cpumask, GFP_KERNEL)) {
    > - pr_err("Failed to allocate cpumask for watchdog");
    > - return;
    > - }
    > - watchdog_cpumask_bits = cpumask_bits(watchdog_cpumask);
    > -
    > #ifdef CONFIG_NO_HZ_FULL
    > if (!cpumask_empty(tick_nohz_full_mask))
    > pr_info("Disabling watchdog on nohz_full cores by default\n");
    >
    > That said, presumably we need to schedule a cage match between Frederic and Don
    > to decide on whether it's best to statically allocate cpumasks or not :-)
    >
    > https://lkml.org/lkml/2015/4/16/416
    >
    > My sense is that in this case it's appropriate, since it's much harder to
    > manage the failure case, whereas in the earlier discussion for
    > smpboot_update_cpumask_percpu_thread() it made sense to just give up and
    > return a quick ENOMEM. Also, in this case we have no locking issues.
    > --
    > Chris Metcalf, EZChip Semiconductor
    > http://www.ezchip.com
    >


    \
     
     \ /
      Last update: 2015-04-28 18:01    [W:4.612 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site