Hi,
The following series of patches implements the changes to the task freezer
that should close the remaining races in it and harden it before it's used for
the CPU hotplugging.
Not all of the patches are from me, but I've decided to make the series out
of all freezer-related patches that have been posted recently. If I have
missed some of them, please let me know.
Most of the patches in this series have been already discussed, so they
should be fine, but of course additional comments are welcome.
Greetings,
Rafael
From: Rafael J. Wysocki <[email protected]>
If the freezing of tasks fails and a task is preempted in refrigerator() before
calling frozen_process(), then thaw_tasks() may run before this task is frozen.
In that case the task will freeze and no one will thaw it.
To fix this race we can call freezing(current) in refrigerator() along with
frozen_process(current) under the task_lock() which also should be taken in
the error path of try_to_freeze_tasks() as well as in thaw_process(). Moreover,
if thaw_process() additionally clears TIF_FREEZE for tasks that are not frozen,
we can be sure that all tasks are thawed and there are no pending "freeze"
requests after thaw_tasks() has run.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
include/linux/freezer.h | 4 ++++
kernel/power/process.c | 12 +++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
Index: linux-2.6.20-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -40,11 +40,15 @@ static inline void do_not_freeze(struct
*/
static inline int thaw_process(struct task_struct *p)
{
+ task_lock(p);
if (frozen(p)) {
p->flags &= ~PF_FROZEN;
+ task_unlock(p);
wake_up_process(p);
return 1;
}
+ clear_tsk_thread_flag(p, TIF_FREEZE);
+ task_unlock(p);
return 0;
}
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -39,10 +39,18 @@ void refrigerator(void)
/* Hmm, should we be allowed to suspend when there are realtime
processes around? */
long save;
+
+ task_lock(current);
+ if (freezing(current)) {
+ frozen_process(current);
+ task_unlock(current);
+ } else {
+ task_unlock(current);
+ return;
+ }
save = current->state;
pr_debug("%s entered refrigerator\n", current->comm);
- frozen_process(current);
spin_lock_irq(¤t->sighand->siglock);
recalc_sigpending(); /* We sent fake signal, clean it up */
spin_unlock_irq(¤t->sighand->siglock);
@@ -159,10 +167,12 @@ static unsigned int try_to_freeze_tasks(
if (is_user_space(p) == !freeze_user_space)
continue;
+ task_lock(p);
if (freezeable(p) && !frozen(p))
printk(KERN_ERR " %s\n", p->comm);
cancel_freezing(p);
+ task_unlock(p);
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
}
From: Oleg Nesterov <[email protected]>
refrigerator() can miss a wakeup, "wait event" loop needs a proper memory
ordering.
Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Pavel Machek <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
kernel/power/process.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -47,8 +47,10 @@ void refrigerator(void)
recalc_sigpending(); /* We sent fake signal, clean it up */
spin_unlock_irq(¤t->sighand->siglock);
- while (frozen(current)) {
- current->state = TASK_UNINTERRUPTIBLE;
+ for (;;) {
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ if (!frozen(current))
+ break;
schedule();
}
pr_debug("%s left refrigerator\n", current->comm);
From: Rafael J. Wysocki <[email protected]>
Currently try_to_freeze_tasks() has to wait until all of the vforked processes
exit and for this reason every user can make it fail. To fix this problem
we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
that do not want to be counted as freezable by the freezer and want to have
TIF_FREEZE set nevertheless. Then, this flag can be set by tasks using
sys_vfork() before they call wait_for_completion() and cleared after they have
woken up and called try_to_freeze(). In case such a task freezes with
PF_FREEZER_SKIP set, refrigerator() clears this flag for the current task before
calling frozen_process(current) to avoid having both PF_FREEZER_SKIP and
PF_FROZEN set at the same time.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
include/linux/freezer.h | 30 ++++++++++++++++++++++++++++--
include/linux/sched.h | 1 +
kernel/fork.c | 3 +++
kernel/power/process.c | 28 +++++++++-------------------
4 files changed, 41 insertions(+), 21 deletions(-)
Index: linux-2.6.20-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
#define PF_SPREAD_SLAB 0x02000000 /* Spread some slab caches over cpuset */
#define PF_MEMPOLICY 0x10000000 /* Non-default NUMA mempolicy */
#define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */
+#define PF_FREEZER_SKIP 0x40000000 /* Freezer should not count it as freezeable */
/*
* Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -75,7 +75,31 @@ static inline int try_to_freeze(void)
return 0;
}
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+ current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+ try_to_freeze();
+ current->flags &= ~PF_FREEZER_SKIP;
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+ return !!(p->flags & PF_FREEZER_SKIP);
+}
#else
static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +114,7 @@ static inline void thaw_processes(void)
static inline int try_to_freeze(void) { return 0; }
-
+static inline void freezer_do_not_count(void) {}
+static inline void freezer_count(void) {}
+static inline int freezer_should_skip(struct task_struct *p) { return 0; }
#endif
Index: linux-2.6.20-mm2/kernel/fork.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/fork.c
+++ linux-2.6.20-mm2/kernel/fork.c
@@ -50,6 +50,7 @@
#include <linux/taskstats_kern.h>
#include <linux/random.h>
#include <linux/ptrace.h>
+#include <linux/freezer.h>
#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
tracehook_report_clone_complete(clone_flags, nr, p);
if (clone_flags & CLONE_VFORK) {
+ freezer_do_not_count();
wait_for_completion(&vfork);
+ freezer_count();
tracehook_report_vfork_done(p, nr);
}
} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -42,6 +42,7 @@ void refrigerator(void)
task_lock(current);
if (freezing(current)) {
+ current->flags &= ~PF_FREEZER_SKIP;
frozen_process(current);
task_unlock(current);
} else {
@@ -127,22 +128,12 @@ static unsigned int try_to_freeze_tasks(
cancel_freezing(p);
continue;
}
- if (is_user_space(p)) {
- if (!freeze_user_space)
- continue;
-
- /* Freeze the task unless there is a vfork
- * completion pending
- */
- if (!p->vfork_done)
- freeze_process(p);
- } else {
- if (freeze_user_space)
- continue;
+ if (is_user_space(p) == !freeze_user_space)
+ continue;
- freeze_process(p);
- }
- todo++;
+ freeze_process(p);
+ if (!freezer_should_skip(p))
+ todo++;
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
yield(); /* Yield is okay here */
@@ -168,7 +159,8 @@ static unsigned int try_to_freeze_tasks(
continue;
task_lock(p);
- if (freezeable(p) && !frozen(p))
+ if (freezeable(p) && !frozen(p) &&
+ !freezer_should_skip(p))
printk(KERN_ERR " %s\n", p->comm);
cancel_freezing(p);
@@ -217,9 +209,7 @@ static void thaw_tasks(int thaw_user_spa
if (is_user_space(p) == !thaw_user_space)
continue;
- if (!thaw_process(p))
- printk(KERN_WARNING " Strange, %s not stopped\n",
- p->comm );
+ thaw_process(p);
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
}
From: Paul E. McKenney <[email protected]>
Remove PF_NOFREEZE from the rcutorture thread, adding a try_to_freeze() call as
required.
Signed-off-by: Paul E. McKenney <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
kernel/rcutorture.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6.20-mm2/kernel/rcutorture.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/rcutorture.c 2007-02-22 23:51:54.000000000 +0100
+++ linux-2.6.20-mm2/kernel/rcutorture.c 2007-02-22 23:55:12.000000000 +0100
@@ -46,6 +46,7 @@
#include <linux/byteorder/swabb.h>
#include <linux/stat.h>
#include <linux/srcu.h>
+#include <linux/freezer.h>
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Paul E. McKenney <[email protected]> and "
@@ -585,7 +586,6 @@ rcu_torture_writer(void *arg)
VERBOSE_PRINTK_STRING("rcu_torture_writer task started");
set_user_nice(current, 19);
- current->flags |= PF_NOFREEZE;
do {
schedule_timeout_uninterruptible(1);
@@ -607,6 +607,7 @@ rcu_torture_writer(void *arg)
}
rcu_torture_current_version++;
oldbatch = cur_ops->completed();
+ try_to_freeze();
} while (!kthread_should_stop() && !fullstop);
VERBOSE_PRINTK_STRING("rcu_torture_writer task stopping");
while (!kthread_should_stop())
From: Rafael J. Wysocki <[email protected]>
Remove PF_NOFREEZE from the bluetooth threads, adding try_to_freeze() calls as
required.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
net/bluetooth/bnep/core.c | 6 ++++--
net/bluetooth/cmtp/core.c | 4 +++-
net/bluetooth/hidp/core.c | 4 +++-
net/bluetooth/rfcomm/core.c | 4 +++-
4 files changed, 13 insertions(+), 5 deletions(-)
Index: linux-2.6.20-mm2/net/bluetooth/bnep/core.c
===================================================================
--- linux-2.6.20-mm2.orig/net/bluetooth/bnep/core.c 2007-02-23 00:26:58.000000000 +0100
+++ linux-2.6.20-mm2/net/bluetooth/bnep/core.c 2007-02-23 00:30:47.000000000 +0100
@@ -39,6 +39,7 @@
#include <linux/errno.h>
#include <linux/smp_lock.h>
#include <linux/net.h>
+#include <linux/freezer.h>
#include <net/sock.h>
#include <linux/socket.h>
@@ -473,11 +474,12 @@ static int bnep_session(void *arg)
daemonize("kbnepd %s", dev->name);
set_user_nice(current, -15);
- current->flags |= PF_NOFREEZE;
init_waitqueue_entry(&wait, current);
add_wait_queue(sk->sk_sleep, &wait);
while (!atomic_read(&s->killed)) {
+ try_to_freeze();
+
set_current_state(TASK_INTERRUPTIBLE);
// RX
Index: linux-2.6.20-mm2/net/bluetooth/cmtp/core.c
===================================================================
--- linux-2.6.20-mm2.orig/net/bluetooth/cmtp/core.c 2007-02-23 00:26:58.000000000 +0100
+++ linux-2.6.20-mm2/net/bluetooth/cmtp/core.c 2007-02-23 00:31:01.000000000 +0100
@@ -34,6 +34,7 @@
#include <linux/ioctl.h>
#include <linux/file.h>
#include <linux/init.h>
+#include <linux/freezer.h>
#include <net/sock.h>
#include <linux/isdn/capilli.h>
@@ -287,11 +288,12 @@ static int cmtp_session(void *arg)
daemonize("kcmtpd_ctr_%d", session->num);
set_user_nice(current, -15);
- current->flags |= PF_NOFREEZE;
init_waitqueue_entry(&wait, current);
add_wait_queue(sk->sk_sleep, &wait);
while (!atomic_read(&session->terminate)) {
+ try_to_freeze();
+
set_current_state(TASK_INTERRUPTIBLE);
if (sk->sk_state != BT_CONNECTED)
Index: linux-2.6.20-mm2/net/bluetooth/hidp/core.c
===================================================================
--- linux-2.6.20-mm2.orig/net/bluetooth/hidp/core.c 2007-02-23 00:26:58.000000000 +0100
+++ linux-2.6.20-mm2/net/bluetooth/hidp/core.c 2007-02-23 00:31:17.000000000 +0100
@@ -35,6 +35,7 @@
#include <linux/file.h>
#include <linux/init.h>
#include <linux/wait.h>
+#include <linux/freezer.h>
#include <net/sock.h>
#include <linux/input.h>
@@ -547,13 +548,14 @@ static int hidp_session(void *arg)
daemonize("khidpd_%04x%04x", vendor, product);
set_user_nice(current, -15);
- current->flags |= PF_NOFREEZE;
init_waitqueue_entry(&ctrl_wait, current);
init_waitqueue_entry(&intr_wait, current);
add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
add_wait_queue(intr_sk->sk_sleep, &intr_wait);
while (!atomic_read(&session->terminate)) {
+ try_to_freeze();
+
set_current_state(TASK_INTERRUPTIBLE);
if (ctrl_sk->sk_state != BT_CONNECTED || intr_sk->sk_state != BT_CONNECTED)
Index: linux-2.6.20-mm2/net/bluetooth/rfcomm/core.c
===================================================================
--- linux-2.6.20-mm2.orig/net/bluetooth/rfcomm/core.c 2007-02-23 00:26:58.000000000 +0100
+++ linux-2.6.20-mm2/net/bluetooth/rfcomm/core.c 2007-02-23 00:31:43.000000000 +0100
@@ -37,6 +37,7 @@
#include <linux/device.h>
#include <linux/net.h>
#include <linux/mutex.h>
+#include <linux/freezer.h>
#include <net/sock.h>
#include <asm/uaccess.h>
@@ -1851,6 +1852,8 @@ static void rfcomm_worker(void)
BT_DBG("");
while (!atomic_read(&terminate)) {
+ try_to_freeze();
+
if (!test_bit(RFCOMM_SCHED_WAKEUP, &rfcomm_event)) {
/* No pending events. Let's sleep.
* Incoming connections and data will wake us up. */
@@ -1937,7 +1940,6 @@ static int rfcomm_run(void *unused)
daemonize("krfcommd");
set_user_nice(current, -10);
- current->flags |= PF_NOFREEZE;
BT_DBG("");
From: Rafael J. Wysocki <[email protected]>
The reading of PF_BORROWED_MM in is_user_space() without task_lock() is racy.
Fix it.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
kernel/power/process.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -8,6 +8,7 @@
#undef DEBUG
+#include <linux/sched.h>
#include <linux/smp_lock.h>
#include <linux/interrupt.h>
#include <linux/suspend.h>
@@ -92,7 +93,12 @@ static void cancel_freezing(struct task_
static inline int is_user_space(struct task_struct *p)
{
- return p->mm && !(p->flags & PF_BORROWED_MM);
+ int ret;
+
+ task_lock(p);
+ ret = p->mm && !(p->flags & PF_BORROWED_MM);
+ task_unlock(p);
+ return ret;
}
static unsigned int try_to_freeze_tasks(int freeze_user_space)
From: Rafael J. Wysocki <[email protected]>
Add try_to_freeze() calls to the remaining kernel threads that do not call
try_to_freeze() already, although they set PF_NOFREEZE.
In the future we are going to replace PF_NOFREEZE with a set of flags that will
be set to indicate in which situations the task should not be frozen (for
example, there can be a task that should be frozen for the CPU hotplugging and
should not be frozen for the system suspend). For this reason every kernel
thread should be able to freeze itself (ie. call try_to_freeze()), so that it
can be frozen whenever necessary.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
arch/i386/kernel/apm.c | 2 ++
drivers/block/loop.c | 2 ++
drivers/char/apm-emulation.c | 3 +++
drivers/ieee1394/ieee1394_core.c | 3 +++
drivers/md/md.c | 2 ++
drivers/mmc/card/queue.c | 3 +++
drivers/mtd/mtd_blkdevs.c | 3 +++
drivers/scsi/libsas/sas_scsi_host.c | 3 +++
drivers/scsi/scsi_error.c | 3 +++
drivers/usb/storage/usb.c | 2 ++
kernel/softirq.c | 2 ++
kernel/softlockup.c | 2 ++
kernel/workqueue.c | 3 +--
13 files changed, 31 insertions(+), 2 deletions(-)
Index: linux-2.6.20-mm2/arch/i386/kernel/apm.c
===================================================================
--- linux-2.6.20-mm2.orig/arch/i386/kernel/apm.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/arch/i386/kernel/apm.c 2007-02-23 00:34:25.000000000 +0100
@@ -227,6 +227,7 @@
#include <linux/dmi.h>
#include <linux/suspend.h>
#include <linux/kthread.h>
+#include <linux/freezer.h>
#include <asm/system.h>
#include <asm/uaccess.h>
@@ -1402,6 +1403,7 @@ static void apm_mainloop(void)
add_wait_queue(&apm_waitqueue, &wait);
set_current_state(TASK_INTERRUPTIBLE);
for (;;) {
+ try_to_freeze();
schedule_timeout(APM_CHECK_TIMEOUT);
if (kthread_should_stop())
break;
Index: linux-2.6.20-mm2/drivers/md/md.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/md/md.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/md/md.c 2007-02-23 00:34:25.000000000 +0100
@@ -4513,6 +4513,8 @@ static int md_thread(void * arg)
|| kthread_should_stop(),
thread->timeout);
+ try_to_freeze();
+
clear_bit(THREAD_WAKEUP, &thread->flags);
thread->run(thread->mddev);
Index: linux-2.6.20-mm2/drivers/mmc/card/queue.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/mmc/card/queue.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/mmc/card/queue.c 2007-02-23 00:34:25.000000000 +0100
@@ -12,6 +12,7 @@
#include <linux/module.h>
#include <linux/blkdev.h>
#include <linux/kthread.h>
+#include <linux/freezer.h>
#include <linux/mmc/card.h>
#include <linux/mmc/host.h>
@@ -71,6 +72,8 @@ static int mmc_queue_thread(void *d)
do {
struct request *req = NULL;
+ try_to_freeze();
+
spin_lock_irq(q->queue_lock);
set_current_state(TASK_INTERRUPTIBLE);
if (!blk_queue_plugged(q))
Index: linux-2.6.20-mm2/drivers/mtd/mtd_blkdevs.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/mtd/mtd_blkdevs.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/mtd/mtd_blkdevs.c 2007-02-23 00:34:25.000000000 +0100
@@ -20,6 +20,7 @@
#include <linux/hdreg.h>
#include <linux/init.h>
#include <linux/mutex.h>
+#include <linux/freezer.h>
#include <asm/uaccess.h>
static LIST_HEAD(blktrans_majors);
@@ -113,6 +114,8 @@ static int mtd_blktrans_thread(void *arg
schedule();
remove_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
+ try_to_freeze();
+
spin_lock_irq(rq->queue_lock);
continue;
Index: linux-2.6.20-mm2/drivers/usb/storage/usb.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/usb/storage/usb.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/usb/storage/usb.c 2007-02-23 00:34:25.000000000 +0100
@@ -304,6 +304,8 @@ static int usb_stor_control_thread(void
current->flags |= PF_NOFREEZE;
for(;;) {
+ try_to_freeze();
+
US_DEBUGP("*** thread sleeping.\n");
if(down_interruptible(&us->sema))
break;
Index: linux-2.6.20-mm2/drivers/ieee1394/ieee1394_core.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/ieee1394/ieee1394_core.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/ieee1394/ieee1394_core.c 2007-02-23 00:34:25.000000000 +0100
@@ -35,6 +35,7 @@
#include <linux/kthread.h>
#include <linux/preempt.h>
#include <linux/time.h>
+#include <linux/freezer.h>
#include <asm/system.h>
#include <asm/byteorder.h>
@@ -1081,6 +1082,8 @@ static int hpsbpkt_thread(void *__hi)
complete_routine(complete_data);
}
+ try_to_freeze();
+
set_current_state(TASK_INTERRUPTIBLE);
if (!skb_peek(&hpsbpkt_queue))
schedule();
Index: linux-2.6.20-mm2/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/char/apm-emulation.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/char/apm-emulation.c 2007-02-23 00:34:25.000000000 +0100
@@ -27,6 +27,7 @@
#include <linux/completion.h>
#include <linux/kthread.h>
#include <linux/delay.h>
+#include <linux/freezer.h>
#include <asm/system.h>
@@ -539,6 +540,8 @@ static int kapmd(void *arg)
apm_event_t event;
int ret;
+ try_to_freeze();
+
wait_event_interruptible(kapmd_wait,
!queue_empty(&kapmd_queue) || kthread_should_stop());
Index: linux-2.6.20-mm2/drivers/block/loop.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/block/loop.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/block/loop.c 2007-02-23 00:34:25.000000000 +0100
@@ -74,6 +74,7 @@
#include <linux/highmem.h>
#include <linux/gfp.h>
#include <linux/kthread.h>
+#include <linux/freezer.h>
#include <asm/uaccess.h>
@@ -580,6 +581,7 @@ static int loop_thread(void *data)
set_user_nice(current, -20);
while (!kthread_should_stop() || lo->lo_bio) {
+ try_to_freeze();
wait_event_interruptible(lo->lo_event,
lo->lo_bio || kthread_should_stop());
Index: linux-2.6.20-mm2/drivers/scsi/libsas/sas_scsi_host.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/scsi/libsas/sas_scsi_host.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/scsi/libsas/sas_scsi_host.c 2007-02-23 00:34:25.000000000 +0100
@@ -39,6 +39,7 @@
#include <linux/err.h>
#include <linux/blkdev.h>
#include <linux/scatterlist.h>
+#include <linux/freezer.h>
/* ---------- SCSI Host glue ---------- */
@@ -875,6 +876,8 @@ static int sas_queue_thread(void *_sas_h
complete(&queue_th_comp);
while (1) {
+ try_to_freeze();
+
down_interruptible(&core->queue_thread_sema);
sas_queue(sas_ha);
if (core->queue_thread_kill)
Index: linux-2.6.20-mm2/drivers/scsi/scsi_error.c
===================================================================
--- linux-2.6.20-mm2.orig/drivers/scsi/scsi_error.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/drivers/scsi/scsi_error.c 2007-02-23 00:34:25.000000000 +0100
@@ -24,6 +24,7 @@
#include <linux/interrupt.h>
#include <linux/blkdev.h>
#include <linux/delay.h>
+#include <linux/freezer.h>
#include <scsi/scsi.h>
#include <scsi/scsi_cmnd.h>
@@ -1536,6 +1537,8 @@ int scsi_error_handler(void *data)
*/
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
+ try_to_freeze();
+
if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
shost->host_failed != shost->host_busy) {
SCSI_LOG_ERROR_RECOVERY(1,
Index: linux-2.6.20-mm2/kernel/softlockup.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/softlockup.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/kernel/softlockup.c 2007-02-23 00:34:25.000000000 +0100
@@ -13,6 +13,7 @@
#include <linux/kthread.h>
#include <linux/notifier.h>
#include <linux/module.h>
+#include <linux/freezer.h>
static DEFINE_SPINLOCK(print_lock);
@@ -93,6 +94,7 @@ static int watchdog(void * __bind_cpu)
* debug-printout triggers in softlockup_tick().
*/
while (!kthread_should_stop()) {
+ try_to_freeze();
set_current_state(TASK_INTERRUPTIBLE);
touch_softlockup_watchdog();
schedule();
Index: linux-2.6.20-mm2/kernel/softirq.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/softirq.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/kernel/softirq.c 2007-02-23 00:34:25.000000000 +0100
@@ -18,6 +18,7 @@
#include <linux/rcupdate.h>
#include <linux/smp.h>
#include <linux/tick.h>
+#include <linux/freezer.h>
#include <asm/irq.h>
/*
@@ -494,6 +495,7 @@ static int ksoftirqd(void * __bind_cpu)
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
+ try_to_freeze();
preempt_disable();
if (!local_softirq_pending()) {
preempt_enable_no_resched();
Index: linux-2.6.20-mm2/kernel/workqueue.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/workqueue.c 2007-02-22 23:48:52.000000000 +0100
+++ linux-2.6.20-mm2/kernel/workqueue.c 2007-02-23 00:34:25.000000000 +0100
@@ -316,8 +316,7 @@ static int worker_thread(void *__cwq)
do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
for (;;) {
- if (cwq->wq->freezeable)
- try_to_freeze();
+ try_to_freeze();
prepare_to_wait(&cwq->more_work, &wait, TASK_INTERRUPTIBLE);
if (!cwq->should_stop && list_empty(&cwq->worklist))
On Fri 2007-02-23 11:18:06, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> The reading of PF_BORROWED_MM in is_user_space() without task_lock() is racy.
> Fix it.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
ACK.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> If the freezing of tasks fails and a task is preempted in refrigerator() before
> calling frozen_process(), then thaw_tasks() may run before this task is frozen.
> In that case the task will freeze and no one will thaw it.
>
> To fix this race we can call freezing(current) in refrigerator() along with
> frozen_process(current) under the task_lock() which also should be taken in
> the error path of try_to_freeze_tasks() as well as in thaw_process(). Moreover,
> if thaw_process() additionally clears TIF_FREEZE for tasks that are not frozen,
> we can be sure that all tasks are thawed and there are no pending "freeze"
> requests after thaw_tasks() has run.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Looks ok to me.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri 2007-02-23 11:23:43, Rafael J. Wysocki wrote:
> From: Paul E. McKenney <[email protected]>
>
> Remove PF_NOFREEZE from the rcutorture thread, adding a try_to_freeze() call as
> required.
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
ACK.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri 2007-02-23 11:25:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Remove PF_NOFREEZE from the bluetooth threads, adding try_to_freeze() calls as
> required.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
ACK.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> Add try_to_freeze() calls to the remaining kernel threads that do not call
> try_to_freeze() already, although they set PF_NOFREEZE.
>
> In the future we are going to replace PF_NOFREEZE with a set of flags that will
> be set to indicate in which situations the task should not be frozen (for
> example, there can be a task that should be frozen for the CPU hotplugging and
> should not be frozen for the system suspend). For this reason every kernel
> thread should be able to freeze itself (ie. call try_to_freeze()), so that it
> can be frozen whenever necessary.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Looks ok to me. ACK.
> @@ -113,6 +114,8 @@ static int mtd_blktrans_thread(void *arg
> schedule();
> remove_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
>
> + try_to_freeze();
> +
> spin_lock_irq(rq->queue_lock);
>
> continue;
I'd prefer to move try_to_freeze() just after schedule().
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> Currently try_to_freeze_tasks() has to wait until all of the vforked processes
> exit and for this reason every user can make it fail. To fix this problem
> we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
> that do not want to be counted as freezable by the freezer and want to have
> TIF_FREEZE set nevertheless. Then, this flag can be set by tasks using
> sys_vfork() before they call wait_for_completion() and cleared after they have
> woken up and called try_to_freeze(). In case such a task freezes with
> PF_FREEZER_SKIP set, refrigerator() clears this flag for the current task before
> calling frozen_process(current) to avoid having both PF_FREEZER_SKIP and
> PF_FROZEN set at the same time.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> @@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
> tracehook_report_clone_complete(clone_flags, nr, p);
>
> if (clone_flags & CLONE_VFORK) {
> + freezer_do_not_count();
> wait_for_completion(&vfork);
> + freezer_count();
> tracehook_report_vfork_done(p, nr);
> }
> } else {
All the infrastructure for this...Would it be easier to introduce
void fastcall __sched wait_for_completion_freezeable(struct completion *x)
{
might_sleep();
spin_lock_irq(&x->wait.lock);
if (!x->done) {
DECLARE_WAITQUEUE(wait, current);
wait.flags |= WQ_FLAG_EXCLUSIVE;
__add_wait_queue_tail(&x->wait, &wait);
do {
__set_current_state(TASK_UNINTERRUPTIBLE);
spin_unlock_irq(&x->wait.lock);
schedule();
try_to_freeze(); /* HERE */
spin_lock_irq(&x->wait.lock);
} while (!x->done);
__remove_wait_queue(&x->wait, &wait);
}
x->done--;
spin_unlock_irq(&x->wait.lock);
}
...and be done with that, in a very obvious way? (Ok, you probably do
not want to duplicate the function, but you get the idea).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi,
On Sunday, 25 February 2007 11:46, Pavel Machek wrote:
> Hi!
>
> > Currently try_to_freeze_tasks() has to wait until all of the vforked processes
> > exit and for this reason every user can make it fail. To fix this problem
> > we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
> > that do not want to be counted as freezable by the freezer and want to have
> > TIF_FREEZE set nevertheless. Then, this flag can be set by tasks using
> > sys_vfork() before they call wait_for_completion() and cleared after they have
> > woken up and called try_to_freeze(). In case such a task freezes with
> > PF_FREEZER_SKIP set, refrigerator() clears this flag for the current task before
> > calling frozen_process(current) to avoid having both PF_FREEZER_SKIP and
> > PF_FROZEN set at the same time.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> > @@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
> > tracehook_report_clone_complete(clone_flags, nr, p);
> >
> > if (clone_flags & CLONE_VFORK) {
> > + freezer_do_not_count();
> > wait_for_completion(&vfork);
> > + freezer_count();
> > tracehook_report_vfork_done(p, nr);
> > }
> > } else {
>
> All the infrastructure for this...Would it be easier to introduce
>
> void fastcall __sched wait_for_completion_freezeable(struct completion *x)
> {
> might_sleep();
>
> spin_lock_irq(&x->wait.lock);
> if (!x->done) {
> DECLARE_WAITQUEUE(wait, current);
>
> wait.flags |= WQ_FLAG_EXCLUSIVE;
> __add_wait_queue_tail(&x->wait, &wait);
> do {
> __set_current_state(TASK_UNINTERRUPTIBLE);
> spin_unlock_irq(&x->wait.lock);
> schedule();
> try_to_freeze(); /* HERE */
> spin_lock_irq(&x->wait.lock);
> } while (!x->done);
> __remove_wait_queue(&x->wait, &wait);
> }
> x->done--;
> spin_unlock_irq(&x->wait.lock);
> }
>
> ...and be done with that, in a very obvious way? (Ok, you probably do
> not want to duplicate the function, but you get the idea).
Yes, I though about that too, but I was thinking of sticking try_to_freeze()
in wait wait_for_completion() itself, which was obviously wrong.
Still, the above might work. I'll try to prepare a patch.
Greetings,
Rafael
On 2/25/07, Pavel Machek <[email protected]> wrote:
> Hi!
>
> > Currently try_to_freeze_tasks() has to wait until all of the vforked processes
> > exit and for this reason every user can make it fail. To fix this problem
> > we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
> > that do not want to be counted as freezable by the freezer and want to have
> > TIF_FREEZE set nevertheless. Then, this flag can be set by tasks using
> > sys_vfork() before they call wait_for_completion() and cleared after they have
> > woken up and called try_to_freeze(). In case such a task freezes with
> > PF_FREEZER_SKIP set, refrigerator() clears this flag for the current task before
> > calling frozen_process(current) to avoid having both PF_FREEZER_SKIP and
> > PF_FROZEN set at the same time.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> > @@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
> > tracehook_report_clone_complete(clone_flags, nr, p);
> >
> > if (clone_flags & CLONE_VFORK) {
> > + freezer_do_not_count();
> > wait_for_completion(&vfork);
> > + freezer_count();
> > tracehook_report_vfork_done(p, nr);
> > }
> > } else {
>
> All the infrastructure for this...Would it be easier to introduce
>
> void fastcall __sched wait_for_completion_freezeable(struct completion *x)
> {
> might_sleep();
>
> spin_lock_irq(&x->wait.lock);
> if (!x->done) {
> DECLARE_WAITQUEUE(wait, current);
>
> wait.flags |= WQ_FLAG_EXCLUSIVE;
> __add_wait_queue_tail(&x->wait, &wait);
> do {
> __set_current_state(TASK_UNINTERRUPTIBLE);
> spin_unlock_irq(&x->wait.lock);
> schedule();
> try_to_freeze(); /* HERE */
> spin_lock_irq(&x->wait.lock);
> } while (!x->done);
> __remove_wait_queue(&x->wait, &wait);
> }
> x->done--;
> spin_unlock_irq(&x->wait.lock);
> }
>
>
I don't see a call to freezer_do_not_count here. Rafel was talking
about a similar change but that would involve special cases in
refrigerator such as wakeup parent if the process which is going to
be frozen have vfork_done set.
if (->vfork_done) {
wakeup_parent()
}
how about
void fastcall __sched wait_for_completion_freezeable(struct completion *x)
{
freezer_do_not_count();
wait_for_completion(x);
freezer_count();
}
-aneesh
On Sunday, 25 February 2007 11:45, Rafael J. Wysocki wrote:
> Hi,
>
> On Sunday, 25 February 2007 11:46, Pavel Machek wrote:
> > Hi!
> >
> > > Currently try_to_freeze_tasks() has to wait until all of the vforked processes
> > > exit and for this reason every user can make it fail. To fix this problem
> > > we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
> > > that do not want to be counted as freezable by the freezer and want to have
> > > TIF_FREEZE set nevertheless. Then, this flag can be set by tasks using
> > > sys_vfork() before they call wait_for_completion() and cleared after they have
> > > woken up and called try_to_freeze(). In case such a task freezes with
> > > PF_FREEZER_SKIP set, refrigerator() clears this flag for the current task before
> > > calling frozen_process(current) to avoid having both PF_FREEZER_SKIP and
> > > PF_FROZEN set at the same time.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> >
> > > @@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
> > > tracehook_report_clone_complete(clone_flags, nr, p);
> > >
> > > if (clone_flags & CLONE_VFORK) {
> > > + freezer_do_not_count();
> > > wait_for_completion(&vfork);
> > > + freezer_count();
> > > tracehook_report_vfork_done(p, nr);
> > > }
> > > } else {
> >
> > All the infrastructure for this...Would it be easier to introduce
> >
> > void fastcall __sched wait_for_completion_freezeable(struct completion *x)
> > {
> > might_sleep();
> >
> > spin_lock_irq(&x->wait.lock);
> > if (!x->done) {
> > DECLARE_WAITQUEUE(wait, current);
> >
> > wait.flags |= WQ_FLAG_EXCLUSIVE;
> > __add_wait_queue_tail(&x->wait, &wait);
> > do {
> > __set_current_state(TASK_UNINTERRUPTIBLE);
> > spin_unlock_irq(&x->wait.lock);
> > schedule();
> > try_to_freeze(); /* HERE */
> > spin_lock_irq(&x->wait.lock);
> > } while (!x->done);
> > __remove_wait_queue(&x->wait, &wait);
> > }
> > x->done--;
> > spin_unlock_irq(&x->wait.lock);
> > }
> >
> > ...and be done with that, in a very obvious way? (Ok, you probably do
> > not want to duplicate the function, but you get the idea).
>
> Yes, I though about that too, but I was thinking of sticking try_to_freeze()
> in wait wait_for_completion() itself, which was obviously wrong.
>
> Still, the above might work. I'll try to prepare a patch.
No, it won't work, because we have to tell the freezer not to count the
vfork parent as a freezable task. For this reason the additional process
flag is required and in that case try_to_freeze() has to be called after
we clear the flag.
Appended is the latest version of the patch (Oleg thinks it's correct).
Greetings,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Currently try_to_freeze_tasks() has to wait until all of the vforked processes
exit and for this reason every user can make it fail. To fix this problem
we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
that do not want to be counted as freezable by the freezer and want to have
TIF_FREEZE set nevertheless. Then, this flag can be set by tasks using
sys_vfork() before they call wait_for_completion() and cleared after they have
woken up. After clearing it, they have to call try_to_freeze().
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
include/linux/freezer.h | 30 ++++++++++++++++++++++++++++--
include/linux/sched.h | 1 +
kernel/fork.c | 3 +++
kernel/power/process.c | 27 ++++++++-------------------
4 files changed, 40 insertions(+), 21 deletions(-)
Index: linux-2.6.20-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/sched.h 2007-02-22 23:43:51.000000000 +0100
+++ linux-2.6.20-mm2/include/linux/sched.h 2007-02-22 23:44:04.000000000 +0100
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
#define PF_SPREAD_SLAB 0x02000000 /* Spread some slab caches over cpuset */
#define PF_MEMPOLICY 0x10000000 /* Non-default NUMA mempolicy */
#define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */
+#define PF_FREEZER_SKIP 0x40000000 /* Freezer should not count it as freezeable */
/*
* Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/freezer.h 2007-02-22 23:44:04.000000000 +0100
+++ linux-2.6.20-mm2/include/linux/freezer.h 2007-02-23 22:32:22.000000000 +0100
@@ -75,7 +75,31 @@ static inline int try_to_freeze(void)
return 0;
}
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+ current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+ current->flags &= ~PF_FREEZER_SKIP;
+ try_to_freeze();
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+ return !!(p->flags & PF_FREEZER_SKIP);
+}
#else
static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +114,7 @@ static inline void thaw_processes(void)
static inline int try_to_freeze(void) { return 0; }
-
+static inline void freezer_do_not_count(void) {}
+static inline void freezer_count(void) {}
+static inline int freezer_should_skip(struct task_struct *p) { return 0; }
#endif
Index: linux-2.6.20-mm2/kernel/fork.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/fork.c 2007-02-22 23:43:51.000000000 +0100
+++ linux-2.6.20-mm2/kernel/fork.c 2007-02-22 23:44:04.000000000 +0100
@@ -50,6 +50,7 @@
#include <linux/taskstats_kern.h>
#include <linux/random.h>
#include <linux/ptrace.h>
+#include <linux/freezer.h>
#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
tracehook_report_clone_complete(clone_flags, nr, p);
if (clone_flags & CLONE_VFORK) {
+ freezer_do_not_count();
wait_for_completion(&vfork);
+ freezer_count();
tracehook_report_vfork_done(p, nr);
}
} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c 2007-02-22 23:44:04.000000000 +0100
+++ linux-2.6.20-mm2/kernel/power/process.c 2007-02-23 22:33:11.000000000 +0100
@@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
cancel_freezing(p);
continue;
}
- if (is_user_space(p)) {
- if (!freeze_user_space)
- continue;
-
- /* Freeze the task unless there is a vfork
- * completion pending
- */
- if (!p->vfork_done)
- freeze_process(p);
- } else {
- if (freeze_user_space)
- continue;
+ if (is_user_space(p) == !freeze_user_space)
+ continue;
- freeze_process(p);
- }
- todo++;
+ freeze_process(p);
+ if (!freezer_should_skip(p))
+ todo++;
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
yield(); /* Yield is okay here */
@@ -168,7 +158,8 @@ static unsigned int try_to_freeze_tasks(
continue;
task_lock(p);
- if (freezeable(p) && !frozen(p))
+ if (freezeable(p) && !frozen(p) &&
+ !freezer_should_skip(p))
printk(KERN_ERR " %s\n", p->comm);
cancel_freezing(p);
@@ -217,9 +208,7 @@ static void thaw_tasks(int thaw_user_spa
if (is_user_space(p) == !thaw_user_space)
continue;
- if (!thaw_process(p))
- printk(KERN_WARNING " Strange, %s not stopped\n",
- p->comm );
+ thaw_process(p);
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
}
On Sunday, 25 February 2007 14:01, Aneesh Kumar wrote:
> On 2/25/07, Pavel Machek <[email protected]> wrote:
> > Hi!
> >
> > > Currently try_to_freeze_tasks() has to wait until all of the vforked processes
> > > exit and for this reason every user can make it fail. To fix this problem
> > > we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
> > > that do not want to be counted as freezable by the freezer and want to have
> > > TIF_FREEZE set nevertheless. Then, this flag can be set by tasks using
> > > sys_vfork() before they call wait_for_completion() and cleared after they have
> > > woken up and called try_to_freeze(). In case such a task freezes with
> > > PF_FREEZER_SKIP set, refrigerator() clears this flag for the current task before
> > > calling frozen_process(current) to avoid having both PF_FREEZER_SKIP and
> > > PF_FROZEN set at the same time.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> >
> > > @@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
> > > tracehook_report_clone_complete(clone_flags, nr, p);
> > >
> > > if (clone_flags & CLONE_VFORK) {
> > > + freezer_do_not_count();
> > > wait_for_completion(&vfork);
> > > + freezer_count();
> > > tracehook_report_vfork_done(p, nr);
> > > }
> > > } else {
> >
> > All the infrastructure for this...Would it be easier to introduce
> >
> > void fastcall __sched wait_for_completion_freezeable(struct completion *x)
> > {
> > might_sleep();
> >
> > spin_lock_irq(&x->wait.lock);
> > if (!x->done) {
> > DECLARE_WAITQUEUE(wait, current);
> >
> > wait.flags |= WQ_FLAG_EXCLUSIVE;
> > __add_wait_queue_tail(&x->wait, &wait);
> > do {
> > __set_current_state(TASK_UNINTERRUPTIBLE);
> > spin_unlock_irq(&x->wait.lock);
> > schedule();
> > try_to_freeze(); /* HERE */
> > spin_lock_irq(&x->wait.lock);
> > } while (!x->done);
> > __remove_wait_queue(&x->wait, &wait);
> > }
> > x->done--;
> > spin_unlock_irq(&x->wait.lock);
> > }
> >
> >
>
> I don't see a call to freezer_do_not_count here. Rafel was talking
> about a similar change but that would involve special cases in
> refrigerator such as wakeup parent if the process which is going to
> be frozen have vfork_done set.
>
> if (->vfork_done) {
> wakeup_parent()
> }
>
> how about
>
> void fastcall __sched wait_for_completion_freezeable(struct completion *x)
> {
> freezer_do_not_count();
> wait_for_completion(x);
> freezer_count();
> }
I think we can something like that on top of my original patch (or rather, the
patch that I've just sent in another message) if it's needed by anything else
than vfork.
Greetings,
Rafael
On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> On Sunday, 25 February 2007 11:45, Rafael J. Wysocki wrote:
> > Hi,
> >
> > =========
> --- linux-2.6.20-mm2.orig/kernel/power/process.c 2007-02-22 23:44:04.000000000 +0100
> +++ linux-2.6.20-mm2/kernel/power/process.c 2007-02-23 22:33:11.000000000 +0100
> @@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
> cancel_freezing(p);
> continue;
> }
> - if (is_user_space(p)) {
> - if (!freeze_user_space)
> - continue;
> -
> - /* Freeze the task unless there is a vfork
> - * completion pending
> - */
> - if (!p->vfork_done)
> - freeze_process(p);
> - } else {
> - if (freeze_user_space)
> - continue;
> + if (is_user_space(p) == !freeze_user_space)
> + continue;
>
How about ?
if ( ! (is_user_space(p) == freeze_user_space) )
continue;
BTW one of the concern that vatsa had was; is it ok to allow some of
the tasks to be left running ( the parent from vfork ) while
freezing. I guess we can solve this in a nice way.
in fork.c
if (clone_flags & CLONE_VFORK) {
p->vfork_done = &vfork;
p->flags |= PF_PARENT_WAKEUP_ON_FREEZE;
init_completion(&vfork);
}
and in freeze_process(struct task_struct *p)
if ( p->flags & PF_PARENT_WAKEUP_ON_FREEZE ) {
wake_up_parent();
}
now parent should be wating for these completion via
wait_for_completion_freezable(); // pavel's implementation.
-aneesh
On Sunday, 25 February 2007 15:33, Aneesh Kumar wrote:
> On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > On Sunday, 25 February 2007 11:45, Rafael J. Wysocki wrote:
> > > Hi,
> > >
> > > =========
> > --- linux-2.6.20-mm2.orig/kernel/power/process.c 2007-02-22 23:44:04.000000000 +0100
> > +++ linux-2.6.20-mm2/kernel/power/process.c 2007-02-23 22:33:11.000000000 +0100
> > @@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
> > cancel_freezing(p);
> > continue;
> > }
> > - if (is_user_space(p)) {
> > - if (!freeze_user_space)
> > - continue;
> > -
> > - /* Freeze the task unless there is a vfork
> > - * completion pending
> > - */
> > - if (!p->vfork_done)
> > - freeze_process(p);
> > - } else {
> > - if (freeze_user_space)
> > - continue;
> > + if (is_user_space(p) == !freeze_user_space)
> > + continue;
> >
>
> How about ?
> if ( ! (is_user_space(p) == freeze_user_space) )
> continue;
I think it would be safer to do
if ( is_user_space(p) != !!freeze_user_space)
continue;
which is equivalent to my previous version, but contains one '!' more. ;-)
Seriously, the one in the patch is consistent with the other occurrences of
it in the file and I'm going to change it anyway in a separate patch
(while freezing kernel threads we need to freeze userspace tasks too in case
one of the kernel threads called kernel_execve() in the meantime).
> BTW one of the concern that vatsa had was; is it ok to allow some of
> the tasks to be left running ( the parent from vfork ) while
> freezing. I guess we can solve this in a nice way.
>
> in fork.c
>
> if (clone_flags & CLONE_VFORK) {
> p->vfork_done = &vfork;
> p->flags |= PF_PARENT_WAKEUP_ON_FREEZE;
> init_completion(&vfork);
> }
>
>
> and in freeze_process(struct task_struct *p)
>
> if ( p->flags & PF_PARENT_WAKEUP_ON_FREEZE ) {
> wake_up_parent();
> }
>
> now parent should be wating for these completion via
>
> wait_for_completion_freezable(); // pavel's implementation.
Hm, I think this leaves us with an analogous problem: we need a method
to tell a vforking task that the child should set PF_PARENT_WAKEUP_ON_FREEZE.
In the approach with PF_FREEZER_SKIP we need a method to tell the
vforking task that it should skip try_to_freeze() in freezer_count(), and I
think there are some possible ways to do this. The patch doesn't implement
any of them, because this is a different issue that can be deal with later.
Greetings,
Rafael
On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> On Sunday, 25 February 2007 15:33, Aneesh Kumar wrote:
> > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > > On Sunday, 25 February 2007 11:45, Rafael J. Wysocki wrote:
> > > > Hi,
> > > >
> > > > =========
> > > --- linux-2.6.20-mm2.orig/kernel/power/process.c 2007-02-22 23:44:04.000000000 +0100
> > > +++ linux-2.6.20-mm2/kernel/power/process.c 2007-02-23 22:33:11.000000000 +0100
> > > @@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
> > > cancel_freezing(p);
> > > continue;
> > > }
> > > - if (is_user_space(p)) {
> > > - if (!freeze_user_space)
> > > - continue;
> > > -
> > > - /* Freeze the task unless there is a vfork
> > > - * completion pending
> > > - */
> > > - if (!p->vfork_done)
> > > - freeze_process(p);
> > > - } else {
> > > - if (freeze_user_space)
> > > - continue;
> > > + if (is_user_space(p) == !freeze_user_space)
> > > + continue;
> > >
> >
> > How about ?
> > if ( ! (is_user_space(p) == freeze_user_space) )
> > continue;
>
> I think it would be safer to do
>
> if ( is_user_space(p) != !!freeze_user_space)
> continue;
>
> which is equivalent to my previous version, but contains one '!' more. ;-)
>
> Seriously, the one in the patch is consistent with the other occurrences of
> it in the file and I'm going to change it anyway in a separate patch
> (while freezing kernel threads we need to freeze userspace tasks too in case
> one of the kernel threads called kernel_execve() in the meantime).
>
> > BTW one of the concern that vatsa had was; is it ok to allow some of
> > the tasks to be left running ( the parent from vfork ) while
> > freezing. I guess we can solve this in a nice way.
> >
> > in fork.c
> >
> > if (clone_flags & CLONE_VFORK) {
> > p->vfork_done = &vfork;
> > p->flags |= PF_PARENT_WAKEUP_ON_FREEZE;
> > init_completion(&vfork);
> > }
> >
> >
> > and in freeze_process(struct task_struct *p)
> >
> > if ( p->flags & PF_PARENT_WAKEUP_ON_FREEZE ) {
> > wake_up_parent();
> > }
> >
> > now parent should be wating for these completion via
> >
> > wait_for_completion_freezable(); // pavel's implementation.
>
> Hm, I think this leaves us with an analogous problem: we need a method
> to tell a vforking task that the child should set PF_PARENT_WAKEUP_ON_FREEZE.
>
> In the approach with PF_FREEZER_SKIP we need a method to tell the
> vforking task that it should skip try_to_freeze() in freezer_count(), and I
> think there are some possible ways to do this. The patch doesn't implement
> any of them, because this is a different issue that can be deal with later.
But approach i outlined above make sure both parent and child get
frozen during the freeze_process. where as with PF_FREEZER_SKIP the
child waits in the completion wait_queue in an uninterruptible state.
I am not sure whether it really make any difference from any of the
freezer users point of view. (suspend, hotplug, kprobes etc ).
-aneesh
On 2/25/07, Aneesh Kumar <[email protected]> wrote:
> On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > On Sunday, 25 February 2007 15:33, Aneesh Kumar wrote:
> > > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > > > On Sunday, 25 February 2007 11:45, Rafael J. Wysocki wrote:
> > > > > Hi,
> > > > >
> > > > > =========
> > > > --- linux-2.6.20-mm2.orig/kernel/power/process.c 2007-02-22 23:44:04.000000000 +0100
> > > > +++ linux-2.6.20-mm2/kernel/power/process.c 2007-02-23 22:33:11.000000000 +0100
> > > > @@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
> > > > cancel_freezing(p);
> > > > continue;
> > > > }
> > > > - if (is_user_space(p)) {
> > > > - if (!freeze_user_space)
> > > > - continue;
> > > > -
> > > > - /* Freeze the task unless there is a vfork
> > > > - * completion pending
> > > > - */
> > > > - if (!p->vfork_done)
> > > > - freeze_process(p);
> > > > - } else {
> > > > - if (freeze_user_space)
> > > > - continue;
> > > > + if (is_user_space(p) == !freeze_user_space)
> > > > + continue;
> > > >
> > >
> > > How about ?
> > > if ( ! (is_user_space(p) == freeze_user_space) )
> > > continue;
> >
> > I think it would be safer to do
> >
> > if ( is_user_space(p) != !!freeze_user_space)
> > continue;
> >
> > which is equivalent to my previous version, but contains one '!' more. ;-)
> >
> > Seriously, the one in the patch is consistent with the other occurrences of
> > it in the file and I'm going to change it anyway in a separate patch
> > (while freezing kernel threads we need to freeze userspace tasks too in case
> > one of the kernel threads called kernel_execve() in the meantime).
> >
> > > BTW one of the concern that vatsa had was; is it ok to allow some of
> > > the tasks to be left running ( the parent from vfork ) while
> > > freezing. I guess we can solve this in a nice way.
> > >
> > > in fork.c
> > >
> > > if (clone_flags & CLONE_VFORK) {
> > > p->vfork_done = &vfork;
> > > p->flags |= PF_PARENT_WAKEUP_ON_FREEZE;
> > > init_completion(&vfork);
> > > }
> > >
> > >
> > > and in freeze_process(struct task_struct *p)
> > >
> > > if ( p->flags & PF_PARENT_WAKEUP_ON_FREEZE ) {
> > > wake_up_parent();
> > > }
> > >
> > > now parent should be wating for these completion via
> > >
> > > wait_for_completion_freezable(); // pavel's implementation.
> >
> > Hm, I think this leaves us with an analogous problem: we need a method
> > to tell a vforking task that the child should set PF_PARENT_WAKEUP_ON_FREEZE.
> >
> > In the approach with PF_FREEZER_SKIP we need a method to tell the
> > vforking task that it should skip try_to_freeze() in freezer_count(), and I
> > think there are some possible ways to do this. The patch doesn't implement
> > any of them, because this is a different issue that can be deal with later.
>
>
> But approach i outlined above make sure both parent and child get
> frozen during the freeze_process. where as with PF_FREEZER_SKIP the
> child waits in the completion wait_queue in an uninterruptible state.
> I am not sure whether it really make any difference from any of the
> freezer users point of view. (suspend, hotplug, kprobes etc ).
>
>
Thinking about this i guess we have a problem with the above approach
i outlined. if we have one task that is waiting on the event and more
than one that can generate the event then the above logic would not
work. Also with cases other than vfork; logic of tracking the waiting
task gets complex. I guess what we have right now is better.
-aneesh
On Sunday, 25 February 2007 16:40, Aneesh Kumar wrote:
> On 2/25/07, Aneesh Kumar <[email protected]> wrote:
> > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > > On Sunday, 25 February 2007 15:33, Aneesh Kumar wrote:
> > > > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
[--snip--]
>
> Thinking about this i guess we have a problem with the above approach
> i outlined. if we have one task that is waiting on the event and more
> than one that can generate the event then the above logic would not
> work. Also with cases other than vfork; logic of tracking the waiting
> task gets complex. I guess what we have right now is better.
I assume by "righ now" you mean the latest version of my patch. ;-)
Still, having pondered the Pavel's suggestion for a while I think it's doable
without the addtitional process flag. Patch below.
Greetings,
Rafael
include/linux/completion.h | 13 ++++++++++++-
kernel/fork.c | 2 +-
kernel/power/process.c | 20 ++++++--------------
kernel/sched.c | 8 ++++++--
4 files changed, 25 insertions(+), 18 deletions(-)
Index: linux-2.6.20-mm2/include/linux/completion.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/completion.h 2007-02-25 14:02:54.000000000 +0100
+++ linux-2.6.20-mm2/include/linux/completion.h 2007-02-25 20:20:35.000000000 +0100
@@ -42,7 +42,18 @@ static inline void init_completion(struc
init_waitqueue_head(&x->wait);
}
-extern void FASTCALL(wait_for_completion(struct completion *));
+extern void FASTCALL(__wait_for_completion(struct completion *, int));
+
+static inline void wait_for_completion(struct completion *x)
+{
+ __wait_for_completion(x, 0);
+}
+
+static inline void wait_for_completion_freezable(struct completion *x)
+{
+ __wait_for_completion(x, 1);
+}
+
extern int FASTCALL(wait_for_completion_interruptible(struct completion *x));
extern unsigned long FASTCALL(wait_for_completion_timeout(struct completion *x,
unsigned long timeout));
Index: linux-2.6.20-mm2/kernel/sched.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/sched.c 2007-02-25 14:02:54.000000000 +0100
+++ linux-2.6.20-mm2/kernel/sched.c 2007-02-25 20:20:35.000000000 +0100
@@ -3803,7 +3803,8 @@ void fastcall complete_all(struct comple
}
EXPORT_SYMBOL(complete_all);
-void fastcall __sched wait_for_completion(struct completion *x)
+void fastcall __sched
+__wait_for_completion(struct completion *x, int freezable)
{
might_sleep();
@@ -3817,6 +3818,9 @@ void fastcall __sched wait_for_completio
__set_current_state(TASK_UNINTERRUPTIBLE);
spin_unlock_irq(&x->wait.lock);
schedule();
+ if (freezable)
+ try_to_freeze();
+
spin_lock_irq(&x->wait.lock);
} while (!x->done);
__remove_wait_queue(&x->wait, &wait);
@@ -3824,7 +3828,7 @@ void fastcall __sched wait_for_completio
x->done--;
spin_unlock_irq(&x->wait.lock);
}
-EXPORT_SYMBOL(wait_for_completion);
+EXPORT_SYMBOL(__wait_for_completion);
unsigned long fastcall __sched
wait_for_completion_timeout(struct completion *x, unsigned long timeout)
Index: linux-2.6.20-mm2/kernel/fork.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/fork.c 2007-02-25 20:17:25.000000000 +0100
+++ linux-2.6.20-mm2/kernel/fork.c 2007-02-25 20:20:35.000000000 +0100
@@ -1393,7 +1393,7 @@ long do_fork(unsigned long clone_flags,
tracehook_report_clone_complete(clone_flags, nr, p);
if (clone_flags & CLONE_VFORK) {
- wait_for_completion(&vfork);
+ wait_for_completion_freezable(&vfork);
tracehook_report_vfork_done(p, nr);
}
} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c 2007-02-25 20:17:25.000000000 +0100
+++ linux-2.6.20-mm2/kernel/power/process.c 2007-02-25 20:20:35.000000000 +0100
@@ -48,6 +48,9 @@ void refrigerator(void)
task_unlock(current);
return;
}
+ if (current->vfork_done)
+ wake_up_process(current->parent);
+
save = current->state;
pr_debug("%s entered refrigerator\n", current->comm);
@@ -127,21 +130,10 @@ static unsigned int try_to_freeze_tasks(
cancel_freezing(p);
continue;
}
- if (is_user_space(p)) {
- if (!freeze_user_space)
- continue;
-
- /* Freeze the task unless there is a vfork
- * completion pending
- */
- if (!p->vfork_done)
- freeze_process(p);
- } else {
- if (freeze_user_space)
- continue;
+ if (is_user_space(p) == !freeze_user_space)
+ continue;
- freeze_process(p);
- }
+ freeze_process(p);
todo++;
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
On 02/25, Rafael J. Wysocki wrote:
>
> On Sunday, 25 February 2007 16:40, Aneesh Kumar wrote:
> > On 2/25/07, Aneesh Kumar <[email protected]> wrote:
> > > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > > > On Sunday, 25 February 2007 15:33, Aneesh Kumar wrote:
> > > > > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> [--snip--]
> >
> > Thinking about this i guess we have a problem with the above approach
> > i outlined. if we have one task that is waiting on the event and more
> > than one that can generate the event then the above logic would not
> > work. Also with cases other than vfork; logic of tracking the waiting
> > task gets complex. I guess what we have right now is better.
>
> I assume by "righ now" you mean the latest version of my patch. ;-)
>
> Still, having pondered the Pavel's suggestion for a while I think it's doable
> without the addtitional process flag. Patch below.
Probably I missed something, (I didn't see this patch and I missed the
start of discussion), but I can't understand this patch.
> +__wait_for_completion(struct completion *x, int freezable)
> {
> might_sleep();
>
> @@ -3817,6 +3818,9 @@ void fastcall __sched wait_for_completio
> __set_current_state(TASK_UNINTERRUPTIBLE);
> spin_unlock_irq(&x->wait.lock);
> schedule();
> + if (freezable)
> + try_to_freeze();
> +
> spin_lock_irq(&x->wait.lock);
> } while (!x->done);
> __remove_wait_queue(&x->wait, &wait);
> @@ -3824,7 +3828,7 @@ void fastcall __sched wait_for_completio
> x->done--;
> spin_unlock_irq(&x->wait.lock);
> }
>
> ..........
>
> @@ -48,6 +48,9 @@ void refrigerator(void)
> task_unlock(current);
> return;
> }
> + if (current->vfork_done)
> + wake_up_process(current->parent);
> +
What if current->parent doesn't have TIF_FREEZE yet? ->parent will schedule()
again, child goes to refrigerator. Now, how can we freeze the ->parent?
Oleg.
On Sunday, 25 February 2007 21:31, Oleg Nesterov wrote:
> On 02/25, Rafael J. Wysocki wrote:
> >
> > On Sunday, 25 February 2007 16:40, Aneesh Kumar wrote:
> > > On 2/25/07, Aneesh Kumar <[email protected]> wrote:
> > > > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > > > > On Sunday, 25 February 2007 15:33, Aneesh Kumar wrote:
> > > > > > On 2/25/07, Rafael J. Wysocki <[email protected]> wrote:
> > [--snip--]
> > >
> > > Thinking about this i guess we have a problem with the above approach
> > > i outlined. if we have one task that is waiting on the event and more
> > > than one that can generate the event then the above logic would not
> > > work. Also with cases other than vfork; logic of tracking the waiting
> > > task gets complex. I guess what we have right now is better.
> >
> > I assume by "righ now" you mean the latest version of my patch. ;-)
> >
> > Still, having pondered the Pavel's suggestion for a while I think it's doable
> > without the addtitional process flag. Patch below.
>
> Probably I missed something, (I didn't see this patch and I missed the
> start of discussion), but I can't understand this patch.
Please see http://lkml.org/lkml/2007/2/25/53 .
> > +__wait_for_completion(struct completion *x, int freezable)
> > {
> > might_sleep();
> >
> > @@ -3817,6 +3818,9 @@ void fastcall __sched wait_for_completio
> > __set_current_state(TASK_UNINTERRUPTIBLE);
> > spin_unlock_irq(&x->wait.lock);
> > schedule();
> > + if (freezable)
> > + try_to_freeze();
> > +
> > spin_lock_irq(&x->wait.lock);
> > } while (!x->done);
> > __remove_wait_queue(&x->wait, &wait);
> > @@ -3824,7 +3828,7 @@ void fastcall __sched wait_for_completio
> > x->done--;
> > spin_unlock_irq(&x->wait.lock);
> > }
> >
> > ..........
> >
> > @@ -48,6 +48,9 @@ void refrigerator(void)
> > task_unlock(current);
> > return;
> > }
> > + if (current->vfork_done)
> > + wake_up_process(current->parent);
> > +
>
> What if current->parent doesn't have TIF_FREEZE yet? ->parent will schedule()
> again, child goes to refrigerator. Now, how can we freeze the ->parent?
Good point. I didn't think about it.
All in all, having tried some different approaches I think that the patch at
http://lkml.org/lkml/2007/2/25/80 is the right thing to do.
Pavel, do you agree?
Rafael
Hi Rafael,
> From: Rafael J. Wysocki <[email protected]>
>
> Remove PF_NOFREEZE from the bluetooth threads, adding try_to_freeze() calls as
> required.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> net/bluetooth/bnep/core.c | 6 ++++--
> net/bluetooth/cmtp/core.c | 4 +++-
> net/bluetooth/hidp/core.c | 4 +++-
> net/bluetooth/rfcomm/core.c | 4 +++-
> 4 files changed, 13 insertions(+), 5 deletions(-)
Signed-off-by: Marcel Holtmann <[email protected]>
for the Bluetooth part of this patch series.
Regards
Marcel