Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
> Actually, spinlock_t is not suitable. Because spin_unlcok() does NOT imply > mb(). The subsequent wait_event_timeout()->atomic_read() may leak into the > critical section. > > We can use set_mb(), if we don't want to play with smp_mb() by hand :) spin_unlock implies a smp_wmb() but yeah, not a full mb(), though having a read leak into a critical section is generally not an issue. Ben. - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On 07/03, Benjamin Herrenschmidt wrote: > > On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: > > > > case PM_HIBERNATION_PREPARE: > > > > case PM_SUSPEND_PREPARE: > > > > usermodehelper_disabled = 1; > > > > - return NOTIFY_OK; > > > > + smp_mb(); > > > > > > usermodehelper_disabled should be atomic variable, too, so we don't > > > have to play these ugly tricks by hand? This should not be > > > performance-critical, right? > > > > Well, I think we'd need to add the barriers anyway. > > > > The problem, as far as I understand it, is that the instructions can > > get > > reordered if there are no barriers in there. > > That seems dodgy either way to me :-) > > Just use a spinlock. Actually, spinlock_t is not suitable. Because spin_unlcok() does NOT imply mb(). The subsequent wait_event_timeout()->atomic_read() may leak into the critical section. We can use set_mb(), if we don't want to play with smp_mb() by hand :) 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tuesday, 3 July 2007 15:19, Rafael J. Wysocki wrote: > On Tuesday, 3 July 2007 07:30, Benjamin Herrenschmidt wrote: > > On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: > > > > > case PM_HIBERNATION_PREPARE: > > > > > case PM_SUSPEND_PREPARE: > > > > > usermodehelper_disabled = 1; > > > > > - return NOTIFY_OK; > > > > > + smp_mb(); > > > > > > > > usermodehelper_disabled should be atomic variable, too, so we don't > > > > have to play these ugly tricks by hand? This should not be > > > > performance-critical, right? > > > > > > Well, I think we'd need to add the barriers anyway. > > > > > > The problem, as far as I understand it, is that the instructions can > > > get > > > reordered if there are no barriers in there. > > > > That seems dodgy either way to me :-) > > > > Just use a spinlock. > > Around wait_for_completion()? I don't think that's a good idea. :-) Sorry, I mistunderstood you (I think). Yes, I could use a spinlock for protecting usermodehelper_disabled, but why would that be better than the current code? 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tuesday, 3 July 2007 07:30, Benjamin Herrenschmidt wrote: > On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: > > > > case PM_HIBERNATION_PREPARE: > > > > case PM_SUSPEND_PREPARE: > > > > usermodehelper_disabled = 1; > > > > - return NOTIFY_OK; > > > > + smp_mb(); > > > > > > usermodehelper_disabled should be atomic variable, too, so we don't > > > have to play these ugly tricks by hand? This should not be > > > performance-critical, right? > > > > Well, I think we'd need to add the barriers anyway. > > > > The problem, as far as I understand it, is that the instructions can > > get > > reordered if there are no barriers in there. > > That seems dodgy either way to me :-) > > Just use a spinlock. Around wait_for_completion()? I don't think that's a good idea. :-) 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tuesday, 3 July 2007 07:30, Benjamin Herrenschmidt wrote: On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. That seems dodgy either way to me :-) Just use a spinlock. Around wait_for_completion()? I don't think that's a good idea. :-) 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tuesday, 3 July 2007 15:19, Rafael J. Wysocki wrote: On Tuesday, 3 July 2007 07:30, Benjamin Herrenschmidt wrote: On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. That seems dodgy either way to me :-) Just use a spinlock. Around wait_for_completion()? I don't think that's a good idea. :-) Sorry, I mistunderstood you (I think). Yes, I could use a spinlock for protecting usermodehelper_disabled, but why would that be better than the current code? 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On 07/03, Benjamin Herrenschmidt wrote: On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. That seems dodgy either way to me :-) Just use a spinlock. Actually, spinlock_t is not suitable. Because spin_unlcok() does NOT imply mb(). The subsequent wait_event_timeout()-atomic_read() may leak into the critical section. We can use set_mb(), if we don't want to play with smp_mb() by hand :) 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
Actually, spinlock_t is not suitable. Because spin_unlcok() does NOT imply mb(). The subsequent wait_event_timeout()-atomic_read() may leak into the critical section. We can use set_mb(), if we don't want to play with smp_mb() by hand :) spin_unlock implies a smp_wmb() but yeah, not a full mb(), though having a read leak into a critical section is generally not an issue. Ben. - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tue, 2007-06-26 at 10:48 +0200, Pavel Machek wrote: > > The problem, as far as I understand it, is that the instructions can > get > > reordered if there are no barriers in there. > > Are you sure? I thought atomic variables have barrirers built-in. bzzzt. they don't. Ben. - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: > > > case PM_HIBERNATION_PREPARE: > > > case PM_SUSPEND_PREPARE: > > > usermodehelper_disabled = 1; > > > - return NOTIFY_OK; > > > + smp_mb(); > > > > usermodehelper_disabled should be atomic variable, too, so we don't > > have to play these ugly tricks by hand? This should not be > > performance-critical, right? > > Well, I think we'd need to add the barriers anyway. > > The problem, as far as I understand it, is that the instructions can > get > reordered if there are no barriers in there. That seems dodgy either way to me :-) Just use a spinlock. Ben. - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tue, 2007-06-26 at 10:48 +0200, Pavel Machek wrote: The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. Are you sure? I thought atomic variables have barrirers built-in. bzzzt. they don't. Ben. - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tue, 2007-06-26 at 00:27 +0200, Rafael J. Wysocki wrote: case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. That seems dodgy either way to me :-) Just use a spinlock. Ben. - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
Hi! > > > [I hope the ACKs still apply.] > > > > Uhuh, not 100% sure. > > > > > +static int usermodehelper_disabled; > > > + > > > > > > ... > > > > > case PM_HIBERNATION_PREPARE: > > > case PM_SUSPEND_PREPARE: > > > usermodehelper_disabled = 1; > > > - return NOTIFY_OK; > > > + smp_mb(); > > > > usermodehelper_disabled should be atomic variable, too, so we don't > > have to play these ugly tricks by hand? This should not be > > performance-critical, right? > > Well, I think we'd need to add the barriers anyway. > > The problem, as far as I understand it, is that the instructions can get > reordered if there are no barriers in there. Are you sure? I thought atomic variables have barrirers built-in. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On 06/26, Rafael J. Wysocki wrote: > > On Monday, 25 June 2007 23:55, Pavel Machek wrote: > > > > > case PM_HIBERNATION_PREPARE: > > > case PM_SUSPEND_PREPARE: > > > usermodehelper_disabled = 1; > > > - return NOTIFY_OK; > > > + smp_mb(); > > > > usermodehelper_disabled should be atomic variable, too, so we don't > > have to play these ugly tricks by hand? This should not be > > performance-critical, right? > > Well, I think we'd need to add the barriers anyway. > > The problem, as far as I understand it, is that the instructions can get > reordered if there are no barriers in there. Yes, and it doesn't help if we make usermodehelper_disabled atomic_t. atomic_xxx() operations do not imply the memory barrier semantics. 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Monday, 25. June 2007, Rafael J. Wysocki wrote:
> [I hope the ACKs still apply.]
>
Mine does
> +static void helper_lock(void)
> +{
> + atomic_inc(_helpers);
> + smp_mb__after_atomic_inc();
> +}
> +
> +static void helper_unlock(void)
> +{
> + if (atomic_dec_and_test(_helpers))
> + wake_up(_helpers_waitq);
> +}
> +
> +static void register_pm_notifier_callback(void)
> +{
> + pm_notifier(usermodehelper_pm_callback, 0);
> +}
> +#else /* CONFIG_PM */
> +static inline void helper_lock(void) {}
> +static inline void helper_unlock(void) {}
> +static inline void register_pm_notifier_callback(void) {}
>
Sorry for not realising this before, but I think, we should also inline the
actual implementations of helper_lock, helper_unlock and
register_pm_notifier_callback
Regards
Uli
--
--- ROAD ...the handyPC Company - - - ) ) )
Uli Luckas
Software Development
ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de
Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit
-
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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Monday, 25. June 2007, Rafael J. Wysocki wrote:
[I hope the ACKs still apply.]
Mine does
+static void helper_lock(void)
+{
+ atomic_inc(running_helpers);
+ smp_mb__after_atomic_inc();
+}
+
+static void helper_unlock(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 */
+static inline void helper_lock(void) {}
+static inline void helper_unlock(void) {}
+static inline void register_pm_notifier_callback(void) {}
Sorry for not realising this before, but I think, we should also inline the
actual implementations of helper_lock, helper_unlock and
register_pm_notifier_callback
Regards
Uli
--
--- ROAD ...the handyPC Company - - - ) ) )
Uli Luckas
Software Development
ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de
Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit
-
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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On 06/26, Rafael J. Wysocki wrote: On Monday, 25 June 2007 23:55, Pavel Machek wrote: case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. Yes, and it doesn't help if we make usermodehelper_disabled atomic_t. atomic_xxx() operations do not imply the memory barrier semantics. 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
Hi! [I hope the ACKs still apply.] Uhuh, not 100% sure. +static int usermodehelper_disabled; + ... case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. Are you sure? I thought atomic variables have barrirers built-in. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Monday, 25 June 2007 23:55, Pavel Machek wrote: > Hi! > > > [I hope the ACKs still apply.] > > Uhuh, not 100% sure. > > > +static int usermodehelper_disabled; > > + > > > ... > > > case PM_HIBERNATION_PREPARE: > > case PM_SUSPEND_PREPARE: > > usermodehelper_disabled = 1; > > - return NOTIFY_OK; > > + smp_mb(); > > usermodehelper_disabled should be atomic variable, too, so we don't > have to play these ugly tricks by hand? This should not be > performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
Hi! > [I hope the ACKs still apply.] Uhuh, not 100% sure. > +static int usermodehelper_disabled; > + ... > case PM_HIBERNATION_PREPARE: > case PM_SUSPEND_PREPARE: > usermodehelper_disabled = 1; > - return NOTIFY_OK; > + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tuesday 26 June 2007 07:52:23 Rafael J. Wysocki wrote: > [I hope the ACKs still apply.] Mine does :> Nigel pgp9gRcYEcqWK.pgp Description: PGP signature
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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(_helpers); in new_helper(). > > Is that correct? Afaics, yes. 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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(_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
-
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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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(_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(_helpers); in new_helper().
Is that correct?
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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(_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.
-
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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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.
-
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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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
-
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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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
-
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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks
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. - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Tuesday 26 June 2007 07:52:23 Rafael J. Wysocki wrote: [I hope the ACKs still apply.] Mine does : Nigel pgp9gRcYEcqWK.pgp Description: PGP signature
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
Hi! [I hope the ACKs still apply.] Uhuh, not 100% sure. +static int usermodehelper_disabled; + ... case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - 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/
Re: [PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks (rev. 2)
On Monday, 25 June 2007 23:55, Pavel Machek wrote: Hi! [I hope the ACKs still apply.] Uhuh, not 100% sure. +static int usermodehelper_disabled; + ... case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + smp_mb(); usermodehelper_disabled should be atomic variable, too, so we don't have to play these ugly tricks by hand? This should not be performance-critical, right? Well, I think we'd need to add the barriers anyway. The problem, as far as I understand it, is that the instructions can get reordered if there are no barriers in there. 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
