Re: [RFC 1/2] watchdog: update watchdog attributes atomically

[Michal Hocko]

On Fri 19-07-13 12:10:48, Don Zickus wrote:
> On Fri, Jul 19, 2013 at 11:04:58AM +0200, Michal Hocko wrote:
> > proc_dowatchdog doesn't synchronize multiple callers which
> > might lead to confusion when two parallel callers might confuse
> > watchdog_enable_all_cpus resp. watchdog_disable_all_cpus (e.g. watchdog
> > gets enabled even if watchdog_thresh was set to 0 already).
> > 
> > This patch adds a local mutex which synchronizes callers to the sysctl
> > handler.
> 
> Looks fine by me, except one little nitpick..
> 
> > 
> > Signed-off-by: Michal Hocko <mhocko@xxxxxxx>
> > ---
> >  kernel/watchdog.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > index 1241d8c..2d64c02 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -520,13 +520,15 @@ int proc_dowatchdog(struct ctl_table *table, int write,
> >  		    void __user *buffer, size_t *lenp, loff_t *ppos)
> >  {
> >  	int err, old_thresh, old_enabled;
> > +	static DEFINE_MUTEX(watchdog_proc_mutex);
> 
> Should we just make this global instead of hiding it as a static inside a
> function.  I don't know the kernel rules for deciding which approach makes
> sense.  I know it is the same result in either case...

I've hidden it into the function to discourage from abusing it for
something else and because the usage is nicely focused in this
function. But I have no problem to pull it out.

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/