workqueue: mark init_workqueues() as early_initcall()

PATCH Superseded Suresh Siddha 3 participants 10 messages Jul 30, 2010 – Aug 4, 2010

Subsystems (2)

x86 Workqueue

lore

Share & tools

lore

Suresh Siddha

author

2010-07-30 22:00 UTC #0 lore raw

PATCH 1/2 workqueue: mark init_workqueues() as early_initcall()

Mark init_workqueues() as early_initcall() and thus it will be initialized before smp bringup. init_workqueues() registers for the hotcpu notifier and thus it should cope with the processors that are brought online after the workqueues are initialized.

x86 smp bringup code uses workqueues and uses a workaround for the cold boot process (as the workqueues are initialized post smp_init()). Marking init_workqueues() as early_initcall() will pave the way for cleaning up this code.

Signed-off-by Suresh Siddha <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/workqueue.h |    1 -
 init/main.c               |    2 --
 kernel/workqueue.c        |    4 +++-
 3 files changed, 3 insertions(+), 4 deletions(-)

init/main.c +0 -2

Index: tip/init/main.c
===================================================================
@@ -32,7 +32,6 @@
 #include <linux/start_kernel.h>
 #include <linux/security.h>
 #include <linux/smp.h>
-#include <linux/workqueue.h>
 #include <linux/profile.h>
 #include <linux/rcupdate.h>
 #include <linux/moduleparam.h>

