Rafael,
Looking at commit:
831441862956fffa17b9801db37e6ea1650b0f69
Freezer: make kernel threads nonfreezable by default
it seems that you broke the apm emulation driver.
You removed PF_NOFREEZE flag setting in apm_ioctl(), which is
definitely not part of the apm kernel daemon but instead is called by
user space proccesses...
I'm just reading this code for the first time so I can be wrong but it
looks like it's not going to work anymore.
Could you confirm ?
Franck
Hi,
On Friday, 16 of November 2007, Franck Bui-Huu wrote:
> Rafael,
>
> Looking at commit:
>
> 831441862956fffa17b9801db37e6ea1650b0f69
> Freezer: make kernel threads nonfreezable by default
>
> it seems that you broke the apm emulation driver.
>
> You removed PF_NOFREEZE flag setting in apm_ioctl(), which is
> definitely not part of the apm kernel daemon but instead is called by
> user space proccesses...
Yes ...
> I'm just reading this code for the first time so I can be wrong but it
> looks like it's not going to work anymore.
>
> Could you confirm ?
Well, no, AFAICS.
The freezer doesn't regard the current task as freezable.
Greetings,
Rafael
On Nov 16, 2007 5:20 PM, Rafael J. Wysocki <[email protected]> wrote:
>
> The freezer doesn't regard the current task as freezable.
>
Hmm, I don't get your point.
If I understood this driver correctly, several processes can be
waiting for a suspend event by reading /dev/apm_bios, apmd (the _user_
space daemon) can be one of them.
Then another process asks to suspend the system by calling 'apm -s',
which results in a apm_ioctl() call. This process will basically
execute:
err = queue_suspend_event(APM_USER_SUSPEND, as);
flags = current->flags;
wait_event_interruptible(apm_suspend_waitqueue,
as->suspend_state == SUSPEND_DONE);
It's basically waiting for the waiters to ack the event. But it won't
be the process that is going to suspend the system, right ?
So now all waiting processes are waken up and need to acknolwedge the
event for the system to actually suspend. So they need to call
apm_ioctl(). They'll basically do:
flags = current->flags;
wait_event(apm_suspend_waitqueue,
as->suspend_state == SUSPEND_DONE);
Except for the last acknowledging process which will do instead:
apm_suspend();
It's a call to pm_suspend().
So you can see that the process which initiates the suspend, the one
that calls 'apm -s', is not the current process but is going to be
waken up by the fake signal sent by freeze_task().
One of the consequence I can see is at this time 'as->result' won't be
setup, so the return value of apm_ioctl() may be wrong.
As I said, I'm not familiar with this code, so please correct me if
I'm wrong.
BTW, how does try_to_freeze_tasks() deal with user land thread waiting
in the UNINTERRUPTIBLE state ?
Thanks.
Franck
On Friday, 16 of November 2007, Franck Bui-Huu wrote:
> On Nov 16, 2007 5:20 PM, Rafael J. Wysocki <[email protected]> wrote:
> >
> > The freezer doesn't regard the current task as freezable.
> >
>
>
> Hmm, I don't get your point.
>
> If I understood this driver correctly, several processes can be
> waiting for a suspend event by reading /dev/apm_bios, apmd (the _user_
> space daemon) can be one of them.
>
> Then another process asks to suspend the system by calling 'apm -s',
> which results in a apm_ioctl() call. This process will basically
> execute:
>
> err = queue_suspend_event(APM_USER_SUSPEND, as);
> flags = current->flags;
> wait_event_interruptible(apm_suspend_waitqueue,
> as->suspend_state == SUSPEND_DONE);
>
> It's basically waiting for the waiters to ack the event. But it won't
> be the process that is going to suspend the system, right ?
>
> So now all waiting processes are waken up and need to acknolwedge the
> event for the system to actually suspend. So they need to call
> apm_ioctl(). They'll basically do:
>
> flags = current->flags;
> wait_event(apm_suspend_waitqueue,
> as->suspend_state == SUSPEND_DONE);
>
> Except for the last acknowledging process which will do instead:
>
> apm_suspend();
>
> It's a call to pm_suspend().
>
> So you can see that the process which initiates the suspend, the one
> that calls 'apm -s', is not the current process but is going to be
> waken up by the fake signal sent by freeze_task().
>
> One of the consequence I can see is at this time 'as->result' won't be
> setup, so the return value of apm_ioctl() may be wrong.
Ah, that. Yes, I see your point.
However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
a good idea.
I'd probably use wait_event_freezable() (defined in include/linux/freezer.h)
for that.
> As I said, I'm not familiar with this code, so please correct me if
> I'm wrong.
No, you're not wrong and I have overlooked the problem.
> BTW, how does try_to_freeze_tasks() deal with user land thread waiting
> in the UNINTERRUPTIBLE state ?
It tries to send them fake signals and waits for them to freeze. If they don't
freeze within the timeout, it fails and clears their TIF_FREEZE bits.
Greetings,
Rafael
Rafael J. Wysocki wrote:
> However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> a good idea.
>
Indeed but...
> I'd probably use wait_event_freezable() (defined in
> include/linux/freezer.h) for that.
...I would just revert this bits from now to make sure this driver
work again for v2.6.24.
> It tries to send them fake signals and waits for them to freeze. If
> they don't freeze within the timeout, it fails and clears their
> TIF_FREEZE bits.
But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
only. Looking at signal_wake_up(), it basically do:
wake_up_state(t, TASK_INTERRUPTIBLE);
What am I missing ?
Franck
On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> > a good idea.
> >
>
> Indeed but...
>
> > I'd probably use wait_event_freezable() (defined in
> > include/linux/freezer.h) for that.
>
> ...I would just revert this bits from now to make sure this driver
> work again for v2.6.24.
I'd prefer not to.
The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
reintroduce it now.
Why do you think that using wait_event_freezable() would not work, BTW?
> > It tries to send them fake signals and waits for them to freeze. If
> > they don't freeze within the timeout, it fails and clears their
> > TIF_FREEZE bits.
>
> But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
> only. Looking at signal_wake_up(), it basically do:
>
> wake_up_state(t, TASK_INTERRUPTIBLE);
>
> What am I missing ?
Nothing. :-)
I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
explicitly in there (should have looked at the current code before replying).
Greetings,
Rafael
On Saturday, 17 of November 2007, Rafael J. Wysocki wrote:
> On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> > Rafael J. Wysocki wrote:
> > > However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> > > a good idea.
> > >
> >
> > Indeed but...
> >
> > > I'd probably use wait_event_freezable() (defined in
> > > include/linux/freezer.h) for that.
> >
> > ...I would just revert this bits from now to make sure this driver
> > work again for v2.6.24.
>
> I'd prefer not to.
>
> The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
> reintroduce it now.
>
> Why do you think that using wait_event_freezable() would not work, BTW?
>
> > > It tries to send them fake signals and waits for them to freeze. If
> > > they don't freeze within the timeout, it fails and clears their
> > > TIF_FREEZE bits.
> >
> > But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
> > only. Looking at signal_wake_up(), it basically do:
> >
> > wake_up_state(t, TASK_INTERRUPTIBLE);
> >
> > What am I missing ?
>
> Nothing. :-)
>
> I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
> explicitly in there (should have looked at the current code before replying).
Actually, not even that one. You're right anyway.
Below is a patch that IMO should fix the issue with apm_ioctl().
Greetings,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
The code in apm_ioctl() allows user space tasks waiting for a suspend to
complete to be woken up prematurely as a result of the thawing of tasks carried
out by the freezer. Fix it.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/char/apm-emulation.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.orig/drivers/char/apm-emulation.c
+++ linux-2.6/drivers/char/apm-emulation.c
@@ -364,7 +364,7 @@ apm_ioctl(struct inode * inode, struct f
*/
flags = current->flags;
- wait_event_interruptible(apm_suspend_waitqueue,
+ wait_event_freezable(apm_suspend_waitqueue,
as->suspend_state == SUSPEND_DONE);
}
Rafael J. Wysocki wrote:
> On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
>> Rafael J. Wysocki wrote:
>>> However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
>>> a good idea.
>>>
>> Indeed but...
>>
>>> I'd probably use wait_event_freezable() (defined in
>>> include/linux/freezer.h) for that.
>> ...I would just revert this bits from now to make sure this driver
>> work again for v2.6.24.
>
> I'd prefer not to.
>
> The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
> reintroduce it now.
>
> Why do you think that using wait_event_freezable() would not work, BTW?
>
I've never claimed this. I just said it may be safer to revert the
changes for v2.6.24 and improve the current code for next releases.
>>> It tries to send them fake signals and waits for them to freeze. If
>>> they don't freeze within the timeout, it fails and clears their
>>> TIF_FREEZE bits.
>> But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
>> only. Looking at signal_wake_up(), it basically do:
>>
>> wake_up_state(t, TASK_INTERRUPTIBLE);
>>
>> What am I missing ?
>
> Nothing. :-)
>
> I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
> explicitly in there (should have looked at the current code before replying).
>
ok so now we agreed on this point, can we assert that a user
land thread waiting for an event in an UNINTERRUPTIBLE state
will prevent a suspend to happen ?
Franck
On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> >> Rafael J. Wysocki wrote:
> >>> However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> >>> a good idea.
> >>>
> >> Indeed but...
> >>
> >>> I'd probably use wait_event_freezable() (defined in
> >>> include/linux/freezer.h) for that.
> >> ...I would just revert this bits from now to make sure this driver
> >> work again for v2.6.24.
> >
> > I'd prefer not to.
> >
> > The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
> > reintroduce it now.
> >
> > Why do you think that using wait_event_freezable() would not work, BTW?
> >
>
> I've never claimed this. I just said it may be safer to revert the
> changes for v2.6.24 and improve the current code for next releases.
>
> >>> It tries to send them fake signals and waits for them to freeze. If
> >>> they don't freeze within the timeout, it fails and clears their
> >>> TIF_FREEZE bits.
> >> But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
> >> only. Looking at signal_wake_up(), it basically do:
> >>
> >> wake_up_state(t, TASK_INTERRUPTIBLE);
> >>
> >> What am I missing ?
> >
> > Nothing. :-)
> >
> > I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
> > explicitly in there (should have looked at the current code before replying).
> >
>
> ok so now we agreed on this point, can we assert that a user
> land thread waiting for an event in an UNINTERRUPTIBLE state
> will prevent a suspend to happen ?
Yes.
Rafael
Rafael J. Wysocki wrote:
> On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
>> ok so now we agreed on this point, can we assert that a user
>> land thread waiting for an event in an UNINTERRUPTIBLE state
>> will prevent a suspend to happen ?
>
> Yes.
>
So this driver seems really broken and actually I'm wondering if
it's used by anyone...
See the call to wait_even() made by apm_ioctl(). If any processes
run this, it will prevent the system to suspend...
And no, I don't know why call wait_event() is called.
Franck
On Sunday, 18 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> >> ok so now we agreed on this point, can we assert that a user
> >> land thread waiting for an event in an UNINTERRUPTIBLE state
> >> will prevent a suspend to happen ?
> >
> > Yes.
> >
>
> So this driver seems really broken and actually I'm wondering if
> it's used by anyone...
Well, it doesn't seem so.
> See the call to wait_even() made by apm_ioctl(). If any processes
> run this, it will prevent the system to suspend...
True, but does it actually happen in practice?
> And no, I don't know why call wait_event() is called.
I hope somebody knows. :-)
At this point the second branch of the "if (as->suspend_state == SUSPEND_READ)"
can be fixed by replacing wait_event_interruptible() with
wait_event_freezable(), but the fix for the first branch depends on whether or
not the wait_event() is really necessary.
If it can be replaced with an interruptible sleep, we can use
wait_event_freezable() in this case too. Otherwise, the only woking fix would
be to reintroduce the PF_NOFREEZE in there.
Honestly, I'm leaning towards replacing wait_event() in apm_ioctl() with
wait_event_freezable() and seeing what happens ...
Greetings,
Rafael
Rafael J. Wysocki wrote:
> On Sunday, 18 of November 2007, Franck Bui-Huu wrote:
>> Rafael J. Wysocki wrote:
>> See the call to wait_even() made by apm_ioctl(). If any processes
>> run this, it will prevent the system to suspend...
>
> True, but does it actually happen in practice?
>
when several processes are waiting for a suspend event.
>
> At this point the second branch of the "if (as->suspend_state == SUSPEND_READ)"
> can be fixed by replacing wait_event_interruptible() with
> wait_event_freezable(),
yes
> but the fix for the first branch depends on whether or
> not the wait_event() is really necessary.
As I said I don't know. It's probably time to put some people
on CC but don't know who though.
>
> If it can be replaced with an interruptible sleep, we can use
> wait_event_freezable() in this case too. Otherwise, the only woking fix would
> be to reintroduce the PF_NOFREEZE in there.
BTW, why not raising PF_NOFREEZE in wait_event(), so thread sleeping
in UNINTERRUPTIBLE state won't prevent suspend to happen ?
Franck
On Monday, 19 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > On Sunday, 18 of November 2007, Franck Bui-Huu wrote:
> >> Rafael J. Wysocki wrote:
> >> See the call to wait_even() made by apm_ioctl(). If any processes
> >> run this, it will prevent the system to suspend...
> >
> > True, but does it actually happen in practice?
> >
>
> when several processes are waiting for a suspend event.
>
> >
> > At this point the second branch of the "if (as->suspend_state == SUSPEND_READ)"
> > can be fixed by replacing wait_event_interruptible() with
> > wait_event_freezable(),
>
> yes
>
> > but the fix for the first branch depends on whether or
> > not the wait_event() is really necessary.
>
> As I said I don't know. It's probably time to put some people
> on CC but don't know who though.
OK, never mind. I think the patch below is the right fix.
---
From: Rafael J. Wysocki <[email protected]>
The APM emulation is currently broken as a result of commit
831441862956fffa17b9801db37e6ea1650b0f69
"Freezer: make kernel threads nonfreezable by default"
that removed the PF_NOFREEZE annotations from apm_ioctl() without adding
the appropriate freezer hooks. Fix it and remove the unnecessary variable flags
from apm_ioctl().
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/char/apm-emulation.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
Index: linux-2.6/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.orig/drivers/char/apm-emulation.c
+++ linux-2.6/drivers/char/apm-emulation.c
@@ -295,7 +295,6 @@ static int
apm_ioctl(struct inode * inode, struct file *filp, u_int cmd, u_long arg)
{
struct apm_user *as = filp->private_data;
- unsigned long flags;
int err = -EINVAL;
if (!as->suser || !as->writer)
@@ -331,10 +330,16 @@ apm_ioctl(struct inode * inode, struct f
* Wait for the suspend/resume to complete. If there
* are pending acknowledges, we wait here for them.
*/
- flags = current->flags;
+ 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();
} else {
as->suspend_state = SUSPEND_WAIT;
mutex_unlock(&state_lock);
@@ -362,14 +367,10 @@ apm_ioctl(struct inode * inode, struct f
* Wait for the suspend/resume to complete. If there
* are pending acknowledges, we wait here for them.
*/
- flags = current->flags;
-
- wait_event_interruptible(apm_suspend_waitqueue,
+ wait_event_freezable(apm_suspend_waitqueue,
as->suspend_state == SUSPEND_DONE);
}
- current->flags = flags;
-
mutex_lock(&state_lock);
err = as->suspend_result;
as->suspend_state = SUSPEND_NONE;