[PATCH] apm-emulation: use wait_event_freezable() instead of freezer_[do_not_]count()

[PATCH] apm-emulation: use wait_event_freezable() instead of freezer_[do_not_]count()

[Tejun Heo]

vfork is moving away from freezer_[do_not_]count() one way or the
other leaving apm_ioctl() as the only user.  apm_ioctl() just wants to
wait for suspend/resume cycle to complete without hindering the
freezer.  Use wait_event_freezable() instead.

The only annoyance is that wait_event_freezable() wakes up with
-ERESTART if there are pending signals while apm_ioctl() wants to
ignore all signals until suspend is complete.  We can play with
@current->[real_]blocked but this is hardly a performance or latency
critical path - simply chill a bit on each iteration until
SUSPEND_DONE for unlikely cases where there are pending signals.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
---
Compile tested only.  It would be great if someone w/ affected
configuraiton can test this.

Thank you.

 drivers/char/apm-emulation.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Index: work/drivers/char/apm-emulation.c
===================================================================
--- work.orig/drivers/char/apm-emulation.c
+++ work/drivers/char/apm-emulation.c
@@ -300,17 +300,13 @@ apm_ioctl(struct file *filp, u_int cmd, 
 			/*
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
+			 * wait_event_freezable() is interruptible and pending
+			 * signal can cause busy looping.  We aren't doing
+			 * anything critical, chill a bit on each iteration.
 			 */
-			freezer_do_not_count();
-
-			wait_event(apm_suspend_waitqueue,
-				   as->suspend_state == SUSPEND_DONE);
-
-			/*
-			 * Since we are waiting until the suspend is done, the
-			 * try_to_freeze() in freezer_count() will not trigger
-			 */
-			freezer_count();
+			while (wait_event_freezable(apm_suspend_waitqueue,
+					as->suspend_state == SUSPEND_DONE))
+				msleep(10);
 			break;
 		case SUSPEND_ACKTO:
 			as->suspend_result = -ETIMEDOUT;

Re: [PATCH] apm-emulation: use wait_event_freezable() instead of freezer_[do_not_]count()

[Oleg Nesterov]

Hi Tejun,

The patch looks fine even if I know nothing about this code.

But I have a bit off-topic question,

On 08/18, Tejun Heo wrote:
>
> vfork is moving away from freezer_[do_not_]count() one way or the
> other

Yes, I think we should do this in any case.

> Use wait_event_freezable() instead.
>
> The only annoyance is that wait_event_freezable() wakes up with
> -ERESTART if there are pending signals

IOW, we do not have wait_event_freezable_uninterruptible/etc.

Perhaps we can introduce TASK_FREEZABLE ? It should be used along
with TASK_UNINTERRUPTIBLE (like TASK_WAKEKILL). freeze_task() can
use TASK_INTERRUPTIBLE | FREEZABLE for wake_up.

vfork() can use FREEZABLE too (although this is not needed in the
long term, we should teach it to sleep in TASK_INTERRUPTIBLE).



But I feel you are going to reimplement freezer completely, in this
case please ignore.

Oleg.

Re: [PATCH] apm-emulation: use wait_event_freezable() instead of freezer_[do_not_]count()

[Tejun Heo]

Hello,

On Thu, Aug 18, 2011 at 07:35:11PM +0200, Oleg Nesterov wrote:
> > Use wait_event_freezable() instead.
> >
> > The only annoyance is that wait_event_freezable() wakes up with
> > -ERESTART if there are pending signals
> 
> IOW, we do not have wait_event_freezable_uninterruptible/etc.

Yeap, we can add it but let's leave it alone for now.

> Perhaps we can introduce TASK_FREEZABLE ? It should be used along
> with TASK_UNINTERRUPTIBLE (like TASK_WAKEKILL). freeze_task() can
> use TASK_INTERRUPTIBLE | FREEZABLE for wake_up.
> 
> vfork() can use FREEZABLE too (although this is not needed in the
> long term, we should teach it to sleep in TASK_INTERRUPTIBLE).
> 
> But I feel you are going to reimplement freezer completely, in this
> case please ignore.

Hmmm... yeah, I just posted preparation patches for the freezer and
will now try to integrate with the rest of job control stuff.  Not
quite sure how it would turn out yet.  Let's see.

Thank you.

-- 
tejun

Re: [PATCH] apm-emulation: use wait_event_freezable() instead of freezer_[do_not_]count()

[Jiri Kosina]

On Thu, 18 Aug 2011, Tejun Heo wrote:

> vfork is moving away from freezer_[do_not_]count() one way or the
> other leaving apm_ioctl() as the only user.  apm_ioctl() just wants to
> wait for suspend/resume cycle to complete without hindering the
> freezer.  Use wait_event_freezable() instead.
> 
> The only annoyance is that wait_event_freezable() wakes up with
> -ERESTART if there are pending signals while apm_ioctl() wants to
> ignore all signals until suspend is complete.  We can play with
> @current->[real_]blocked but this is hardly a performance or latency
> critical path - simply chill a bit on each iteration until
> SUSPEND_DONE for unlikely cases where there are pending signals.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
> Compile tested only.  It would be great if someone w/ affected
> configuraiton can test this.
> 
> Thank you.
> 
>  drivers/char/apm-emulation.c |   16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> Index: work/drivers/char/apm-emulation.c
> ===================================================================
> --- work.orig/drivers/char/apm-emulation.c
> +++ work/drivers/char/apm-emulation.c
> @@ -300,17 +300,13 @@ apm_ioctl(struct file *filp, u_int cmd, 
>  			/*
>  			 * Wait for the suspend/resume to complete.  If there
>  			 * are pending acknowledges, we wait here for them.
> +			 * wait_event_freezable() is interruptible and pending
> +			 * signal can cause busy looping.  We aren't doing
> +			 * anything critical, chill a bit on each iteration.
>  			 */
> -			freezer_do_not_count();
> -
> -			wait_event(apm_suspend_waitqueue,
> -				   as->suspend_state == SUSPEND_DONE);
> -
> -			/*
> -			 * Since we are waiting until the suspend is done, the
> -			 * try_to_freeze() in freezer_count() will not trigger
> -			 */
> -			freezer_count();
> +			while (wait_event_freezable(apm_suspend_waitqueue,
> +					as->suspend_state == SUSPEND_DONE))
> +				msleep(10);
>  			break;
>  		case SUSPEND_ACKTO:
>  			as->suspend_result = -ETIMEDOUT;

Applied, thanks Tejun.

-- 
Jiri Kosina
SUSE Labs