Messages in this thread | | | Date | Wed, 29 Apr 2015 05:49:22 -0400 (EDT) | From | Ulrich Obergfell <> | Subject | Re: [PATCH v9 2/3] watchdog: add watchdog_cpumask sysctl to assist nohz |
| |
----- Original Message ----- From: "Chris Metcalf" <cmetcalf@ezchip.com> [...] On 04/21/2015 08:32 AM, Ulrich Obergfell wrote: >> Chris, >> >> in v9, smpboot_update_cpumask_percpu_thread() allocates 'tmp' mask dynamically. >> This allocation can fail and thus the function can now return an error. However, >> this error is being ignored by proc_watchdog_cpumask(). > > Yes, I did that intentionally, because it seemed like a pretty extreme > corner case (not enough memory to allocate one cpumask), and a relatively > unproblematic outcome (we don't actually modify the running set of watchdog > threads the way the /proc knob requested). > > The problem with your proposal (to save the old cpumask and put it back on > failure) is that we will almost certainly not be able to do that either > if we can't successfully run smpboot_update_cpumask_percpu_thread(), > since that's exactly the allocation that we're presuming is going to fail > internally. > > I went down this rathole and decided it wasn't worth worrying about. > Let me know if you think we need to beat on it some more :-)
Chris,
the other handlers for the watchdog parameters in /proc restore the original value on failure, so I thought it would be nice to make the error handling consistent in that regard.
However, on the other hand the 'watchdog_cpumask' parameter is kind of an exception in terms of when and how it should be used, and thus it's probably OK if this interface is less 'user-friendly'. As Don commented in https://lkml.org/lkml/2015/4/22/325 in reply to my suggestion to add a plausibility check for 'watchdog_cpumask':
"I am not sure that is necessary. This was supposed to be a debugging interface for nohz (and possibly other technologies). ... Personally, I feel anyone who will use this sys interface will need to do so at their own risk."
So I think we could apply the same rationale here and ignore a possible error returned by smpboot_update_cpumask_percpu_thread(). Perhaps you could add a few comment lines to the code.
Don,
please let us know what you think.
Regards,
Uli
| |