Workqueue freezer support.

PATCH In review Nigel Cunningham 6 participants 12 messages Jul 21, 2005 – Aug 8, 2005

Subsystems (8)

SCSI USB Block layer ACPI +4 more subsystems

lore

Share & tools

lore

Nigel Cunningham

author

2005-07-21 05:16 UTC #0 lore raw

PATCH 1/1 Workqueue freezer support.

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 <[email protected]>

 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(-)

drivers/acpi/osl.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/acpi/osl.c 400-workthreads.patch-new/drivers/acpi/osl.c
@@ -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;

drivers/block/ll_rw_blk.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/block/ll_rw_blk.c 400-workthreads.patch-new/drivers/block/ll_rw_blk.c
@@ -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");

drivers/char/hvc_console.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/char/hvc_console.c 400-workthreads.patch-new/drivers/char/hvc_console.c
@@ -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);

drivers/char/hvcs.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/char/hvcs.c 400-workthreads.patch-new/drivers/char/hvcs.c
@@ -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);

drivers/input/serio/serio.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/input/serio/serio.c 400-workthreads.patch-new/drivers/input/serio/serio.c
@@ -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);

drivers/md/dm-crypt.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/md/dm-crypt.c 400-workthreads.patch-new/drivers/md/dm-crypt.c
@@ -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");

drivers/scsi/hosts.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/scsi/hosts.c 400-workthreads.patch-new/drivers/scsi/hosts.c
@@ -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;

drivers/usb/net/pegasus.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/usb/net/pegasus.c 400-workthreads.patch-new/drivers/usb/net/pegasus.c
@@ -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);

include/linux/kthread.h +18 -2

diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h
@@ -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().

include/linux/workqueue.h +6 -3

diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
@@ -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);

kernel/kthread.c +22 -1

diff -ruNp 400-workthreads.patch-old/kernel/kthread.c 400-workthreads.patch-new/kernel/kthread.c
@@ -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)

kernel/sched.c +2 -2

diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
@@ -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);

kernel/softirq.c +1 -2

diff -ruNp 400-workthreads.patch-old/kernel/softirq.c 400-workthreads.patch-new/kernel/softirq.c
@@ -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;

kernel/workqueue.c +12 -9

diff -ruNp 400-workthreads.patch-old/kernel/workqueue.c 400-workthreads.patch-new/kernel/workqueue.c
@@ -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);
 }

Signature · 4 lines

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

Pavel Machek

2005-07-21 15:38 UTC #1 lore raw

Hi!

Quoted 31 lines

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 :>.
kernel/kthread.c +4 -0
@@ -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[], ...)

