login
Search
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, Well, no, AFAICS. The freezer doesn't regard the current task as freezable. Greetings, Rafael -
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
-
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) 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 -
...I would just revert this bits from now to make sure this driver 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 -
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. 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 -
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 <rjw@sisk.pl> 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 <rjw@sisk.pl> --- 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); } -
I've never claimed this. I just said it may be safer to revert the 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 -
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 -
As I said I don't know. It's probably time to put some people BTW, why not raising PF_NOFREEZE in wait_event(), so thread sleeping in UNINTERRUPTIBLE state won't prevent suspend to happen ? Franck -
OK, never mind. I think the patch below is the right fix.
---
From: Rafael J. Wysocki <rjw@sisk.pl>
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 <rjw@sisk.pl>
---
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;
-
...[