Skip to contentSkip to conversation

Fxi hardcoded cpu=0 in workqueue for per_cpu_ptr() calls

PATCH In review Ben Collins 2 participants 2 messages Nov 24, 2005 – Nov 25, 2005

Subsystems (2)
Workqueue Per-CPU memory allocator
lore
Share & tools
lore

Ben Collins

author
#0 lore raw

PATCH 1/1 Fxi hardcoded cpu=0 in workqueue for per_cpu_ptr() calls

Tracked this down on an Ultra Enterprise 3000. It's a 6-way machine. Odd thing about this machine (and it's good for finding bugs like this) is that the CPU id's are not 0 based. For instance, on my machine the CPU's are 6/7/10/11/14/15.

This caused some NULL pointer dereference in kernel/workqueue.c because for single_threaded workqueue's, it hardcoded the cpu to 0.

I changed the 0's to any_online_cpu(cpu_online_mask), which cpumask.h claims is "First cpu in mask". So this fits the same usage.

include/linux/percpu.h +0 -0
kernel/workqueue.c +6 -6
@@ -102,7 +102,7 @@ int fastcall queue_work(struct workqueue
 
 	if (!test_and_set_bit(0, &work->pending)) {
 		if (unlikely(is_single_threaded(wq)))
-			cpu = 0;
+			cpu = any_online_cpu(cpu_online_map);
 		BUG_ON(!list_empty(&work->entry));
 		__queue_work(per_cpu_ptr(wq->cpu_wq, cpu), work);
 		ret = 1;

@@ -118,7 +118,7 @@ static void delayed_work_timer_fn(unsign
 	int cpu = smp_processor_id();
 
 	if (unlikely(is_single_threaded(wq)))
-		cpu = 0;
+		cpu = any_online_cpu(cpu_online_map);
 
 	__queue_work(per_cpu_ptr(wq->cpu_wq, cpu), work);
 }

@@ -266,8 +266,8 @@ void fastcall flush_workqueue(struct wor
 	might_sleep();
 
 	if (is_single_threaded(wq)) {
-		/* Always use cpu 0's area. */
-		flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, 0));
+		/* Always use first cpu's area. */
+		flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, any_online_cpu(cpu_online_map)));
 	} else {
 		int cpu;
 

@@ -325,7 +325,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, any_online_cpu(cpu_online_map));
 		if (!p)
 			destroy = 1;
 		else

@@ -379,7 +379,7 @@ void destroy_workqueue(struct workqueue_
 	/* We don't need the distraction of CPUs appearing and vanishing. */
 	lock_cpu_hotplug();
 	if (is_single_threaded(wq))
-		cleanup_workqueue_thread(wq, 0);
+		cleanup_workqueue_thread(wq, any_online_cpu(cpu_online_map));
 	else {
 		for_each_online_cpu(cpu)
 			cleanup_workqueue_thread(wq, cpu);
Signature · 4 lines
Ubuntu     - http://www.ubuntu.com/
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
SwissDisk  - http://www.swissdisk.com/
#1 lore raw
Quoted 4 lines · Ben Collins

Tracked this down on an Ultra Enterprise 3000. It's a 6-way machine. Odd thing about this machine (and it's good for finding bugs like this) is that the CPU id's are not 0 based. For instance, on my machine the CPU's are 6/7/10/11/14/15.

Good catch.

Quoted 2 lines

I changed the 0's to any_online_cpu(cpu_online_mask), which cpumask.h claims is "First cpu in mask". So this fits the same usage.

any_online_cpu(cpu_online_mask) is a bit of a tautology. How about first_cpu(cpu_online_mask)? In fact, I'd prefer a new macro altogether which you can then define to zero on UP.

This patch raises an interesting question actually. What happens when the CPU that's designated to run single-threaded wq's gets removed?

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt