Re: [PATCH v9 2/3] watchdog: add watchdog_cpumask sysctl to assist nohz

Don Zickus
2015-04-28T15:18:22Z

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
>