@@ -788,7 +787,6 @@ static void __init do_initcalls(void)
  */
 static void __init do_basic_setup(void)
 {
-	init_workqueues();
 	cpuset_init_smp();
 	usermodehelper_init();
 	init_tmpfs();

include/linux/workqueue.h +0 -1

Index: tip/include/linux/workqueue.h
===================================================================
@@ -234,7 +234,6 @@ extern int schedule_on_each_cpu(work_fun
 extern int current_is_keventd(void);
 extern int keventd_up(void);
 
-extern void init_workqueues(void);
 int execute_in_process_context(work_func_t fn, struct execute_work *);
 
 extern int flush_work(struct work_struct *work);

kernel/workqueue.c +3 -1

Index: tip/kernel/workqueue.c
===================================================================
@@ -1216,7 +1216,7 @@ long work_on_cpu(unsigned int cpu, long 
 EXPORT_SYMBOL_GPL(work_on_cpu);
 #endif /* CONFIG_SMP */
 
-void __init init_workqueues(void)
+static int __init init_workqueues(void)
 {
 	alloc_cpumask_var(&cpu_populated_map, GFP_KERNEL);

@@ -1226,4 +1226,6 @@ void __init init_workqueues(void)
 	hotcpu_notifier(workqueue_cpu_callback, 0);
 	keventd_wq = create_workqueue("events");
 	BUG_ON(!keventd_wq);
+	return 0;
 }
+early_initcall(init_workqueues);

Suresh Siddha

author

2010-07-30 22:00 UTC #1 lore raw

PATCH 2/2 x86, smp: use workqueues unconditionally during do_boot_cpu()

Workqueues are now initialized as part of the early_initcall(). So they are available for use during cold boot process aswell.

And also ACPI hotplug notifier no longer calls do_boot_cpu() in the keventd_wq context. So we can unconditionally use workqueues in do_boot_cpu()

Signed-off-by Suresh Siddha <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Zhang Rui <[email protected]>

---
 arch/x86/kernel/smpboot.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

arch/x86/kernel/smpboot.c +2 -6

Index: tip/arch/x86/kernel/smpboot.c
===================================================================
@@ -735,12 +735,8 @@ static int __cpuinit do_boot_cpu(int api
 		goto do_rest;
 	}
 
-	if (!keventd_up() || current_is_keventd())
-		c_idle.work.func(&c_idle.work);
-	else {
-		schedule_work(&c_idle.work);
-		wait_for_completion(&c_idle.done);
-	}
+	schedule_work(&c_idle.work);
+	wait_for_completion(&c_idle.done);
 
 	if (IS_ERR(c_idle.idle)) {
 		printk("failed fork for CPU %d\n", cpu);

Andrew Morton

2010-07-30 23:55 UTC #2 lore raw

In reply to Suresh Siddha

Quoted 10 lines · On Fri, 30 Jul 2010 14:57:37 -0700 Suresh Siddha <[email protected]>…

Mark init_workqueues() as early_initcall() and thus it will be initialized before smp bringup. init_workqueues() registers for the hotcpu notifier and thus it should cope with the processors that are brought online after the workqueues are initialized.
x86 smp bringup code uses workqueues and uses a workaround for the cold boot process (as the workqueues are initialized post smp_init()). Marking init_workqueues() as early_initcall() will pave the way for cleaning up this code.

I sure hope this has been tested against linux-next. kernel/workqueue.c has been vastly changed and -tip doesn't know about that. linux-next should include -tip and is hence a better tree to develop and test against.

AFAICT the main thing which needs checking is that the new init_workqueues() doesn't do anything which requires that sched_init_smp() has been executed.

The patch otherwise looks OK and killing that hack in the x86 code was most merciful.

for_each_gcwq_cpu(), for_each_online_gcwq_cpu() and for_each_cwq_cpu() make me cry.

Suresh Siddha

author

2010-07-31 00:48 UTC #3 lore raw

Quoted 23 lines · Andrew Morton

Quoted 10 lines · On Fri, 30 Jul 2010 14:57:37 -0700 Suresh Siddha <[email protected]>…
Mark init_workqueues() as early_initcall() and thus it will be initialized before smp bringup. init_workqueues() registers for the hotcpu notifier and thus it should cope with the processors that are brought online after the workqueues are initialized.
x86 smp bringup code uses workqueues and uses a workaround for the cold boot process (as the workqueues are initialized post smp_init()). Marking init_workqueues() as early_initcall() will pave the way for cleaning up this code.
I sure hope this has been tested against linux-next. kernel/workqueue.c has been vastly changed and -tip doesn't know about that. linux-next should include -tip and is hence a better tree to develop and test against.
AFAICT the main thing which needs checking is that the new init_workqueues() doesn't do anything which requires that sched_init_smp() has been executed.

Quoted 5 lines

The patch otherwise looks OK and killing that hack in the x86 code was most merciful.
for_each_gcwq_cpu(), for_each_online_gcwq_cpu() and for_each_cwq_cpu() make me cry.

hmm.. too many changes in linux-next.

Yes, as far as these patches are concerned, they work against linux-next aswell. Just posted -v2 which is on top of linux-next. ia64 also had the same issue, addressed in -v2 aswell.

thanks,
suresh

Tejun Heo

maintainer

2010-07-31 10:29 UTC #4 lore raw

Hello,

Quoted 5 lines · Suresh Siddha

hmm.. too many changes in linux-next.
Yes, as far as these patches are concerned, they work against linux-next aswell. Just posted -v2 which is on top of linux-next. ia64 also had the same issue, addressed in -v2 aswell.

I think it'll be best to route the changes through workqueue tree. I'll audit init path and if everything looks okay will add the workqueue patch to wq#for-next.

Thank you.

Signature · 1 line

tejun

Tejun Heo

maintainer

2010-07-31 10:28 UTC #5 lore raw

In reply to Andrew Morton

Hello,

Quoted 3 lines · Andrew Morton

On Fri, 30 Jul 2010 14:57:37 -0700
for_each_gcwq_cpu(), for_each_online_gcwq_cpu() and for_each_cwq_cpu() make me cry.

Yeah, those are new additions for unbound workqueue support and I should have added comments there. Will do so.

Thanks.

Signature · 1 line

tejun

Tejun Heo

maintainer

2010-08-01 09:54 UTC #6 lore raw

PATCH 1/1 workqueue: explain for_each_*cwq_cpu() iterators

In reply to Andrew Morton

for_each_*cwq_cpu() are similar to regular CPU iterators except that it also considers the pseudo CPU number used for unbound workqueues. Explain them.

Signed-off-by Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

---
 kernel/workqueue.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

kernel/workqueue.c +13 -0

index e5cb7fa..1105c47 100644
@@ -271,6 +271,19 @@ static inline int __next_wq_cpu(int cpu, const struct cpumask *mask,
 	return __next_gcwq_cpu(cpu, mask, !(wq->flags & WQ_UNBOUND) ? 1 : 2);
 }
 
+/*
+ * CPU iterators
+ *
+ * An extra gcwq is defined for an invalid cpu number
+ * (WORK_CPU_UNBOUND) to host workqueues which are not bound to any
+ * specific CPU.  The following iterators are similar to
+ * for_each_*_cpu() iterators but also considers the unbound gcwq.
+ *
+ * for_each_gcwq_cpu()		: possible CPUs + WORK_CPU_UNBOUND
+ * for_each_online_gcwq_cpu()	: online CPUs + WORK_CPU_UNBOUND
+ * for_each_cwq_cpu()		: possible CPUs for bound workqueues,
+ *				  WORK_CPU_UNBOUND for unbound workqueues
+ */
 #define for_each_gcwq_cpu(cpu)						\
 	for ((cpu) = __next_gcwq_cpu(-1, cpu_possible_mask, 3);		\
 	     (cpu) < WORK_CPU_NONE;					\

Signature · 1 line

1.7.1

Tejun Heo

maintainer

2010-08-01 11:07 UTC #7 lore raw

PATCH 1/1 workqueue: mark init_workqueues() as early_initcall()

In reply to Suresh Siddha

Quoted 1 line

From 6ee0578b4daaea01c96b172c6aacca43fd9807a6 Mon Sep 17 00:00:00 2001

From: Suresh Siddha <[email protected]>
Date: Fri, 30 Jul 2010 14:57:37 -0700

Signed-off-by Suresh Siddha <[email protected]>

Signed-off-by Tejun Heo <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andrew Morton <[email protected]>

---
Audited and tested. It should be fine. Patch applied and pushed out to linux-next. How shall we route the second patch?

Thanks.

 include/linux/workqueue.h |    1 -
 init/main.c               |    2 --
 kernel/workqueue.c        |    4 +++-
 3 files changed, 3 insertions(+), 4 deletions(-)

include/linux/workqueue.h +0 -1

index 5f76001..51dc9a7 100644
@@ -327,7 +327,6 @@ extern int schedule_delayed_work_on(int cpu, struct delayed_work *work,
 extern int schedule_on_each_cpu(work_func_t func);
 extern int keventd_up(void);
 
-extern void init_workqueues(void);
 int execute_in_process_context(work_func_t fn, struct execute_work *);
 
 extern int flush_work(struct work_struct *work);

init/main.c +0 -2

index 3bdb152..5f2ec2c 100644
@@ -32,7 +32,6 @@
 #include <linux/start_kernel.h>
 #include <linux/security.h>
 #include <linux/smp.h>
-#include <linux/workqueue.h>
 #include <linux/profile.h>
 #include <linux/rcupdate.h>
 #include <linux/moduleparam.h>

@@ -786,7 +785,6 @@ static void __init do_initcalls(void)
  */
 static void __init do_basic_setup(void)
 {
-	init_workqueues();
 	cpuset_init_smp();
 	usermodehelper_init();
 	init_tmpfs();

kernel/workqueue.c +3 -1

index 1105c47..e2eb351 100644
@@ -3507,7 +3507,7 @@ out_unlock:
 }
 #endif /* CONFIG_FREEZER */
 
-void __init init_workqueues(void)
+static int __init init_workqueues(void)
 {
 	unsigned int cpu;
 	int i;

@@ -3559,4 +3559,6 @@ void __init init_workqueues(void)
 	system_unbound_wq = alloc_workqueue("events_unbound", WQ_UNBOUND,
 					    WQ_UNBOUND_MAX_ACTIVE);
 	BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq);
+	return 0;
 }
+early_initcall(init_workqueues);

Signature · 1 line

1.7.1

Suresh Siddha

author

2010-08-02 18:41 UTC #8 lore raw

Quoted 3 lines · Tejun Heo

Audited and tested. It should be fine. Patch applied and pushed out to linux-next. How shall we route the second patch?

Tejun, v2 version of the second patch had both x86 and ia64 changes.

http://marc.info/?l=linux-kernel&m=128053714303108&w=2

So I am ok if you carry this with the rest of your workqueue changes that touch these same files.

thanks,
suresh

Tejun Heo

maintainer

2010-08-04 13:43 UTC #9 lore raw

Quoted 11 lines · Suresh Siddha

Quoted 3 lines · Tejun Heo
Audited and tested. It should be fine. Patch applied and pushed out to linux-next. How shall we route the second patch?
Tejun, v2 version of the second patch had both x86 and ia64 changes.
http://marc.info/?l=linux-kernel&m=128053714303108&w=2
So I am ok if you carry this with the rest of your workqueue changes that touch these same files.

Andrew seemed to have already picked up the patch. I've sent the pull request with the first patch.

Thank you.

Signature · 1 line

tejun