[PATCH] Workqueue freezer support.

[PATCH] Workqueue freezer support.

[Nigel Cunningham]

This patch implements freezer support for workqueues. The current
refrigerator implementation makes all workqueues NOFREEZE, regardless of
whether they need to be or not.

While this doesn't appear to have caused any problems with swsusp (ie
Pavel's version) to date, this is no guarantee for the future.
Furthermore, it seems better to me to treat kernel and userspace threads
consistently, and it also enables us to err on the side of caution by
default with new workqueues.

Queues can be made unfreezable via the new kthread_nonfreeze_run,
create_nofreeze_workqueue and create_nofreeze_singlethread_workqueue
calls, which take the same parameters as kthread_run, create_workqueue
and create_singlethread_workqueue respectively. Existing call syntax is
unchanged and the vast majority of current workqueue calls are therefore
unaffected.

As far as Suspend2 goes, I don't rate this as critical. May save your
hard disk partition one day, but that depends upon what workqueues get
implemented in the future, what out of tree ones do and whether I've
missed good rationale for having nofreeze on existing in tree instances.
If you must flame me, call me overly careful :>.

Regards,

Nigel

Signed-off by: Nigel Cunningham <nigel@suspend2.net>

 drivers/acpi/osl.c          |    2 +-
 drivers/block/ll_rw_blk.c   |    2 +-
 drivers/char/hvc_console.c  |    2 +-
 drivers/char/hvcs.c         |    2 +-
 drivers/input/serio/serio.c |    2 +-
 drivers/md/dm-crypt.c       |    2 +-
 drivers/scsi/hosts.c        |    2 +-
 drivers/usb/net/pegasus.c   |    2 +-
 include/linux/kthread.h     |   20 ++++++++++++++++++--
 include/linux/workqueue.h   |    9 ++++++---
 kernel/kmod.c               |    4 ++++
 kernel/kthread.c            |   23 ++++++++++++++++++++++-
 kernel/sched.c              |    4 ++--
 kernel/softirq.c            |    3 +--
 kernel/workqueue.c          |   21 ++++++++++++---------
 15 files changed, 73 insertions(+), 27 deletions(-)
diff -ruNp 400-workthreads.patch-old/drivers/acpi/osl.c 400-workthreads.patch-new/drivers/acpi/osl.c
--- 400-workthreads.patch-old/drivers/acpi/osl.c	2005-07-18 06:36:41.000000000 +1000
+++ 400-workthreads.patch-new/drivers/acpi/osl.c	2005-07-20 08:52:31.000000000 +1000
@@ -98,7 +98,7 @@ acpi_os_initialize1(void)
 		return AE_NULL_ENTRY;
 	}
 #endif
-	kacpid_wq = create_singlethread_workqueue("kacpid");
+	kacpid_wq = create_nofreeze_singlethread_workqueue("kacpid");
 	BUG_ON(!kacpid_wq);
 
 	return AE_OK;
