Messages in this thread Patch in this message | | | Subject | Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare | From | Arjan van de Ven <> | Date | Wed, 26 Jul 2006 21:42:34 +0200 |
| |
On Wed, 2006-07-26 at 10:09 -0700, Linus Torvalds wrote:
> It looked sensible to me too, although it still shows some "Lukewarm IQ" > notices for the ondemand driver: > > Lukewarm IQ detected in hotplug locking > BUG: warning at kernel/cpu.c:38/lock_cpu_hotplug() > [<c0103d07>] show_trace+0xd/0x10 > [<c01042ec>] dump_stack+0x19/0x1b > [<c013778d>] lock_cpu_hotplug+0x43/0x69 > [<c012f9df>] __create_workqueue+0x52/0x11f > [<df0ec34b>] cpufreq_governor_dbs+0x9f/0x2bd [cpufreq_ondemand]
ok so this is messy. ondemand wants to create a workqueue, but this function is called (after my patch) with the lock_cpu_hotplug() already held, and all the workqueue functions take the lock_cpu_hotplug themselves (looking at that code I'm not 100% convinced it's quite right how it does that, but that's a separate matter). Before my first patch it wasn't doing that, but that situation was even nastier; there is a lock that is taken in the caller, and the lock_cpu_hotplug() lock *really* wants to nest outside that lock ;(
As a quick hack I made non-lock_cpu_hotplug()'ing versions of the 3 key workqueue functions (patch below). It works, it's correct, it's just so ugly that I'm almost too ashamed to post it. I haven't found a better solution yet though... time to take a step back I suppose.
Index: linux-2.6.18-rc2-git5/include/linux/workqueue.h =================================================================== --- linux-2.6.18-rc2-git5.orig/include/linux/workqueue.h +++ linux-2.6.18-rc2-git5/include/linux/workqueue.h @@ -55,17 +55,20 @@ struct execute_work { } 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, int locked); +#define create_workqueue(name) __create_workqueue((name), 0, 0) +#define create_workqueue_cpulocked(name) __create_workqueue((name), 0, 1) +#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0) extern void destroy_workqueue(struct workqueue_struct *wq); +extern void destroy_workqueue_cpulock(struct workqueue_struct *wq); extern int FASTCALL(queue_work(struct workqueue_struct *wq, struct work_struct *work)); extern int FASTCALL(queue_delayed_work(struct workqueue_struct *wq, struct work_struct *work, unsigned long delay)); extern int queue_delayed_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work, unsigned long delay); extern void FASTCALL(flush_workqueue(struct workqueue_struct *wq)); +extern void FASTCALL(__flush_workqueue(struct workqueue_struct *wq)); extern int FASTCALL(schedule_work(struct work_struct *work)); extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay)); Index: linux-2.6.18-rc2-git5/kernel/workqueue.c =================================================================== --- linux-2.6.18-rc2-git5.orig/kernel/workqueue.c +++ linux-2.6.18-rc2-git5/kernel/workqueue.c @@ -307,6 +307,22 @@ void fastcall flush_workqueue(struct wor } EXPORT_SYMBOL_GPL(flush_workqueue); +void fastcall __flush_workqueue(struct workqueue_struct *wq) +{ + might_sleep(); + + if (is_single_threaded(wq)) { + /* Always use first cpu's area. */ + flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, singlethread_cpu)); + } else { + int cpu; + + for_each_online_cpu(cpu) + flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu)); + } +} +EXPORT_SYMBOL_GPL(__flush_workqueue); + static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq, int cpu) { @@ -333,7 +349,7 @@ static struct task_struct *create_workqu } struct workqueue_struct *__create_workqueue(const char *name, - int singlethread) + int singlethread, int locked) { int cpu, destroy = 0; struct workqueue_struct *wq; @@ -351,7 +367,8 @@ struct workqueue_struct *__create_workqu wq->name = name; /* We don't need the distraction of CPUs appearing and vanishing. */ - lock_cpu_hotplug(); + if (!locked) + lock_cpu_hotplug(); if (singlethread) { INIT_LIST_HEAD(&wq->list); p = create_workqueue_thread(wq, singlethread_cpu); @@ -372,7 +389,8 @@ struct workqueue_struct *__create_workqu destroy = 1; } } - unlock_cpu_hotplug(); + if (!locked) + unlock_cpu_hotplug(); /* * Was there any error during startup? If yes then clean up: @@ -400,14 +418,17 @@ static void cleanup_workqueue_thread(str kthread_stop(p); } -void destroy_workqueue(struct workqueue_struct *wq) +static void __destroy_workqueue(struct workqueue_struct *wq, int locked) { int cpu; - flush_workqueue(wq); /* We don't need the distraction of CPUs appearing and vanishing. */ - lock_cpu_hotplug(); + if (!locked) + lock_cpu_hotplug(); + + __flush_workqueue(wq); + if (is_single_threaded(wq)) cleanup_workqueue_thread(wq, singlethread_cpu); else { @@ -417,11 +438,22 @@ void destroy_workqueue(struct workqueue_ list_del(&wq->list); spin_unlock(&workqueue_lock); } - unlock_cpu_hotplug(); + if (!locked) + unlock_cpu_hotplug(); free_percpu(wq->cpu_wq); kfree(wq); } + +void destroy_workqueue(struct workqueue_struct *wq) +{ + __destroy_workqueue(wq, 0); +} EXPORT_SYMBOL_GPL(destroy_workqueue); +void destroy_workqueue_cpulock(struct workqueue_struct *wq) +{ + __destroy_workqueue(wq, 1); +} +EXPORT_SYMBOL_GPL(destroy_workqueue_cpulock); static struct workqueue_struct *keventd_wq; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |