Re: [RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[Rafael J. Wysocki]

On Friday, 22 June 2007 23:07, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@xxxxxxx>
> > 
> > At present, if a user mode helper is running while usermodehelper_pm_callback()
> > is executed, the helper may be frozen and the completion in
> > call_usermodehelper_exec() won't be completed until user space processes are
> > thawed.  As a result, the freezing of kernel threads may fail, which is not
> > desirable.
> > 
> > Prevent this from happening by introducing a counter of running user mode
> > helpers and allowing usermodehelper_pm_callback() to succeed for
> > action = PM_HIBERNATION_PREPARE or action = PM_SUSPEND_PREPARE only if there
> > are no helpers running.  [Namely, usermodehelper_pm_callback() waits for at most
> > RUNNING_HELPERS_TIMEOUT for the number of running helpers to become zero and
> > fails if that doesn't happen.]
> > 
> > Special thanks to Uli Luckas <u.luckas@xxxxxxx> for reviewing the previous
> > versions of this patch and for very useful comments.
> ...
> > Reviewed-by: Pavel Machek <pavel@xxxxxx>
> 
> Eh, not sure this header is worth anything. Sometimes I'm lazy and
> stop when I see first problem.
> 
> >  	switch (action) {
> >  	case PM_HIBERNATION_PREPARE:
> >  	case PM_SUSPEND_PREPARE:
> >  		usermodehelper_disabled = 1;
> > -		return NOTIFY_OK;
> > +		retval = wait_event_timeout(running_helpers_waitq,
> > +					atomic_read(&running_helpers) == 0,
> 
> Are you sure here? What happens when atomic variable changes between
> the atomic_read and the function call?

Er, this is a macro. :-)

In fact we rely only on atomic_read(&running_helpers) being still zero after
helper_finished() has woken us up, but I think that's acceptable.

IOW, if the wait_event_timeout() returns with retval different from zero, this
means that atomic_read(&running_helpers) returned zero at one point after
we'd set usermodehelper_disabled, which is enough.  OTOH, if it doesn't
return with retval different from zero, this means that either one or more
helpers are waited for to finish or they are coming and going very quickly,
which would be suspicious enough to cancel the suspend, IMHO.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth
-
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/