diff -ruNp 400-workthreads.patch-old/drivers/block/ll_rw_blk.c 400-workthreads.patch-new/drivers/block/ll_rw_blk.c
--- 400-workthreads.patch-old/drivers/block/ll_rw_blk.c	2005-07-18 06:36:42.000000000 +1000
+++ 400-workthreads.patch-new/drivers/block/ll_rw_blk.c	2005-07-21 00:44:30.000000000 +1000
@@ -3215,7 +3215,7 @@ EXPORT_SYMBOL(kblockd_flush);
 
 int __init blk_dev_init(void)
 {
-	kblockd_workqueue = create_workqueue("kblockd");
+	kblockd_workqueue = create_nofreeze_workqueue("kblockd");
 	if (!kblockd_workqueue)
 		panic("Failed to create kblockd\n");
 
diff -ruNp 400-workthreads.patch-old/drivers/char/hvc_console.c 400-workthreads.patch-new/drivers/char/hvc_console.c
--- 400-workthreads.patch-old/drivers/char/hvc_console.c	2005-07-18 06:36:43.000000000 +1000
+++ 400-workthreads.patch-new/drivers/char/hvc_console.c	2005-07-20 08:52:31.000000000 +1000
@@ -844,7 +844,7 @@ int __init hvc_init(void)
 
 	/* Always start the kthread because there can be hotplug vty adapters
 	 * added later. */
-	hvc_task = kthread_run(khvcd, NULL, "khvcd");
+	hvc_task = kthread_nofreeze_run(khvcd, NULL, "khvcd");
 	if (IS_ERR(hvc_task)) {
 		panic("Couldn't create kthread for console.\n");
 		put_tty_driver(hvc_driver);
diff -ruNp 400-workthreads.patch-old/drivers/char/hvcs.c 400-workthreads.patch-new/drivers/char/hvcs.c
--- 400-workthreads.patch-old/drivers/char/hvcs.c	2005-07-18 06:36:43.000000000 +1000
+++ 400-workthreads.patch-new/drivers/char/hvcs.c	2005-07-20 08:52:31.000000000 +1000
@@ -1403,7 +1403,7 @@ static int __init hvcs_module_init(void)
 		return -ENOMEM;
 	}
 
-	hvcs_task = kthread_run(khvcsd, NULL, "khvcsd");
+	hvcs_task = kthread_nofreeze_run(khvcsd, NULL, "khvcsd");
 	if (IS_ERR(hvcs_task)) {
 		printk(KERN_ERR "HVCS: khvcsd creation failed.  Driver not loaded.\n");
 		kfree(hvcs_pi_buff);
diff -ruNp 400-workthreads.patch-old/drivers/input/serio/serio.c 400-workthreads.patch-new/drivers/input/serio/serio.c
--- 400-workthreads.patch-old/drivers/input/serio/serio.c	2005-07-21 04:00:03.000000000 +1000
+++ 400-workthreads.patch-new/drivers/input/serio/serio.c	2005-07-20 08:52:31.000000000 +1000
@@ -889,7 +889,7 @@ irqreturn_t serio_interrupt(struct serio
 
 static int __init serio_init(void)
 {
-	serio_task = kthread_run(serio_thread, NULL, "kseriod");
+	serio_task = kthread_nofreeze_run(serio_thread, NULL, "kseriod");
 	if (IS_ERR(serio_task)) {
 		printk(KERN_ERR "serio: Failed to start kseriod\n");
 		return PTR_ERR(serio_task);
diff -ruNp 400-workthreads.patch-old/drivers/md/dm-crypt.c 400-workthreads.patch-new/drivers/md/dm-crypt.c
--- 400-workthreads.patch-old/drivers/md/dm-crypt.c	2005-07-18 06:36:47.000000000 +1000
+++ 400-workthreads.patch-new/drivers/md/dm-crypt.c	2005-07-20 08:52:31.000000000 +1000
@@ -926,7 +926,7 @@ static int __init dm_crypt_init(void)
 	if (!_crypt_io_pool)
 		return -ENOMEM;
 
-	_kcryptd_workqueue = create_workqueue("kcryptd");
+	_kcryptd_workqueue = create_nofreeze_workqueue("kcryptd");
 	if (!_kcryptd_workqueue) {
 		r = -ENOMEM;
 		DMERR(PFX "couldn't create kcryptd");
diff -ruNp 400-workthreads.patch-old/drivers/scsi/hosts.c 400-workthreads.patch-new/drivers/scsi/hosts.c
--- 400-workthreads.patch-old/drivers/scsi/hosts.c	2005-07-18 06:36:54.000000000 +1000
+++ 400-workthreads.patch-new/drivers/scsi/hosts.c	2005-07-20 08:52:31.000000000 +1000
@@ -132,7 +132,7 @@ int scsi_add_host(struct Scsi_Host *shos
 	if (shost->transportt->create_work_queue) {
 		snprintf(shost->work_q_name, KOBJ_NAME_LEN, "scsi_wq_%d",
 			shost->host_no);
-		shost->work_q = create_singlethread_workqueue(
+		shost->work_q = create_nofreeze_singlethread_workqueue(
 					shost->work_q_name);
 		if (!shost->work_q)
 			goto out_free_shost_data;
diff -ruNp 400-workthreads.patch-old/drivers/usb/net/pegasus.c 400-workthreads.patch-new/drivers/usb/net/pegasus.c
--- 400-workthreads.patch-old/drivers/usb/net/pegasus.c	2005-07-18 06:36:58.000000000 +1000
+++ 400-workthreads.patch-new/drivers/usb/net/pegasus.c	2005-07-20 08:52:31.000000000 +1000
@@ -1412,7 +1412,7 @@ static struct usb_driver pegasus_driver 
 static int __init pegasus_init(void)
 {
 	pr_info("%s: %s, " DRIVER_DESC "\n", driver_name, DRIVER_VERSION);
-	pegasus_workqueue = create_singlethread_workqueue("pegasus");
+	pegasus_workqueue = create_nofreeze_singlethread_workqueue("pegasus");
 	if (!pegasus_workqueue)
 		return -ENOMEM;
 	return usb_register(&pegasus_driver);
diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h
--- 400-workthreads.patch-old/include/linux/kthread.h	2004-11-03 21:51:12.000000000 +1100
+++ 400-workthreads.patch-new/include/linux/kthread.h	2005-07-20 15:11:37.000000000 +1000
@@ -27,6 +27,14 @@ struct task_struct *kthread_create(int (
 				   void *data,
 				   const char namefmt[], ...);
 
+struct task_struct *_kthread_create(int (*threadfn)(void *data),
+				   void *data,
+				   unsigned long freezer_flags,
+				   const char namefmt[], ...);
+
+#define kthread_nofreeze_create(threadfn, data, namefmt, args...) \
+	_kthread_create(threadfn, data, PF_NOFREEZE, namefmt, ##args)
+
 /**
  * kthread_run: create and wake a thread.
  * @threadfn: the function to run until signal_pending(current).
@@ -35,15 +43,23 @@ struct task_struct *kthread_create(int (
  *
  * Description: Convenient wrapper for kthread_create() followed by
  * wake_up_process().  Returns the kthread, or ERR_PTR(-ENOMEM). */
-#define kthread_run(threadfn, data, namefmt, ...)			   \
+#define kthread_run(threadfn, data, namefmt, args...)			   \
 ({									   \
 	struct task_struct *__k						   \
-		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
+		= kthread_create(threadfn, data, namefmt, ##args);	   \
 	if (!IS_ERR(__k))						   \
 		wake_up_process(__k);					   \
 	__k;								   \
 })
 
+#define kthread_nofreeze_run(threadfn, data, namefmt, args...)		   \
+({									   \
+	struct task_struct *__k	= kthread_nofreeze_create(threadfn, data,  \
+			namefmt, ##args);				   \
+	if (!IS_ERR(__k))						   \
+		wake_up_process(__k);					   \
+	__k;								   \
+})
 /**
  * kthread_bind: bind a just-created kthread to a cpu.
  * @k: thread created by kthread_create().
diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
--- 400-workthreads.patch-old/include/linux/workqueue.h	2005-06-20 11:47:30.000000000 +1000
+++ 400-workthreads.patch-new/include/linux/workqueue.h	2005-07-20 16:21:55.000000000 +1000
@@ -51,9 +51,12 @@ struct work_struct {
 	} while (0)
 
 extern struct workqueue_struct *__create_workqueue(const char *name,
-						    int singlethread);
-#define create_workqueue(name) __create_workqueue((name), 0)
-#define create_singlethread_workqueue(name) __create_workqueue((name), 1)
+						    int singlethread,
+						    unsigned long freezer_flag);
+#define create_workqueue(name) __create_workqueue((name), 0, 0)
+#define create_nofreeze_workqueue(name) __create_workqueue((name), 0, PF_NOFREEZE)
+#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0)
+#define create_nofreeze_singlethread_workqueue(name) __create_workqueue((name), 1, PF_NOFREEZE)
 
 extern void destroy_workqueue(struct workqueue_struct *wq);
 
diff -ruNp 400-workthreads.patch-old/kernel/kthread.c 400-workthreads.patch-new/kernel/kthread.c
--- 400-workthreads.patch-old/kernel/kthread.c	2005-06-20 11:47:31.000000000 +1000
+++ 400-workthreads.patch-new/kernel/kthread.c	2005-07-21 04:00:19.000000000 +1000
@@ -25,6 +25,7 @@ struct kthread_create_info
 	/* Information passed to kthread() from keventd. */
 	int (*threadfn)(void *data);
 	void *data;
+	unsigned long freezer_flags;
 	struct completion started;
 
 	/* Result passed back to kthread_create() from keventd. */
@@ -86,6 +87,10 @@ static int kthread(void *_create)
 	/* By default we can run anywhere, unlike keventd. */
 	set_cpus_allowed(current, CPU_MASK_ALL);
 
+	/* Set our freezer flags */
+	current->flags &= ~(PF_SYNCTHREAD | PF_NOFREEZE);
+	current->flags |= (create->freezer_flags & PF_NOFREEZE);
+
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_INTERRUPTIBLE);
 	complete(&create->started);
@@ -119,8 +124,9 @@ static void keventd_create_kthread(void 
 	complete(&create->done);
 }
 
-struct task_struct *kthread_create(int (*threadfn)(void *data),
+struct task_struct *_kthread_create(int (*threadfn)(void *data),
 				   void *data,
+				   unsigned long freezer_flags,
 				   const char namefmt[],
 				   ...)
 {
@@ -129,6 +135,7 @@ struct task_struct *kthread_create(int (
 
 	create.threadfn = threadfn;
 	create.data = data;
+	create.freezer_flags = freezer_flags;
 	init_completion(&create.started);
 	init_completion(&create.done);
 
@@ -151,6 +158,20 @@ struct task_struct *kthread_create(int (
 
 	return create.result;
 }
+
+struct task_struct *kthread_create(int (*threadfn)(void *data),
+				   void *data,
+				   const char namefmt[], ...)
+{
+	char result[TASK_COMM_LEN];
+
+	va_list args;
+	va_start(args, namefmt);
+	vsnprintf(result, TASK_COMM_LEN, namefmt, args);
+	va_end(args);
+	return _kthread_create(threadfn, data, 0, result);
+}
+
 EXPORT_SYMBOL(kthread_create);
 
 void kthread_bind(struct task_struct *k, unsigned int cpu)
diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
--- 400-workthreads.patch-old/kernel/sched.c	2005-07-21 04:00:02.000000000 +1000
+++ 400-workthreads.patch-new/kernel/sched.c	2005-07-21 04:00:19.000000000 +1000
@@ -4580,10 +4580,10 @@ static int migration_call(struct notifie
 
 	switch (action) {
 	case CPU_UP_PREPARE:
-		p = kthread_create(migration_thread, hcpu, "migration/%d",cpu);
+		p = kthread_create(migration_thread, hcpu,
+				"migration/%d",cpu);
 		if (IS_ERR(p))
 			return NOTIFY_BAD;
-		p->flags |= PF_NOFREEZE;
 		kthread_bind(p, cpu);
 		/* Must be high prio: stop_machine expects to yield to it. */
 		rq = task_rq_lock(p, &flags);
diff -ruNp 400-workthreads.patch-old/kernel/softirq.c 400-workthreads.patch-new/kernel/softirq.c
--- 400-workthreads.patch-old/kernel/softirq.c	2005-06-20 11:47:32.000000000 +1000
+++ 400-workthreads.patch-new/kernel/softirq.c	2005-07-20 08:52:31.000000000 +1000
@@ -350,7 +350,6 @@ void __init softirq_init(void)
 static int ksoftirqd(void * __bind_cpu)
 {
 	set_user_nice(current, 19);
-	current->flags |= PF_NOFREEZE;
 
 	set_current_state(TASK_INTERRUPTIBLE);
 
@@ -456,7 +455,7 @@ static int __devinit cpu_callback(struct
 	case CPU_UP_PREPARE:
 		BUG_ON(per_cpu(tasklet_vec, hotcpu).list);
 		BUG_ON(per_cpu(tasklet_hi_vec, hotcpu).list);
-		p = kthread_create(ksoftirqd, hcpu, "ksoftirqd/%d", hotcpu);
+		p = kthread_nofreeze_create(ksoftirqd, hcpu, "ksoftirqd/%d", hotcpu);
 		if (IS_ERR(p)) {
 			printk("ksoftirqd for %i failed\n", hotcpu);
 			return NOTIFY_BAD;
diff -ruNp 400-workthreads.patch-old/kernel/workqueue.c 400-workthreads.patch-new/kernel/workqueue.c
--- 400-workthreads.patch-old/kernel/workqueue.c	2005-06-20 11:47:32.000000000 +1000
+++ 400-workthreads.patch-new/kernel/workqueue.c	2005-07-21 00:39:28.000000000 +1000
@@ -186,8 +186,6 @@ static int worker_thread(void *__cwq)
 	struct k_sigaction sa;
 	sigset_t blocked;
 
-	current->flags |= PF_NOFREEZE;
-
 	set_user_nice(current, -5);
 
 	/* Block and flush all signals */
@@ -208,6 +206,7 @@ static int worker_thread(void *__cwq)
 			schedule();
 		else
 			__set_current_state(TASK_RUNNING);
+		try_to_freeze();
 		remove_wait_queue(&cwq->more_work, &wait);
 
 		if (!list_empty(&cwq->worklist))
@@ -277,7 +276,8 @@ void fastcall flush_workqueue(struct wor
 }
 
 static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq,
-						   int cpu)
+						   int cpu,
+						   unsigned long freezer_flags)
 {
 	struct cpu_workqueue_struct *cwq = wq->cpu_wq + cpu;
 	struct task_struct *p;
@@ -292,9 +292,11 @@ static struct task_struct *create_workqu
 	init_waitqueue_head(&cwq->work_done);
 
 	if (is_single_threaded(wq))
-		p = kthread_create(worker_thread, cwq, "%s", wq->name);
+		p = _kthread_create(worker_thread, cwq, freezer_flags,
+				"%s", wq->name);
 	else
-		p = kthread_create(worker_thread, cwq, "%s/%d", wq->name, cpu);
+		p = _kthread_create(worker_thread, cwq, freezer_flags,
+				"%s/%d", wq->name, cpu);
 	if (IS_ERR(p))
 		return NULL;
 	cwq->thread = p;
@@ -302,7 +304,8 @@ static struct task_struct *create_workqu
 }
 
 struct workqueue_struct *__create_workqueue(const char *name,
-					    int singlethread)
+					    int singlethread,
+					    unsigned long freezer_flags)
 {
 	int cpu, destroy = 0;
 	struct workqueue_struct *wq;
@@ -320,7 +323,7 @@ struct workqueue_struct *__create_workqu
 	lock_cpu_hotplug();
 	if (singlethread) {
 		INIT_LIST_HEAD(&wq->list);
-		p = create_workqueue_thread(wq, 0);
+		p = create_workqueue_thread(wq, 0, freezer_flags);
 		if (!p)
 			destroy = 1;
 		else
@@ -330,7 +333,7 @@ struct workqueue_struct *__create_workqu
 		list_add(&wq->list, &workqueues);
 		spin_unlock(&workqueue_lock);
 		for_each_online_cpu(cpu) {
-			p = create_workqueue_thread(wq, cpu);
+			p = create_workqueue_thread(wq, cpu, freezer_flags);
 			if (p) {
 				kthread_bind(p, cpu);
 				wake_up_process(p);
@@ -540,7 +543,7 @@ static int __devinit workqueue_cpu_callb
 void init_workqueues(void)
 {
 	hotcpu_notifier(workqueue_cpu_callback, 0);
-	keventd_wq = create_workqueue("events");
+	keventd_wq = create_nofreeze_workqueue("events");
 	BUG_ON(!keventd_wq);
 }
 

-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Pavel Machek]

Hi!

> This patch implements freezer support for workqueues. The current
> refrigerator implementation makes all workqueues NOFREEZE, regardless of
> whether they need to be or not.
> 
> While this doesn't appear to have caused any problems with swsusp (ie
> Pavel's version) to date, this is no guarantee for the future.
> Furthermore, it seems better to me to treat kernel and userspace threads
> consistently, and it also enables us to err on the side of caution by
> default with new workqueues.
> 
> Queues can be made unfreezable via the new kthread_nonfreeze_run,
> create_nofreeze_workqueue and create_nofreeze_singlethread_workqueue
> calls, which take the same parameters as kthread_run, create_workqueue
> and create_singlethread_workqueue respectively. Existing call syntax is
> unchanged and the vast majority of current workqueue calls are therefore
> unaffected.
> 
> As far as Suspend2 goes, I don't rate this as critical. May save your
> hard disk partition one day, but that depends upon what workqueues get
> implemented in the future, what out of tree ones do and whether I've
> missed good rationale for having nofreeze on existing in tree instances.
> If you must flame me, call me overly careful :>.

> @@ -151,6 +158,20 @@ struct task_struct *kthread_create(int (
>  
>  	return create.result;
>  }
> +
> +struct task_struct *kthread_create(int (*threadfn)(void *data),
> +				   void *data,
> +				   const char namefmt[], ...)
> +{
> +	char result[TASK_COMM_LEN];
> +
> +	va_list args;
> +	va_start(args, namefmt);
> +	vsnprintf(result, TASK_COMM_LEN, namefmt, args);
> +	va_end(args);
> +	return _kthread_create(threadfn, data, 0, result);
> +}
> +

This is slightly ugly and uses lot of stack. Otherwise patch looks
okay. If you want me to apply it, be sure to put me into To: or at
least Cc:. Or perhaps you want to just mail it to akpm, noting that I
acked it (if you do something with the char result[] :-).

								Pavel
-- 
teflon -- maybe it is a trademark, but it should not be.

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Ingo Molnar]

* Pavel Machek <pavel@ucw.cz> wrote:

> > +struct task_struct *kthread_create(int (*threadfn)(void *data),
> > +				   void *data,
> > +				   const char namefmt[], ...)
> > +{
> > +	char result[TASK_COMM_LEN];
> > +
> > +	va_list args;
> > +	va_start(args, namefmt);
> > +	vsnprintf(result, TASK_COMM_LEN, namefmt, args);
> > +	va_end(args);
> > +	return _kthread_create(threadfn, data, 0, result);
> > +}
> > +
> 
> This is slightly ugly and uses lot of stack. Otherwise patch looks 
> okay. If you want me to apply it, be sure to put me into To: or at 
> least Cc:. Or perhaps you want to just mail it to akpm, noting that I 
> acked it (if you do something with the char result[] :-).

TASK_COMM_LEN is only 16 bytes so it's not that bad.

	Ingo

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Pavel Machek]

Hi!

> > > +struct task_struct *kthread_create(int (*threadfn)(void *data),
> > > +				   void *data,
> > > +				   const char namefmt[], ...)
> > > +{
> > > +	char result[TASK_COMM_LEN];
> > > +
> > > +	va_list args;
> > > +	va_start(args, namefmt);
> > > +	vsnprintf(result, TASK_COMM_LEN, namefmt, args);
> > > +	va_end(args);
> > > +	return _kthread_create(threadfn, data, 0, result);
> > > +}
> > > +
> > 
> > This is slightly ugly and uses lot of stack. Otherwise patch looks 
> > okay. If you want me to apply it, be sure to put me into To: or at 
> > least Cc:. Or perhaps you want to just mail it to akpm, noting that I 
> > acked it (if you do something with the char result[] :-).
> 
> TASK_COMM_LEN is only 16 bytes so it's not that bad.

Okay then ;-).
							Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Patrick Mochel]

On Thu, 21 Jul 2005, Nigel Cunningham wrote:

> This patch implements freezer support for workqueues. The current
> refrigerator implementation makes all workqueues NOFREEZE, regardless of
> whether they need to be or not.

A few comments..

> Signed-off by: Nigel Cunningham <nigel@suspend2.net>
>
>  drivers/acpi/osl.c          |    2 +-
>  drivers/block/ll_rw_blk.c   |    2 +-
>  drivers/char/hvc_console.c  |    2 +-
>  drivers/char/hvcs.c         |    2 +-
>  drivers/input/serio/serio.c |    2 +-
>  drivers/md/dm-crypt.c       |    2 +-
>  drivers/scsi/hosts.c        |    2 +-
>  drivers/usb/net/pegasus.c   |    2 +-

If you want some practice splitting things up, submit the patches above
individually to the maintainers o the relevant code once the patches you
submit below get merged to -mm.

>  include/linux/kthread.h     |   20 ++++++++++++++++++--
>  include/linux/workqueue.h   |    9 ++++++---
>  kernel/kmod.c               |    4 ++++
>  kernel/kthread.c            |   23 ++++++++++++++++++++++-
>  kernel/sched.c              |    4 ++--
>  kernel/softirq.c            |    3 +--
>  kernel/workqueue.c          |   21 ++++++++++++---------
>  15 files changed, 73 insertions(+), 27 deletions(-)


You should make sure that you get an explicit ACK from people (Ingo et al)
about whether this is an acceptable interface.

> --- 400-workthreads.patch-old/include/linux/kthread.h	2004-11-03 21:51:12.000000000 +1100
> +++ 400-workthreads.patch-new/include/linux/kthread.h	2005-07-20 15:11:37.000000000 +1000
> @@ -27,6 +27,14 @@ struct task_struct *kthread_create(int (
>  				   void *data,
>  				   const char namefmt[], ...);
>
> +struct task_struct *_kthread_create(int (*threadfn)(void *data),
> +				   void *data,
> +				   unsigned long freezer_flags,
> +				   const char namefmt[], ...);
> +

This should be __kthread_create(...)

> -#define kthread_run(threadfn, data, namefmt, ...)			   \
> +#define kthread_run(threadfn, data, namefmt, args...)			   \
>  ({									   \
>  	struct task_struct *__k						   \
> -		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
> +		= kthread_create(threadfn, data, namefmt, ##args);	   \
>  	if (!IS_ERR(__k))						   \
>  		wake_up_process(__k);					   \
>  	__k;								   \
>  })
>
> +#define kthread_nofreeze_run(threadfn, data, namefmt, args...)		   \
> +({									   \
> +	struct task_struct *__k	= kthread_nofreeze_create(threadfn, data,  \
> +			namefmt, ##args);				   \
> +	if (!IS_ERR(__k))						   \
> +		wake_up_process(__k);					   \
> +	__k;								   \
> +})

Do these functions need to be inlined?

> @@ -86,6 +87,10 @@ static int kthread(void *_create)
>  	/* By default we can run anywhere, unlike keventd. */
>  	set_cpus_allowed(current, CPU_MASK_ALL);
>
> +	/* Set our freezer flags */
> +	current->flags &= ~(PF_SYNCTHREAD | PF_NOFREEZE);
> +	current->flags |= (create->freezer_flags & PF_NOFREEZE);
> +

Maybe these should be encapsulated in a helper in include/linux/sched.h
like some other flags manipulations are?

> diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
> --- 400-workthreads.patch-old/kernel/sched.c	2005-07-21 04:00:02.000000000 +1000
> +++ 400-workthreads.patch-new/kernel/sched.c	2005-07-21 04:00:19.000000000 +1000
> @@ -4580,10 +4580,10 @@ static int migration_call(struct notifie
>
>  	switch (action) {
>  	case CPU_UP_PREPARE:
> -		p = kthread_create(migration_thread, hcpu, "migration/%d",cpu);
> +		p = kthread_create(migration_thread, hcpu,
> +				"migration/%d",cpu);

This is unnecessary.

Overall, it looks pretty good.

Thanks,



	Pat

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Nigel Cunningham]

Hi.

On Fri, 2005-07-22 at 05:42, Patrick Mochel wrote:
> On Thu, 21 Jul 2005, Nigel Cunningham wrote:
> 
> > This patch implements freezer support for workqueues. The current
> > refrigerator implementation makes all workqueues NOFREEZE, regardless of
> > whether they need to be or not.
> 
> A few comments..
> 
> > Signed-off by: Nigel Cunningham <nigel@suspend2.net>
> >
> >  drivers/acpi/osl.c          |    2 +-
> >  drivers/block/ll_rw_blk.c   |    2 +-
> >  drivers/char/hvc_console.c  |    2 +-
> >  drivers/char/hvcs.c         |    2 +-
> >  drivers/input/serio/serio.c |    2 +-
> >  drivers/md/dm-crypt.c       |    2 +-
> >  drivers/scsi/hosts.c        |    2 +-
> >  drivers/usb/net/pegasus.c   |    2 +-
> 
> If you want some practice splitting things up, submit the patches above
> individually to the maintainers o the relevant code once the patches you
> submit below get merged to -mm.

Ok. Thanks for telling me.

> >  include/linux/kthread.h     |   20 ++++++++++++++++++--
> >  include/linux/workqueue.h   |    9 ++++++---
> >  kernel/kmod.c               |    4 ++++
> >  kernel/kthread.c            |   23 ++++++++++++++++++++++-
> >  kernel/sched.c              |    4 ++--
> >  kernel/softirq.c            |    3 +--
> >  kernel/workqueue.c          |   21 ++++++++++++---------
> >  15 files changed, 73 insertions(+), 27 deletions(-)
> 
> 
> You should make sure that you get an explicit ACK from people (Ingo et al)
> about whether this is an acceptable interface.

Ok. How do I know who to ask? (Who besides Ingo, and could I learn who
without help - Maintainers?)

> > --- 400-workthreads.patch-old/include/linux/kthread.h	2004-11-03 21:51:12.000000000 +1100
> > +++ 400-workthreads.patch-new/include/linux/kthread.h	2005-07-20 15:11:37.000000000 +1000
> > @@ -27,6 +27,14 @@ struct task_struct *kthread_create(int (
> >  				   void *data,
> >  				   const char namefmt[], ...);
> >
> > +struct task_struct *_kthread_create(int (*threadfn)(void *data),
> > +				   void *data,
> > +				   unsigned long freezer_flags,
> > +				   const char namefmt[], ...);
> > +
> 
> This should be __kthread_create(...)

Ok. Fixed. Is one underscore ever right?

> > -#define kthread_run(threadfn, data, namefmt, ...)			   \
> > +#define kthread_run(threadfn, data, namefmt, args...)			   \
> >  ({									   \
> >  	struct task_struct *__k						   \
> > -		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
> > +		= kthread_create(threadfn, data, namefmt, ##args);	   \
> >  	if (!IS_ERR(__k))						   \
> >  		wake_up_process(__k);					   \
> >  	__k;								   \
> >  })
> >
> > +#define kthread_nofreeze_run(threadfn, data, namefmt, args...)		   \
> > +({									   \
> > +	struct task_struct *__k	= kthread_nofreeze_create(threadfn, data,  \
> > +			namefmt, ##args);				   \
> > +	if (!IS_ERR(__k))						   \
> > +		wake_up_process(__k);					   \
> > +	__k;								   \
> > +})
> 
> Do these functions need to be inlined?

I tried to find out how to pass the va_list on nicely without using a
#define, but could find the info. If you're able to tell me, I'll make
them inline. Perhaps I could also improve the kthread_create call Pavel
and Ingo commented on.

> > @@ -86,6 +87,10 @@ static int kthread(void *_create)
> >  	/* By default we can run anywhere, unlike keventd. */
> >  	set_cpus_allowed(current, CPU_MASK_ALL);
> >
> > +	/* Set our freezer flags */
> > +	current->flags &= ~(PF_SYNCTHREAD | PF_NOFREEZE);
> > +	current->flags |= (create->freezer_flags & PF_NOFREEZE);
> > +
> 
> Maybe these should be encapsulated in a helper in include/linux/sched.h
> like some other flags manipulations are?

This would be the only place it's used. Does that matter? (And note from
the updated patch that the SYNCTHREAD wouldn't be there).

> > diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
> > --- 400-workthreads.patch-old/kernel/sched.c	2005-07-21 04:00:02.000000000 +1000
> > +++ 400-workthreads.patch-new/kernel/sched.c	2005-07-21 04:00:19.000000000 +1000
> > @@ -4580,10 +4580,10 @@ static int migration_call(struct notifie
> >
> >  	switch (action) {
> >  	case CPU_UP_PREPARE:
> > -		p = kthread_create(migration_thread, hcpu, "migration/%d",cpu);
> > +		p = kthread_create(migration_thread, hcpu,
> > +				"migration/%d",cpu);
> 
> This is unnecessary.

Oops. Comes from adding an extra parameter, fixing line length and then
removing it. Fixed.

> Overall, it looks pretty good.

Thanks!

Nigel

> Thanks,
> 
> 
> 
> 	Pat
-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Nigel Cunningham]

Hi.

I finally found some time to finish this off. I don't really like the
end result - the macros looked clearer to me - but here goes. If it
looks okay, I'll seek sign offs from each of the affected driver
maintainers and from Ingo. Anyone else?

Regards,

Nigel

 drivers/acpi/osl.c          |    2 
 drivers/block/ll_rw_blk.c   |    2 
 drivers/char/hvc_console.c  |    2 
 drivers/char/hvcs.c         |    2 
 drivers/input/serio/serio.c |    2 
 drivers/md/dm-crypt.c       |    2 
 drivers/scsi/hosts.c        |    2 
 drivers/usb/net/pegasus.c   |    2 
 include/linux/kthread.h     |   27 ++++++++----
 include/linux/workqueue.h   |    9 ++--
 kernel/kthread.c            |   94 ++++++++++++++++++++++++++++++++++++++++----
 kernel/sched.c              |    1 
 kernel/softirq.c            |    3 -
 kernel/workqueue.c          |   36 +++++++++++-----
 14 files changed, 144 insertions(+), 42 deletions(-)
diff -ruNp 400-workthreads.patch-old/drivers/acpi/osl.c 400-workthreads.patch-new/drivers/acpi/osl.c
--- 400-workthreads.patch-old/drivers/acpi/osl.c	2005-08-02 22:30:57.000000000 +1000
+++ 400-workthreads.patch-new/drivers/acpi/osl.c	2005-08-02 22:33:49.000000000 +1000
@@ -98,7 +98,7 @@ acpi_os_initialize1(void)
 		return AE_NULL_ENTRY;
 	}
 #endif
-	kacpid_wq = create_singlethread_workqueue("kacpid");
+	kacpid_wq = create_nofreeze_singlethread_workqueue("kacpid");
 	BUG_ON(!kacpid_wq);
 
 	return AE_OK;
diff -ruNp 400-workthreads.patch-old/drivers/block/ll_rw_blk.c 400-workthreads.patch-new/drivers/block/ll_rw_blk.c
--- 400-workthreads.patch-old/drivers/block/ll_rw_blk.c	2005-08-02 22:30:57.000000000 +1000
+++ 400-workthreads.patch-new/drivers/block/ll_rw_blk.c	2005-08-02 22:33:49.000000000 +1000
@@ -3215,7 +3215,7 @@ EXPORT_SYMBOL(kblockd_flush);
 
 int __init blk_dev_init(void)
 {
-	kblockd_workqueue = create_workqueue("kblockd");
+	kblockd_workqueue = create_nofreeze_workqueue("kblockd");
 	if (!kblockd_workqueue)
 		panic("Failed to create kblockd\n");
 
diff -ruNp 400-workthreads.patch-old/drivers/char/hvc_console.c 400-workthreads.patch-new/drivers/char/hvc_console.c
--- 400-workthreads.patch-old/drivers/char/hvc_console.c	2005-08-02 22:30:58.000000000 +1000
+++ 400-workthreads.patch-new/drivers/char/hvc_console.c	2005-08-02 22:33:49.000000000 +1000
@@ -844,7 +844,7 @@ int __init hvc_init(void)
 
 	/* Always start the kthread because there can be hotplug vty adapters
 	 * added later. */
-	hvc_task = kthread_run(khvcd, NULL, "khvcd");
+	hvc_task = kthread_nofreeze_run(khvcd, NULL, "khvcd");
 	if (IS_ERR(hvc_task)) {
 		panic("Couldn't create kthread for console.\n");
 		put_tty_driver(hvc_driver);
diff -ruNp 400-workthreads.patch-old/drivers/char/hvcs.c 400-workthreads.patch-new/drivers/char/hvcs.c
--- 400-workthreads.patch-old/drivers/char/hvcs.c	2005-08-02 22:30:58.000000000 +1000
+++ 400-workthreads.patch-new/drivers/char/hvcs.c	2005-08-02 22:33:49.000000000 +1000
@@ -1403,7 +1403,7 @@ static int __init hvcs_module_init(void)
 		return -ENOMEM;
 	}
 
-	hvcs_task = kthread_run(khvcsd, NULL, "khvcsd");
+	hvcs_task = kthread_nofreeze_run(khvcsd, NULL, "khvcsd");
 	if (IS_ERR(hvcs_task)) {
 		printk(KERN_ERR "HVCS: khvcsd creation failed.  Driver not loaded.\n");
 		kfree(hvcs_pi_buff);
diff -ruNp 400-workthreads.patch-old/drivers/input/serio/serio.c 400-workthreads.patch-new/drivers/input/serio/serio.c
--- 400-workthreads.patch-old/drivers/input/serio/serio.c	2005-08-04 11:48:01.000000000 +1000
+++ 400-workthreads.patch-new/drivers/input/serio/serio.c	2005-08-02 22:33:49.000000000 +1000
@@ -899,7 +899,7 @@ irqreturn_t serio_interrupt(struct serio
 
 static int __init serio_init(void)
 {
-	serio_task = kthread_run(serio_thread, NULL, "kseriod");
+	serio_task = kthread_nofreeze_run(serio_thread, NULL, "kseriod");
 	if (IS_ERR(serio_task)) {
 		printk(KERN_ERR "serio: Failed to start kseriod\n");
 		return PTR_ERR(serio_task);
diff -ruNp 400-workthreads.patch-old/drivers/md/dm-crypt.c 400-workthreads.patch-new/drivers/md/dm-crypt.c
--- 400-workthreads.patch-old/drivers/md/dm-crypt.c	2005-08-02 22:31:02.000000000 +1000
+++ 400-workthreads.patch-new/drivers/md/dm-crypt.c	2005-08-02 22:33:49.000000000 +1000
@@ -926,7 +926,7 @@ static int __init dm_crypt_init(void)
 	if (!_crypt_io_pool)
 		return -ENOMEM;
 
-	_kcryptd_workqueue = create_workqueue("kcryptd");
+	_kcryptd_workqueue = create_nofreeze_workqueue("kcryptd");
 	if (!_kcryptd_workqueue) {
 		r = -ENOMEM;
 		DMERR(PFX "couldn't create kcryptd");
diff -ruNp 400-workthreads.patch-old/drivers/scsi/hosts.c 400-workthreads.patch-new/drivers/scsi/hosts.c
--- 400-workthreads.patch-old/drivers/scsi/hosts.c	2005-08-02 22:31:10.000000000 +1000
+++ 400-workthreads.patch-new/drivers/scsi/hosts.c	2005-08-02 22:33:49.000000000 +1000
@@ -132,7 +132,7 @@ int scsi_add_host(struct Scsi_Host *shos
 	if (shost->transportt->create_work_queue) {
 		snprintf(shost->work_q_name, KOBJ_NAME_LEN, "scsi_wq_%d",
 			shost->host_no);
-		shost->work_q = create_singlethread_workqueue(
+		shost->work_q = create_nofreeze_singlethread_workqueue(
 					shost->work_q_name);
 		if (!shost->work_q)
 			goto out_free_shost_data;
diff -ruNp 400-workthreads.patch-old/drivers/usb/net/pegasus.c 400-workthreads.patch-new/drivers/usb/net/pegasus.c
--- 400-workthreads.patch-old/drivers/usb/net/pegasus.c	2005-08-02 22:31:15.000000000 +1000
+++ 400-workthreads.patch-new/drivers/usb/net/pegasus.c	2005-08-02 22:33:49.000000000 +1000
@@ -1411,7 +1411,7 @@ static struct usb_driver pegasus_driver 
 static int __init pegasus_init(void)
 {
 	pr_info("%s: %s, " DRIVER_DESC "\n", driver_name, DRIVER_VERSION);
-	pegasus_workqueue = create_singlethread_workqueue("pegasus");
+	pegasus_workqueue = create_nofreeze_singlethread_workqueue("pegasus");
 	if (!pegasus_workqueue)
 		return -ENOMEM;
 	return usb_register(&pegasus_driver);
diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h
--- 400-workthreads.patch-old/include/linux/kthread.h	2004-11-03 21:51:12.000000000 +1100
+++ 400-workthreads.patch-new/include/linux/kthread.h	2005-08-03 11:52:01.000000000 +1000
@@ -23,10 +23,20 @@
  *
  * Returns a task_struct or ERR_PTR(-ENOMEM).
  */
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				   void *data,
+				   unsigned long freezer_flags,
+				   const char namefmt[],
+				   va_list * args);
+
 struct task_struct *kthread_create(int (*threadfn)(void *data),
 				   void *data,
 				   const char namefmt[], ...);
 
+struct task_struct *kthread_nofreeze_create(int (*threadfn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
 /**
  * kthread_run: create and wake a thread.
  * @threadfn: the function to run until signal_pending(current).
@@ -35,14 +45,15 @@ struct task_struct *kthread_create(int (
  *
  * Description: Convenient wrapper for kthread_create() followed by
  * wake_up_process().  Returns the kthread, or ERR_PTR(-ENOMEM). */
-#define kthread_run(threadfn, data, namefmt, ...)			   \
-({									   \
-	struct task_struct *__k						   \
-		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
-	if (!IS_ERR(__k))						   \
-		wake_up_process(__k);					   \
-	__k;								   \
-})
+
+extern struct task_struct * kthread_run(int (*threadfn)(void *data),
+					void *data,
+					const char namefmt[], ...);
+
+extern struct task_struct * kthread_nofreeze_run(int (*threadfn)(void *data),
+					void *data,
+					const char namefmt[], ...);
+
 
 /**
  * kthread_bind: bind a just-created kthread to a cpu.
diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
--- 400-workthreads.patch-old/include/linux/workqueue.h	2005-06-20 11:47:30.000000000 +1000
+++ 400-workthreads.patch-new/include/linux/workqueue.h	2005-08-03 11:49:34.000000000 +1000
@@ -51,9 +51,12 @@ struct work_struct {
 	} while (0)
 
 extern struct workqueue_struct *__create_workqueue(const char *name,
-						    int singlethread);
-#define create_workqueue(name) __create_workqueue((name), 0)
-#define create_singlethread_workqueue(name) __create_workqueue((name), 1)
+						    int singlethread,
+						    unsigned long freezer_flag);
+#define create_workqueue(name) __create_workqueue((name), 0, 0)
+#define create_nofreeze_workqueue(name) __create_workqueue((name), 0, PF_NOFREEZE)
+#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0)
+#define create_nofreeze_singlethread_workqueue(name) __create_workqueue((name), 1, PF_NOFREEZE)
 
 extern void destroy_workqueue(struct workqueue_struct *wq);
 
diff -ruNp 400-workthreads.patch-old/kernel/kthread.c 400-workthreads.patch-new/kernel/kthread.c
--- 400-workthreads.patch-old/kernel/kthread.c	2005-06-20 11:47:31.000000000 +1000
+++ 400-workthreads.patch-new/kernel/kthread.c	2005-08-04 09:46:43.000000000 +1000
@@ -25,6 +25,7 @@ struct kthread_create_info
 	/* Information passed to kthread() from keventd. */
 	int (*threadfn)(void *data);
 	void *data;
+	unsigned long freezer_flags;
 	struct completion started;
 
 	/* Result passed back to kthread_create() from keventd. */
@@ -86,6 +87,10 @@ static int kthread(void *_create)
 	/* By default we can run anywhere, unlike keventd. */
 	set_cpus_allowed(current, CPU_MASK_ALL);
 
+	/* Set our freezer flags */
+	current->flags &= ~PF_NOFREEZE;
+	current->flags |= (create->freezer_flags & PF_NOFREEZE);
+
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_INTERRUPTIBLE);
 	complete(&create->started);
@@ -119,16 +124,18 @@ static void keventd_create_kthread(void 
 	complete(&create->done);
 }
 
-struct task_struct *kthread_create(int (*threadfn)(void *data),
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
 				   void *data,
+				   unsigned long freezer_flags,
 				   const char namefmt[],
-				   ...)
+				   va_list * args)
 {
 	struct kthread_create_info create;
 	DECLARE_WORK(work, keventd_create_kthread, &create);
 
 	create.threadfn = threadfn;
 	create.data = data;
+	create.freezer_flags = freezer_flags;
 	init_completion(&create.started);
 	init_completion(&create.done);
 
@@ -141,18 +148,89 @@ struct task_struct *kthread_create(int (
 		queue_work(helper_wq, &work);
 		wait_for_completion(&create.done);
 	}
-	if (!IS_ERR(create.result)) {
-		va_list args;
-		va_start(args, namefmt);
+	if (!IS_ERR(create.result))
 		vsnprintf(create.result->comm, sizeof(create.result->comm),
-			  namefmt, args);
-		va_end(args);
-	}
+			  namefmt, *args);
 
 	return create.result;
 }
+
+struct task_struct *kthread_create(int (*threadfn)(void *data),
+				   void *data,
+				   const char namefmt[], ...)
+{
+	struct task_struct * result;
+
+	va_list args;
+	va_start(args, namefmt);
+	result = __kthread_create(threadfn, data, 0, namefmt, &args);
+	va_end(args);
+	return result;
+}
+
 EXPORT_SYMBOL(kthread_create);
 
+struct task_struct *kthread_nofreeze_create(int (*threadfn)(void *data),
+				   void *data,
+				   const char namefmt[], ...)
+{
+	struct task_struct * result;
+
+	va_list args;
+	va_start(args, namefmt);
+	result = __kthread_create(threadfn, data, PF_NOFREEZE, namefmt, &args);
+	va_end(args);
+	return result;
+}
+
+EXPORT_SYMBOL(kthread_nofreeze_create);
+
+/**
+ * kthread_run: create and wake a thread.
+ * @threadfn: the function to run until signal_pending(current).
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * wake_up_process().  Returns the kthread, or ERR_PTR(-ENOMEM).
+ **/
+struct task_struct * kthread_run(int (*threadfn)(void *data),
+		void *data,
+		const char namefmt[], ...)
+{
+	struct task_struct *__k;
+	va_list args;
+
+	va_start(args, namefmt);
+	__k = __kthread_create(threadfn, data, 0, namefmt, &args);
+	va_end(args);
+
+	if(!IS_ERR(__k))
+		wake_up_process(__k);
+
+	return __k;
+}
+
+EXPORT_SYMBOL(kthread_run);
+
+struct task_struct * kthread_nofreeze_run(int (*threadfn)(void *data),
+		void *data,
+		const char namefmt[], ...)
+{
+	struct task_struct *__k;
+	va_list args;
+
+	va_start(args, namefmt);
+	__k = __kthread_create(threadfn, data, PF_NOFREEZE, namefmt, &args);
+	va_end(args);
+
+	if(!IS_ERR(__k))
+		wake_up_process(__k);
+
+	return __k;
+}
+EXPORT_SYMBOL(kthread_nofreeze_run);
+
 void kthread_bind(struct task_struct *k, unsigned int cpu)
 {
 	BUG_ON(k->state != TASK_INTERRUPTIBLE);
diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
--- 400-workthreads.patch-old/kernel/sched.c	2005-08-04 11:48:00.000000000 +1000
+++ 400-workthreads.patch-new/kernel/sched.c	2005-08-04 11:48:19.000000000 +1000
@@ -4585,7 +4585,6 @@ static int migration_call(struct notifie
 		p = kthread_create(migration_thread, hcpu, "migration/%d",cpu);
 		if (IS_ERR(p))
 			return NOTIFY_BAD;
-		p->flags |= PF_NOFREEZE;
 		kthread_bind(p, cpu);
 		/* Must be high prio: stop_machine expects to yield to it. */
 		rq = task_rq_lock(p, &flags);
diff -ruNp 400-workthreads.patch-old/kernel/softirq.c 400-workthreads.patch-new/kernel/softirq.c
--- 400-workthreads.patch-old/kernel/softirq.c	2005-06-20 11:47:32.000000000 +1000
+++ 400-workthreads.patch-new/kernel/softirq.c	2005-08-02 22:33:49.000000000 +1000
@@ -350,7 +350,6 @@ void __init softirq_init(void)
 static int ksoftirqd(void * __bind_cpu)
 {
 	set_user_nice(current, 19);
-	current->flags |= PF_NOFREEZE;
 
 	set_current_state(TASK_INTERRUPTIBLE);
 
@@ -456,7 +455,7 @@ static int __devinit cpu_callback(struct
 	case CPU_UP_PREPARE:
 		BUG_ON(per_cpu(tasklet_vec, hotcpu).list);
 		BUG_ON(per_cpu(tasklet_hi_vec, hotcpu).list);
-		p = kthread_create(ksoftirqd, hcpu, "ksoftirqd/%d", hotcpu);
+		p = kthread_nofreeze_create(ksoftirqd, hcpu, "ksoftirqd/%d", hotcpu);
 		if (IS_ERR(p)) {
 			printk("ksoftirqd for %i failed\n", hotcpu);
 			return NOTIFY_BAD;
diff -ruNp 400-workthreads.patch-old/kernel/workqueue.c 400-workthreads.patch-new/kernel/workqueue.c
--- 400-workthreads.patch-old/kernel/workqueue.c	2005-06-20 11:47:32.000000000 +1000
+++ 400-workthreads.patch-new/kernel/workqueue.c	2005-08-03 11:57:01.000000000 +1000
@@ -186,8 +186,6 @@ static int worker_thread(void *__cwq)
 	struct k_sigaction sa;
 	sigset_t blocked;
 
-	current->flags |= PF_NOFREEZE;
-
 	set_user_nice(current, -5);
 
 	/* Block and flush all signals */
@@ -208,6 +206,7 @@ static int worker_thread(void *__cwq)
 			schedule();
 		else
 			__set_current_state(TASK_RUNNING);
+		try_to_freeze();
 		remove_wait_queue(&cwq->more_work, &wait);
 
 		if (!list_empty(&cwq->worklist))
@@ -277,7 +276,8 @@ void fastcall flush_workqueue(struct wor
 }
 
 static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq,
-						   int cpu)
+						   int cpu,
+						   unsigned long freezer_flags)
 {
 	struct cpu_workqueue_struct *cwq = wq->cpu_wq + cpu;
 	struct task_struct *p;
@@ -291,10 +291,21 @@ static struct task_struct *create_workqu
 	init_waitqueue_head(&cwq->more_work);
 	init_waitqueue_head(&cwq->work_done);
 
-	if (is_single_threaded(wq))
-		p = kthread_create(worker_thread, cwq, "%s", wq->name);
-	else
-		p = kthread_create(worker_thread, cwq, "%s/%d", wq->name, cpu);
+	if (is_single_threaded(wq)) {
+		if (freezer_flags)
+			p = kthread_nofreeze_create(worker_thread, cwq,
+					"%s", wq->name);
+		else
+			p = kthread_create(worker_thread, cwq,
+					"%s", wq->name);
+	} else {
+		if (freezer_flags)
+			p = kthread_nofreeze_create(worker_thread, cwq,
+					"%s/%d", wq->name, cpu);
+		else
+			p = kthread_create(worker_thread, cwq,
+					"%s/%d", wq->name, cpu);
+	}
 	if (IS_ERR(p))
 		return NULL;
 	cwq->thread = p;
@@ -302,7 +313,8 @@ static struct task_struct *create_workqu
 }
 
 struct workqueue_struct *__create_workqueue(const char *name,
-					    int singlethread)
+					    int singlethread,
+					    unsigned long freezer_flags)
 {
 	int cpu, destroy = 0;
 	struct workqueue_struct *wq;
@@ -320,7 +332,7 @@ struct workqueue_struct *__create_workqu
 	lock_cpu_hotplug();
 	if (singlethread) {
 		INIT_LIST_HEAD(&wq->list);
-		p = create_workqueue_thread(wq, 0);
+		p = create_workqueue_thread(wq, 0, freezer_flags);
 		if (!p)
 			destroy = 1;
 		else
@@ -330,7 +342,7 @@ struct workqueue_struct *__create_workqu
 		list_add(&wq->list, &workqueues);
 		spin_unlock(&workqueue_lock);
 		for_each_online_cpu(cpu) {
-			p = create_workqueue_thread(wq, cpu);
+			p = create_workqueue_thread(wq, cpu, freezer_flags);
 			if (p) {
 				kthread_bind(p, cpu);
 				wake_up_process(p);
@@ -501,7 +513,7 @@ static int __devinit workqueue_cpu_callb
 	case CPU_UP_PREPARE:
 		/* Create a new workqueue thread for it. */
 		list_for_each_entry(wq, &workqueues, list) {
-			if (create_workqueue_thread(wq, hotcpu) < 0) {
+			if (create_workqueue_thread(wq, hotcpu, 0) < 0) {
 				printk("workqueue for %i failed\n", hotcpu);
 				return NOTIFY_BAD;
 			}
@@ -540,7 +552,7 @@ static int __devinit workqueue_cpu_callb
 void init_workqueues(void)
 {
 	hotcpu_notifier(workqueue_cpu_callback, 0);
-	keventd_wq = create_workqueue("events");
+	keventd_wq = create_nofreeze_workqueue("events");
 	BUG_ON(!keventd_wq);
 }
 

-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Patrick Mochel]

On Fri, 5 Aug 2005, Nigel Cunningham wrote:

> Hi.
>
> I finally found some time to finish this off. I don't really like the
> end result - the macros looked clearer to me - but here goes. If it
> looks okay, I'll seek sign offs from each of the affected driver
> maintainers and from Ingo. Anyone else?

What are your feelings about this: http://lwn.net/Articles/145417/ ?

It seems like a cleaner way to do things. How would these patches change
if that were merged?

Concerning the patch specifically:

> diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h
> --- 400-workthreads.patch-old/include/linux/kthread.h	2004-11-03 21:51:12.000000000 +1100
> +++ 400-workthreads.patch-new/include/linux/kthread.h	2005-08-03 11:52:01.000000000 +1000
> @@ -23,10 +23,20 @@
>   *
>   * Returns a task_struct or ERR_PTR(-ENOMEM).
>   */
> +struct task_struct *__kthread_create(int (*threadfn)(void *data),
> +				   void *data,
> +				   unsigned long freezer_flags,
> +				   const char namefmt[],
> +				   va_list * args);
> +

When comparing this to this:

> diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
> --- 400-workthreads.patch-old/include/linux/workqueue.h	2005-06-20 11:47:30.000000000 +1000
> +++ 400-workthreads.patch-new/include/linux/workqueue.h	2005-08-03 11:49:34.000000000 +1000
> @@ -51,9 +51,12 @@ struct work_struct {
>  	} while (0)
>
>  extern struct workqueue_struct *__create_workqueue(const char *name,
> -						    int singlethread);
> -#define create_workqueue(name) __create_workqueue((name), 0)
> -#define create_singlethread_workqueue(name) __create_workqueue((name), 1)
> +						    int singlethread,
> +						    unsigned long freezer_flag);
> +#define create_workqueue(name) __create_workqueue((name), 0, 0)
> +#define create_nofreeze_workqueue(name) __create_workqueue((name), 0, PF_NOFREEZE)
> +#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0)
> +#define create_nofreeze_singlethread_workqueue(name) __create_workqueue((name), 1, PF_NOFREEZE)

And to this:


>  static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq,
> -						   int cpu)
> +						   int cpu,
> +						   unsigned long freezer_flags)
>  {

There is a slight discrepancy in the API changes. Obviously, we don't want
to change every caller of create_workqueue() to support this. But, perhaps
you could standardize the handling of the freezer flags (by e.g. creating
a separate _nofreeze version for each).

Also, functions that take a va_list parameter pass the structure (array)
rather than the pointer. See the definition of vprintk() in
include/linux/kernel.h for an example.


Thanks,


	Pat

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Nigel Cunningham]

Hi.

Sorry for the slow response. Busy still.

On Sat, 2005-08-06 at 15:06, Patrick Mochel wrote:
> On Fri, 5 Aug 2005, Nigel Cunningham wrote:
> 
> > Hi.
> >
> > I finally found some time to finish this off. I don't really like the
> > end result - the macros looked clearer to me - but here goes. If it
> > looks okay, I'll seek sign offs from each of the affected driver
> > maintainers and from Ingo. Anyone else?
> 
> What are your feelings about this: http://lwn.net/Articles/145417/ ?

I'm sure it could work, but I do worry a little about the possibilities
for exploits. It seems to me that if someone can get root, they an
insmod a module that could schedule any kind of work via any process.
Tracing that sort of security hole could be intractable. Christoph, is
that something you've considered/have thoughts on? Perhaps I'm just
being paranoid :>

> It seems like a cleaner way to do things. How would these patches change
> if that were merged?

We would still need some way to mark which threads to freeze, so we'd
still need the same sort of thing as far as kthread_run etc goes.

We'd also still mark unfreezable threads with NOFREEZE and not mark
other threads, so no change there.

The first substantial change I can see would be switching from
try_to_freeze to try_todo_list (could this be 'run_todo_list'? Try
implies failure is possible - if it is possible, why do we ignore
whether it fails?).

The second substantial change would be in the area of the refrigeration
code itself, ala Christoph's patch.

One more question regarding Christoph's patch - would it be worth moving
the refrigerator related code from sched.h to a separate file? We
already have so many things that depend on sched.h, and this will add
more. It will also make maintenance less painful because you won't have
to recompile everything that depends on sched.h when all you did was
modify (say) freezing().

> Concerning the patch specifically:
> 
> > diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h
> > --- 400-workthreads.patch-old/include/linux/kthread.h	2004-11-03 21:51:12.000000000 +1100
> > +++ 400-workthreads.patch-new/include/linux/kthread.h	2005-08-03 11:52:01.000000000 +1000
> > @@ -23,10 +23,20 @@
> >   *
> >   * Returns a task_struct or ERR_PTR(-ENOMEM).
> >   */
> > +struct task_struct *__kthread_create(int (*threadfn)(void *data),
> > +				   void *data,
> > +				   unsigned long freezer_flags,
> > +				   const char namefmt[],
> > +				   va_list * args);
> > +
> 
> When comparing this to this:
> 
> > diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
> > --- 400-workthreads.patch-old/include/linux/workqueue.h	2005-06-20 11:47:30.000000000 +1000
> > +++ 400-workthreads.patch-new/include/linux/workqueue.h	2005-08-03 11:49:34.000000000 +1000
> > @@ -51,9 +51,12 @@ struct work_struct {
> >  	} while (0)
> >
> >  extern struct workqueue_struct *__create_workqueue(const char *name,
> > -						    int singlethread);
> > -#define create_workqueue(name) __create_workqueue((name), 0)
> > -#define create_singlethread_workqueue(name) __create_workqueue((name), 1)
> > +						    int singlethread,
> > +						    unsigned long freezer_flag);
> > +#define create_workqueue(name) __create_workqueue((name), 0, 0)
> > +#define create_nofreeze_workqueue(name) __create_workqueue((name), 0, PF_NOFREEZE)
> > +#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0)
> > +#define create_nofreeze_singlethread_workqueue(name) __create_workqueue((name), 1, PF_NOFREEZE)
> 
> And to this:
> 
> 
> >  static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq,
> > -						   int cpu)
> > +						   int cpu,
> > +						   unsigned long freezer_flags)
> >  {
> 
> There is a slight discrepancy in the API changes. Obviously, we don't want
> to change every caller of create_workqueue() to support this. But, perhaps
> you could standardize the handling of the freezer flags (by e.g. creating
> a separate _nofreeze version for each).

Missed that one, sorry.

> Also, functions that take a va_list parameter pass the structure (array)
> rather than the pointer. See the definition of vprintk() in
> include/linux/kernel.h for an example.

Thanks for the pointer.

Nigel

> 
> Thanks,
> 
> 
> 	Pat
> 
> ______________________________________________________________________
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/linux-pm
-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Con Kolivas]

On Mon, 8 Aug 2005 10:46 am, Nigel Cunningham wrote:
> Hi.
>
> Sorry for the slow response. Busy still.
>
> On Sat, 2005-08-06 at 15:06, Patrick Mochel wrote:
> > On Fri, 5 Aug 2005, Nigel Cunningham wrote:
> > > Hi.
> > >
> > > I finally found some time to finish this off. I don't really like the
> > > end result - the macros looked clearer to me - but here goes. If it
> > > looks okay, I'll seek sign offs from each of the affected driver
> > > maintainers and from Ingo. Anyone else?
> >
> > What are your feelings about this: http://lwn.net/Articles/145417/ ?
>
> I'm sure it could work, but I do worry a little about the possibilities
> for exploits. It seems to me that if someone can get root, they an
> insmod a module that could schedule any kind of work via any process.
> Tracing that sort of security hole could be intractable. Christoph, is
> that something you've considered/have thoughts on? Perhaps I'm just
> being paranoid :>

If someone gets root access it means you're already exploited.

Cheers,
Con

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Nigel Cunningham]

Hi.

On Mon, 2005-08-08 at 11:27, Con Kolivas wrote:
> On Mon, 8 Aug 2005 10:46 am, Nigel Cunningham wrote:
> > Hi.
> >
> > Sorry for the slow response. Busy still.
> >
> > On Sat, 2005-08-06 at 15:06, Patrick Mochel wrote:
> > > On Fri, 5 Aug 2005, Nigel Cunningham wrote:
> > > > Hi.
> > > >
> > > > I finally found some time to finish this off. I don't really like the
> > > > end result - the macros looked clearer to me - but here goes. If it
> > > > looks okay, I'll seek sign offs from each of the affected driver
> > > > maintainers and from Ingo. Anyone else?
> > >
> > > What are your feelings about this: http://lwn.net/Articles/145417/ ?
> >
> > I'm sure it could work, but I do worry a little about the possibilities
> > for exploits. It seems to me that if someone can get root, they an
> > insmod a module that could schedule any kind of work via any process.
> > Tracing that sort of security hole could be intractable. Christoph, is
> > that something you've considered/have thoughts on? Perhaps I'm just
> > being paranoid :>
> 
> If someone gets root access it means you're already exploited.

Yeah, true. Ok. Lame thought :>

Nigel

> Cheers,
> Con
-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.

Re: [linux-pm] [PATCH] Workqueue freezer support.

[Nigel Cunningham]

Ok. Now that I've actually done some work toward getting it to work with
Suspend2, I'll give a more cogent response to Christoph's approach.

I believe it can work, but the algorithm in freeze() is a bit of a
concern.

Checking whether the todo list is empty is fine while we're the only
user, but when other functionality is using this, it will be unreliable.
We'll either need a quick way to check what todo work is pending for a
task (traverse list would be racy). We can't say "We know we signalled
them all already" because more might have forked.

One possibility might be to leverage the existing counting - when
num_signalled = num_frozen, we can signal a new batch with impunity. But
this assumes everything signalled properly enters the fridge (which
might not always be guaranteed).

Perhaps it's best to keep PF_FREEZING? Perhaps there's another
possibility.

Regards,

Nigel
-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.