From: Rafael J. Wysocki <[email protected]>
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 <[email protected]> for reviewing the previous
versions of this patch and for very useful comments.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Acked-by: Nigel Cunningham <[email protected]>
Acked-by: Uli Luckas <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
kernel/kmod.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 66 insertions(+), 10 deletions(-)
Index: linux-2.6.22-rc4-mm2/kernel/kmod.c
===================================================================
--- linux-2.6.22-rc4-mm2.orig/kernel/kmod.c
+++ linux-2.6.22-rc4-mm2/kernel/kmod.c
@@ -41,14 +41,6 @@ extern int max_threads;
static struct workqueue_struct *khelper_wq;
-/*
- * If set, both call_usermodehelper_keys() and call_usermodehelper_pipe() exit
- * immediately returning -EBUSY. Used for preventing user land processes from
- * being created after the user land has been frozen during a system-wide
- * hibernation or suspend operation.
- */
-static int usermodehelper_disabled;
-
#ifdef CONFIG_KMOD
/*
@@ -275,15 +267,54 @@ static void __call_usermodehelper(struct
}
}
+#ifdef CONFIG_PM
+/*
+ * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
+ * (used for preventing user land processes from being created after the user
+ * land has been frozen during a system-wide hibernation or suspend operation).
+ */
+static int usermodehelper_disabled;
+
+/* Number of helpers running */
+static atomic_t running_helpers = ATOMIC_INIT(0);
+
+/*
+ * Wait queue head used by usermodehelper_pm_callback() to wait for all running
+ * helpers to finish.
+ */
+static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
+
+/*
+ * Time to wait for running_helpers to become zero before the setting of
+ * usermodehelper_disabled in usermodehelper_pm_callback() fails
+ */
+#define RUNNING_HELPERS_TIMEOUT (5 * HZ)
+
static int usermodehelper_pm_callback(struct notifier_block *nfb,
unsigned long action,
void *ignored)
{
+ long retval;
+
switch (action) {
case PM_HIBERNATION_PREPARE:
case PM_SUSPEND_PREPARE:
usermodehelper_disabled = 1;
- return NOTIFY_OK;
+ /*
+ * From now on call_usermodehelper_exec() won't start any new
+ * helpers, so it is sufficient if running_helpers turns out to
+ * be zero at one point (it may be increased later, but that
+ * doesn't matter).
+ */
+ retval = wait_event_timeout(running_helpers_waitq,
+ atomic_read(&running_helpers) == 0,
+ RUNNING_HELPERS_TIMEOUT);
+ if (retval) {
+ return NOTIFY_OK;
+ } else {
+ usermodehelper_disabled = 0;
+ return NOTIFY_BAD;
+ }
case PM_POST_HIBERNATION:
case PM_POST_SUSPEND:
usermodehelper_disabled = 0;
@@ -293,6 +324,29 @@ static int usermodehelper_pm_callback(st
return NOTIFY_DONE;
}
+static void new_helper(void)
+{
+ atomic_inc(&running_helpers);
+}
+
+static void helper_finished(void)
+{
+ if (atomic_dec_and_test(&running_helpers))
+ wake_up(&running_helpers_waitq);
+}
+
+static void register_pm_notifier_callback(void)
+{
+ pm_notifier(usermodehelper_pm_callback, 0);
+}
+#else /* CONFIG_PM */
+#define usermodehelper_disabled 0
+
+static inline void new_helper(void) {}
+static inline void helper_finished(void) {}
+static inline void register_pm_notifier_callback(void) {}
+#endif /* CONFIG_PM */
+
/**
* call_usermodehelper_setup - prepare to call a usermode helper
* @path - path to usermode executable
@@ -397,6 +451,7 @@ int call_usermodehelper_exec(struct subp
DECLARE_COMPLETION_ONSTACK(done);
int retval;
+ new_helper();
if (sub_info->path[0] == '\0') {
retval = 0;
goto out;
@@ -418,6 +473,7 @@ int call_usermodehelper_exec(struct subp
out:
call_usermodehelper_freeinfo(sub_info);
+ helper_finished();
return retval;
}
EXPORT_SYMBOL(call_usermodehelper_exec);
@@ -459,5 +515,5 @@ void __init usermodehelper_init(void)
{
khelper_wq = create_singlethread_workqueue("khelper");
BUG_ON(!khelper_wq);
- pm_notifier(usermodehelper_pm_callback, 0);
+ register_pm_notifier_callback();
}
Rafael J. Wysocki wrote:
>
> static int usermodehelper_pm_callback(struct notifier_block *nfb,
> unsigned long action,
> void *ignored)
> {
> + long retval;
> +
> switch (action) {
> case PM_HIBERNATION_PREPARE:
> case PM_SUSPEND_PREPARE:
> usermodehelper_disabled = 1;
> - return NOTIFY_OK;
> + /*
> + * From now on call_usermodehelper_exec() won't start any new
> + * helpers, so it is sufficient if running_helpers turns out to
> + * be zero at one point (it may be increased later, but that
> + * doesn't matter).
> + */
> + retval = wait_event_timeout(running_helpers_waitq,
> + atomic_read(&running_helpers) == 0,
> + RUNNING_HELPERS_TIMEOUT);
> + if (retval) {
> + return NOTIFY_OK;
> + } else {
> + usermodehelper_disabled = 0;
> + return NOTIFY_BAD;
I think this is racy. First, this needs smp_mb() between "usermodehelper_disabled = 1"
and wait_event_timeout().
Second, call_usermodehelper's path should first increment the counter, and only
then check usermodehelper_disabled, and it needs an mb() in between too. Otherwise,
the helper can see usermodehelper_disabled == 0, then PM_SUSPEND_PREPARE comes and
returns NOTIFY_OK, then the helper increments the counter and starts application.
Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
Oleg.
On Monday, 25 June 2007 12:43, Oleg Nesterov wrote:
> Rafael J. Wysocki wrote:
> >
> > static int usermodehelper_pm_callback(struct notifier_block *nfb,
> > unsigned long action,
> > void *ignored)
> > {
> > + long retval;
> > +
> > switch (action) {
> > case PM_HIBERNATION_PREPARE:
> > case PM_SUSPEND_PREPARE:
> > usermodehelper_disabled = 1;
> > - return NOTIFY_OK;
> > + /*
> > + * From now on call_usermodehelper_exec() won't start any new
> > + * helpers, so it is sufficient if running_helpers turns out to
> > + * be zero at one point (it may be increased later, but that
> > + * doesn't matter).
> > + */
> > + retval = wait_event_timeout(running_helpers_waitq,
> > + atomic_read(&running_helpers) == 0,
> > + RUNNING_HELPERS_TIMEOUT);
> > + if (retval) {
> > + return NOTIFY_OK;
> > + } else {
> > + usermodehelper_disabled = 0;
> > + return NOTIFY_BAD;
>
> I think this is racy. First, this needs smp_mb() between "usermodehelper_disabled = 1"
> and wait_event_timeout().
Yes ...
> Second, call_usermodehelper's path should first increment the counter, and only
> then check usermodehelper_disabled,
It does this already.
> and it needs an mb() in between too. Otherwise, the helper can see
> usermodehelper_disabled == 0, then PM_SUSPEND_PREPARE comes and
> returns NOTIFY_OK, then the helper increments the counter and starts application.
>
> Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
OK
If I understand that correctly, it should suffice to place smp_mb() after
usermodehelper_disabled = 1; in usermodehelper_pm_callback() and another
smp_mb() after atomic_inc(&running_helpers); in new_helper().
Is that correct?
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth
On Mon, Jun 25, 2007 at 02:43:32PM +0400, Oleg Nesterov wrote:
> Rafael J. Wysocki wrote:
> >
> > static int usermodehelper_pm_callback(struct notifier_block *nfb,
> > unsigned long action,
> > void *ignored)
> > {
> > + long retval;
> > +
> > switch (action) {
> > case PM_HIBERNATION_PREPARE:
> > case PM_SUSPEND_PREPARE:
> > usermodehelper_disabled = 1;
> > - return NOTIFY_OK;
> > + /*
> > + * From now on call_usermodehelper_exec() won't start any new
> > + * helpers, so it is sufficient if running_helpers turns out to
> > + * be zero at one point (it may be increased later, but that
> > + * doesn't matter).
> > + */
> > + retval = wait_event_timeout(running_helpers_waitq,
> > + atomic_read(&running_helpers) == 0,
> > + RUNNING_HELPERS_TIMEOUT);
> > + if (retval) {
> > + return NOTIFY_OK;
> > + } else {
> > + usermodehelper_disabled = 0;
> > + return NOTIFY_BAD;
>
> I think this is racy. First, this needs smp_mb() between "usermodehelper_disabled = 1"
> and wait_event_timeout().
>
> Second, call_usermodehelper's path should first increment the counter, and only
> then check usermodehelper_disabled, and it needs an mb() in between too. Otherwise,
> the helper can see usermodehelper_disabled == 0, then PM_SUSPEND_PREPARE comes and
> returns NOTIFY_OK, then the helper increments the counter and starts application.
>
> Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
Interesting... So the thought is to have a synchronize_srcu_timeout()
or something similar that waited for a grace period to elapse or for
a timeout to expire, whichever comes first? It should not be too hard
to arrange something, if needed.
Thanx, Paul
On 06/25, Paul E. McKenney wrote:
>
> On Mon, Jun 25, 2007 at 02:43:32PM +0400, Oleg Nesterov wrote:
> >
> > Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
>
> Interesting... So the thought is to have a synchronize_srcu_timeout()
> or something similar that waited for a grace period to elapse or for
> a timeout to expire, whichever comes first? It should not be too hard
> to arrange something, if needed.
Yes. As for qrcu (see http://marc.info/?t=116484476500001), I think it is easy.
First, we add "int interrupted" into struct qrcu_struct, then something like this
long synchronize_qrcu_timeout(struct qrcu_struct *qp, long tout)
{
int idx;
smp_mb();
mutex_lock(&qp->mutex);
again:
idx = qp->completed & 0x1;
if (unlikely(qp->interrupted)) {
idx ^= 0x1;
goto restart;
}
if (atomic_read(qp->ctr + idx) == 1)
goto out;
atomic_inc(qp->ctr + (idx ^ 0x1));
qp->completed++;
atomic_dec(qp->ctr + idx);
restart:
__wait_event_timeout(qp->wq, !atomic_read(qp->ctr + idx), tout);
if (unlikely(!tout)) {
qp->interrupted = 1;
goto out;
}
if (unlikely(qp->interrupted)) {
qp->interrupted = 0;
goto again;
}
out:
mutex_unlock(&qp->mutex);
smp_mb();
return tout;
}
Of course, this needs some other changes to implement synchronize_qrcu() on top
of synchronize_qrcu_timeout(). I also think that this doesn't break
[RFC PATCH] QRCU fastpath optimization
http://marc.info/?l=linux-kernel&m=117125577710947
Looks like we can do something similar for srcu.
Oleg.
On 06/25, Rafael J. Wysocki wrote:
>
> On Monday, 25 June 2007 12:43, Oleg Nesterov wrote:
>
> > Second, call_usermodehelper's path should first increment the counter, and only
> > then check usermodehelper_disabled,
>
> It does this already.
Ah, sorry, I looked at this patch without seeing the underlying code.
BTW, isn't it better to rename new_helper/helper_finished to
helper_lock/helper_unlock ?
> If I understand that correctly, it should suffice to place smp_mb() after
> usermodehelper_disabled = 1; in usermodehelper_pm_callback() and another
> smp_mb() after atomic_inc(&running_helpers); in new_helper().
>
> Is that correct?
Afaics, yes.
Oleg.
On 06/25, Oleg Nesterov wrote:
>
> On 06/25, Paul E. McKenney wrote:
> >
> > On Mon, Jun 25, 2007 at 02:43:32PM +0400, Oleg Nesterov wrote:
> > >
> > > Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
> >
> > Interesting... So the thought is to have a synchronize_srcu_timeout()
> > or something similar that waited for a grace period to elapse or for
> > a timeout to expire, whichever comes first? It should not be too hard
> > to arrange something, if needed.
>
> Yes. As for qrcu (see http://marc.info/?t=116484476500001), I think it is easy.
> First, we add "int interrupted" into struct qrcu_struct, then something like this
Even simpler, we don't need ->interrupted.
long synchronize_qrcu_timeout(struct qrcu_struct *qp, long tout)
{
int idx, prv;
smp_mb();
mutex_lock(&qp->mutex);
idx = qp->completed & 0x1;
prv = idx ^ 0x1;
if (unlikely(atomic_read(qp->ctr + prv))) {
// the previous call has not succeed,
// finish the wait
__wait_event_timeout(qp->wq, !atomic_read(qp->ctr + prv), tout);
if (unlikely(!tout))
goto out;
}
if (atomic_read(qp->ctr + idx) == 1)
goto out;
atomic_inc(qp->ctr + prv);
qp->completed++;
atomic_dec(qp->ctr + idx);
__wait_event_timeout(qp->wq, !atomic_read(qp->ctr + idx), tout);
out:
mutex_unlock(&qp->mutex);
smp_mb();
return tout;
}
Oleg.
On Mon 2007-06-25 19:49:57, Oleg Nesterov wrote:
> On 06/25, Oleg Nesterov wrote:
> >
> > On 06/25, Paul E. McKenney wrote:
> > >
> > > On Mon, Jun 25, 2007 at 02:43:32PM +0400, Oleg Nesterov wrote:
> > > >
> > > > Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
> > >
> > > Interesting... So the thought is to have a synchronize_srcu_timeout()
> > > or something similar that waited for a grace period to elapse or for
> > > a timeout to expire, whichever comes first? It should not be too hard
> > > to arrange something, if needed.
> >
> > Yes. As for qrcu (see http://marc.info/?t=116484476500001), I think it is easy.
> > First, we add "int interrupted" into struct qrcu_struct, then something like this
>
> Even simpler, we don't need ->interrupted.
>
> long synchronize_qrcu_timeout(struct qrcu_struct *qp, long tout)
> {
> int idx, prv;
Could we get less encrypted variable names? tout? Yes, I can decipher it.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Jun 25, 2007 at 07:49:57PM +0400, Oleg Nesterov wrote:
> On 06/25, Oleg Nesterov wrote:
> >
> > On 06/25, Paul E. McKenney wrote:
> > >
> > > On Mon, Jun 25, 2007 at 02:43:32PM +0400, Oleg Nesterov wrote:
> > > >
> > > > Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
> > >
> > > Interesting... So the thought is to have a synchronize_srcu_timeout()
> > > or something similar that waited for a grace period to elapse or for
> > > a timeout to expire, whichever comes first? It should not be too hard
> > > to arrange something, if needed.
> >
> > Yes. As for qrcu (see http://marc.info/?t=116484476500001), I think it is easy.
> > First, we add "int interrupted" into struct qrcu_struct, then something like this
>
> Even simpler, we don't need ->interrupted.
I have to ask...
What sorts of performance characteristics are needed here? The reason
that I ask is because putting a straight synchronize_qrcu() into a
workqueue (or something similar) and then using a timer to provide
any needed wakeup seems a lot simpler than rearranging the innards of
synchronize_qrcu().
(Yes, I am feeling cowardly. Why do you ask?)
Thanx, Paul
> long synchronize_qrcu_timeout(struct qrcu_struct *qp, long tout)
> {
> int idx, prv;
>
> smp_mb();
> mutex_lock(&qp->mutex);
>
> idx = qp->completed & 0x1;
> prv = idx ^ 0x1;
>
> if (unlikely(atomic_read(qp->ctr + prv))) {
> // the previous call has not succeed,
> // finish the wait
> __wait_event_timeout(qp->wq, !atomic_read(qp->ctr + prv), tout);
> if (unlikely(!tout))
> goto out;
> }
>
> if (atomic_read(qp->ctr + idx) == 1)
> goto out;
>
> atomic_inc(qp->ctr + prv);
> qp->completed++;
>
> atomic_dec(qp->ctr + idx);
> __wait_event_timeout(qp->wq, !atomic_read(qp->ctr + idx), tout);
> out:
> mutex_unlock(&qp->mutex);
> smp_mb();
>
> return tout;
> }
>
> Oleg.
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 06/26, Paul E. McKenney wrote:
>
> On Mon, Jun 25, 2007 at 07:49:57PM +0400, Oleg Nesterov wrote:
> > On 06/25, Oleg Nesterov wrote:
> > >
> > > On 06/25, Paul E. McKenney wrote:
> > > >
> > > > On Mon, Jun 25, 2007 at 02:43:32PM +0400, Oleg Nesterov wrote:
> > > > >
> > > > > Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
> > > >
> > > > Interesting... So the thought is to have a synchronize_srcu_timeout()
> > > > or something similar that waited for a grace period to elapse or for
> > > > a timeout to expire, whichever comes first? It should not be too hard
> > > > to arrange something, if needed.
> > >
> > > Yes. As for qrcu (see http://marc.info/?t=116484476500001), I think it is easy.
> > > First, we add "int interrupted" into struct qrcu_struct, then something like this
> >
> > Even simpler, we don't need ->interrupted.
>
> I have to ask...
>
> What sorts of performance characteristics are needed here? The reason
> that I ask is because putting a straight synchronize_qrcu() into a
> workqueue (or something similar) and then using a timer to provide
> any needed wakeup seems a lot simpler than rearranging the innards of
> synchronize_qrcu().
Didn't think about that... But yes, we can implement synchronize_qrcu_timeout()
on top of synchronize_qrcu() as you suggested. Hovewer, this implementation
will add more code to .text compared to changing synchronize_qrcu().
Note also that we can't share the "context" of synchronize_qrcu_timeout() in
that case. Each caller of synchronize_qrcu_timeout() needs a separate
wait_queue_head_t + work_struct. This context can't live on stack, because
it should survive after the timeout.
If we change synchronize_qrcu() instead, we only add
if (unlikely(atomic_read(qp->ctr + prv))) {
__wait_event_timeout(qp->wq, !atomic_read(qp->ctr + prv), tout);
if (unlikely(!tout))
goto out;
}
to the slow path. This has nearly zero perfomance penalty. Yes, we have
to wait if the previous call was interrupted by timeout. But in that case
we lost nothing, we spend the same time waiting for qp->mutex when the
previous call succeeds.
That said, I am not sure synchronize_Xrcu_timeout() is terribly useful.
Rafael could use it, but it is not better for this particular case.
Except the code re-use, which is always good.
Oleg.
On Wednesday, 27 June 2007 13:18, Oleg Nesterov wrote:
> On 06/26, Paul E. McKenney wrote:
> >
> > On Mon, Jun 25, 2007 at 07:49:57PM +0400, Oleg Nesterov wrote:
> > > On 06/25, Oleg Nesterov wrote:
> > > >
> > > > On 06/25, Paul E. McKenney wrote:
> > > > >
> > > > > On Mon, Jun 25, 2007 at 02:43:32PM +0400, Oleg Nesterov wrote:
> > > > > >
> > > > > > Sadly, you can't use srcu/qrcu because it doesn't handle timeouts.
> > > > >
> > > > > Interesting... So the thought is to have a synchronize_srcu_timeout()
> > > > > or something similar that waited for a grace period to elapse or for
> > > > > a timeout to expire, whichever comes first? It should not be too hard
> > > > > to arrange something, if needed.
> > > >
> > > > Yes. As for qrcu (see http://marc.info/?t=116484476500001), I think it is easy.
> > > > First, we add "int interrupted" into struct qrcu_struct, then something like this
> > >
> > > Even simpler, we don't need ->interrupted.
> >
> > I have to ask...
> >
> > What sorts of performance characteristics are needed here? The reason
> > that I ask is because putting a straight synchronize_qrcu() into a
> > workqueue (or something similar) and then using a timer to provide
> > any needed wakeup seems a lot simpler than rearranging the innards of
> > synchronize_qrcu().
>
> Didn't think about that... But yes, we can implement synchronize_qrcu_timeout()
> on top of synchronize_qrcu() as you suggested. Hovewer, this implementation
> will add more code to .text compared to changing synchronize_qrcu().
>
> Note also that we can't share the "context" of synchronize_qrcu_timeout() in
> that case. Each caller of synchronize_qrcu_timeout() needs a separate
> wait_queue_head_t + work_struct. This context can't live on stack, because
> it should survive after the timeout.
>
>
> If we change synchronize_qrcu() instead, we only add
>
> if (unlikely(atomic_read(qp->ctr + prv))) {
> __wait_event_timeout(qp->wq, !atomic_read(qp->ctr + prv), tout);
> if (unlikely(!tout))
> goto out;
> }
>
> to the slow path. This has nearly zero perfomance penalty. Yes, we have
> to wait if the previous call was interrupted by timeout. But in that case
> we lost nothing, we spend the same time waiting for qp->mutex when the
> previous call succeeds.
>
>
> That said, I am not sure synchronize_Xrcu_timeout() is terribly useful.
> Rafael could use it, but it is not better for this particular case.
> Except the code re-use, which is always good.
Well, if there are more cases in which it's useful, we'll be able to modify
this code to use it too in the future.
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth