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

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

[Oleg Nesterov]

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.

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

[Paul E. McKenney]

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

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

[Rafael J. Wysocki]

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

synchronize_qrcu_timeout()

[Oleg Nesterov]

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.

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

[Oleg Nesterov]

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.

Re: synchronize_qrcu_timeout()

[Oleg Nesterov]

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.

Re: synchronize_qrcu_timeout()

[Pavel Machek]

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

Re: synchronize_qrcu_timeout()

[Paul E. McKenney]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: synchronize_qrcu_timeout()

[Oleg Nesterov]

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.

Re: synchronize_qrcu_timeout()

[Rafael J. Wysocki]

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