lkml.org 
[lkml]   [2006]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare
From
Date
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/

\
 
 \ /
  Last update: 2006-07-26 21:45    [W:0.456 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site