kernel/kthread.c struct task_struct *kthread_create(int ( · line 174

Quoted 10 lines

kernel/kthread.c +10 -0

+{
+	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

Signature · 1 line

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

Ingo Molnar

2005-07-21 15:42 UTC #2 lore raw

Quoted 10 lines · Pavel Machek

Quoted 10 lines

kernel/kthread.c +10 -0

+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);

Quoted 8 lines

kernel/kthread.c struct task_struct *kthread_create(int ( · line 174
Quoted 3 lines
kernel/kthread.c +3 -0
+	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

Pavel Machek

2005-07-21 15:44 UTC #3 lore raw

Hi!

Quoted 19 lines

Quoted 10 lines
Quoted 10 lines
kernel/kthread.c +10 -0
+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);
Quoted 8 lines
kernel/kthread.c struct task_struct *kthread_create(int ( · line 174
Quoted 3 lines
kernel/kthread.c +3 -0
+	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[] :-).

Quoted 1 line

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

Okay then ;-).
Pavel

Signature · 1 line

Boycott Kodak -- for their patent abuse against Java.

Patrick Mochel

2005-07-21 19:42 UTC #4 lore raw

In reply to Nigel Cunningham

Quoted 3 lines · 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.

A few comments..

Quoted 10 lines

Signed-off by: Nigel Cunningham <[email protected]>

 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.

Quoted 8 lines

 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.

include/linux/kthread.h struct task_struct *kthread_create(int ( · line 34

Quoted 11 lines

include/linux/kthread.h +5 -0

@@ -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(...)

Quoted 9 lines

include/linux/kthread.h +2 -2

-#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;								   \

include/linux/kthread.h struct task_struct *kthread_create(int ( · line 62

Quoted 10 lines

include/linux/kthread.h +8 -0

 })
 
+#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?

include/linux/kthread.h static int kthread(void *_create) · line 93

Quoted 8 lines

include/linux/kthread.h +4 -0

@@ -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?

kernel/sched.c static int migration_call(struct notifie · line 4584

Quoted 10 lines

diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c

kernel/sched.c +2 -1

@@ -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

Nigel Cunningham

author

2005-07-22 03:02 UTC #5 lore raw

Hi.

Quoted 19 lines · Patrick Mochel

Quoted 3 lines · 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.
A few comments..
Quoted 10 lines
Signed-off by: Nigel Cunningham <[email protected]>
 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 +-

Quoted 3 lines

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.

Quoted 10 lines

Quoted 8 lines

 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(-)

Quoted 2 lines

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?)

Quoted 6 lines

include/linux/kthread.h +0 -0

@@ -27,6 +27,14 @@ struct task_struct *kthread_create(int (
 				   void *data,
 				   const char namefmt[], ...);

Quoted 7 lines

include/linux/kthread.h struct task_struct *kthread_create(int ( · line 34
Quoted 5 lines
include/linux/kthread.h +5 -0
+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?

Quoted 11 lines

include/linux/kthread.h +2 -2

-#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;								   \
 })

Quoted 10 lines

include/linux/kthread.h struct task_struct *kthread_create(int ( · line 62
Quoted 8 lines
include/linux/kthread.h +8 -0
+#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.

Quoted 11 lines

include/linux/kthread.h static int kthread(void *_create) · line 93
Quoted 8 lines
include/linux/kthread.h +4 -0
@@ -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).

Quoted 5 lines

Quoted 5 lines
diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
kernel/sched.c +0 -0
@@ -4580,10 +4580,10 @@ static int migration_call(struct notifie
 

Quoted 7 lines

kernel/sched.c static int migration_call(struct notifie · line 4584
Quoted 5 lines
kernel/sched.c +2 -1
 	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.

Quoted 1 line

Overall, it looks pretty good.

Thanks!

Nigel

Quoted 5 lines

Thanks,
Pat

Signature · 4 lines

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

Nigel Cunningham

author

2005-08-05 12:12 UTC #6 lore raw

In reply to Patrick Mochel

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(-)

drivers/acpi/osl.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/acpi/osl.c 400-workthreads.patch-new/drivers/acpi/osl.c
@@ -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;

drivers/block/ll_rw_blk.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/block/ll_rw_blk.c 400-workthreads.patch-new/drivers/block/ll_rw_blk.c
@@ -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");

drivers/char/hvc_console.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/char/hvc_console.c 400-workthreads.patch-new/drivers/char/hvc_console.c
@@ -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);

drivers/char/hvcs.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/char/hvcs.c 400-workthreads.patch-new/drivers/char/hvcs.c
@@ -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);

drivers/input/serio/serio.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/input/serio/serio.c 400-workthreads.patch-new/drivers/input/serio/serio.c
@@ -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);

drivers/md/dm-crypt.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/md/dm-crypt.c 400-workthreads.patch-new/drivers/md/dm-crypt.c
@@ -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");

drivers/scsi/hosts.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/scsi/hosts.c 400-workthreads.patch-new/drivers/scsi/hosts.c
@@ -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;

drivers/usb/net/pegasus.c +1 -1

diff -ruNp 400-workthreads.patch-old/drivers/usb/net/pegasus.c 400-workthreads.patch-new/drivers/usb/net/pegasus.c
@@ -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);

include/linux/kthread.h +19 -8

diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h
@@ -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.

include/linux/workqueue.h +6 -3

diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
@@ -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);

kernel/kthread.c +86 -8

132 lines not on this page · raw message

kernel/sched.c +0 -1

diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
@@ -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);

kernel/softirq.c +1 -2

diff -ruNp 400-workthreads.patch-old/kernel/softirq.c 400-workthreads.patch-new/kernel/softirq.c
@@ -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;

kernel/workqueue.c +24 ~~-12~~

diff -ruNp 400-workthreads.patch-old/kernel/workqueue.c 400-workthreads.patch-new/kernel/workqueue.c
@@ -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);
 }

Signature · 4 lines

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

Patrick Mochel

2005-08-06 05:06 UTC #7 lore raw

Quoted 6 lines · 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?

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:

include/linux/kthread.h line 31

Quoted 13 lines

diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h

include/linux/kthread.h +6 -0

@@ -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:

Quoted 6 lines

diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
include/linux/workqueue.h +0 -0
@@ -51,9 +51,12 @@ struct work_struct {
 	} while (0)
 

include/linux/workqueue.h struct work_struct { · line 59

Quoted 10 lines

include/linux/workqueue.h +6 -3

 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:

include/linux/workqueue.h struct work_struct {

Quoted 5 lines

include/linux/workqueue.h +2 -1

 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

Nigel Cunningham

author

2005-08-08 00:46 UTC #8 lore raw

Hi.

Sorry for the slow response. Busy still.

Quoted 10 lines · Patrick Mochel

Quoted 6 lines · 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?
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 :>

Quoted 2 lines

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().

Quoted 38 lines

Concerning the patch specifically:

include/linux/kthread.h line 31

Quoted 13 lines

diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h

include/linux/kthread.h +6 -0

@@ -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:

Quoted 6 lines

diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
include/linux/workqueue.h +0 -0
@@ -51,9 +51,12 @@ struct work_struct {
 	} while (0)
 

include/linux/workqueue.h struct work_struct { · line 59

Quoted 10 lines

include/linux/workqueue.h +6 -3

 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:

Quoted 10 lines

include/linux/workqueue.h struct work_struct {
Quoted 5 lines
include/linux/workqueue.h +2 -1
 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.

Quoted 3 lines

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

Quoted 11 lines

Thanks,
Pat
______________________________________________________________________
_______________________________________________
linux-pm mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/linux-pm

Signature · 4 lines

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

Con Kolivas

2005-08-08 01:27 UTC #9 lore raw

Quoted 13 lines · Nigel Cunningham

Hi.
Sorry for the slow response. Busy still.
Quoted 8 lines · Patrick Mochel
Quoted 6 lines · 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?

Quoted 8 lines

Quoted 1 line
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

Nigel Cunningham

author

2005-08-08 01:38 UTC #10 lore raw

Hi.

Quoted 16 lines · Con Kolivas

Quoted 15 lines · Nigel Cunningham
Hi.
Sorry for the slow response. Busy still.
Quoted 9 lines · Patrick Mochel
Quoted 6 lines · 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?
What are your feelings about this: http://lwn.net/Articles/145417/ ?

Quoted 8 lines

Quoted 6 lines
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

Quoted 2 lines

Cheers,
Con

Signature · 4 lines

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

Nigel Cunningham

author

2005-08-08 12:18 UTC #11 lore raw

In reply to Patrick Mochel

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

Signature · 4 lines

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