On Saturday 31 January 2009 08:47:44 Andrew Morton wrote:
> Just as an example, take a look at allocate_threshold_blocks(). That
> function way down in the innards of x86 has blotted out large amounts of
> kernel code, so that code can now not use work_on_cpu(). Anything which
> happens inside ext3 commit (the entire block layer and all drivers
> underneath it). Large lumps of networking code. Parts of the page
> allocator and the VFS which I haven't started to think about yet.
Yes, you're right. Any infrastructure which does callouts holding a lock
has the same problem. We have several of those, as I pointed out, but the
problem comes when invoking any two; more likely when they're general.
And I did so much under work_on_cpu here (Mike is credited, but it looks like
my work) precisely because I have no idea what this code is doing, so chose
the simplest conversion.
AFAICT, it just wants to rdmsr on a particular CPU. rdmsr_on_cpu() is pretty
easy to implement which would fix *this* case (and IIRC, would be useful
elsewhere)
If we want a general work_on_cpu(), we need this:
Subject: work_on_cpu: __work_on_cpu and singlethread work_on_cpu
Andrew Morton points out two problems with the current work_on_cpu
implementation. Firstly, it adds a thread per cpu, which is wasteful.
Secondly, by holding a lock across a generic callback, it creates more
potential for lock inversion: any lock grabbed by the callback is a
lock which another unrelated caller to work_on_cpu() can't hold.
(A similar issue has plagued kevent).
This patch does two things: firstly, it changes to a singlethread
workqueue which simply moves itself to the appropriate CPU. Secondly,
it adds an __work_on_cpu() for callbacks which need to grab locks:
this allows them to use their own independent workqueue.
Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
---
include/linux/workqueue.h | 7 +++++
kernel/workqueue.c | 59 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 56 insertions(+), 10 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -257,7 +257,14 @@ static inline long work_on_cpu(unsigned
{
return fn(arg);
}
+static inline long __work_on_cpu(struct workqueue_struct *swq,
+ unsigned cpu, long (*fn)(void *), void *arg)
+{
+ return fn(arg);
+}
#else
+long __work_on_cpu(struct workqueue_struct *swq,
+ unsigned int cpu, long (*fn)(void *), void *arg);
long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg);
#endif /* CONFIG_SMP */
#endif
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -977,6 +977,7 @@ struct work_for_cpu {
struct work_struct work;
long (*fn)(void *);
void *arg;
+ unsigned int cpu;
long ret;
};
@@ -984,8 +985,48 @@ static void do_work_for_cpu(struct work_
{
struct work_for_cpu *wfc = container_of(w, struct work_for_cpu, work);
+ if (set_cpus_allowed_ptr(current, cpumask_of(wfc->cpu)) != 0)
+ WARN(1, "work_on_cpu on offline cpu %u?\n", wfc->cpu);
wfc->ret = wfc->fn(wfc->arg);
}
+
+
+/**
+ * __work_on_cpu - run a function in a workqueue on a particular cpu
+ * @swq: the (singlethreaded) workqueue
+ * @cpu: the cpu to run on
+ * @fn: the function to run
+ * @arg: the function arg
+ *
+ * This will return the value @fn returns.
+ * It is up to the caller to ensure that the cpu doesn't go offline.
+ *
+ * Example:
+ * int ret;
+ * struct workqueue_struct *wq = create_singlethread_workqueue("myq");
+ * if (unlikely(!wq))
+ * ret = -ENOMEM;
+ * else {
+ * ret = __work_on_cpu(wq, cpu, fn, arg);
+ * destroy_workqueue(wq);
+ * }
+ */
+long __work_on_cpu(struct workqueue_struct *swq,
+ unsigned int cpu, long (*fn)(void *), void *arg)
+{
+ struct work_for_cpu wfc;
+
+ INIT_WORK(&wfc.work, do_work_for_cpu);
+ wfc.fn = fn;
+ wfc.arg = arg;
+ wfc.cpu = cpu;
+ BUG_ON(!swq->singlethread);
+ queue_work(swq, &wfc.work);
+ flush_work(&wfc.work);
+
+ return wfc.ret;
+}
+EXPORT_SYMBOL_GPL(__work_on_cpu);
/**
* work_on_cpu - run a function in user context on a particular cpu
@@ -995,18 +1036,16 @@ static void do_work_for_cpu(struct work_
*
* This will return the value @fn returns.
* It is up to the caller to ensure that the cpu doesn't go offline.
+ *
+ * @fn is called with a lock held (the work_on_cpu workqueue's lock):
+ * if it grabs any externally-visible locks, you might get a locking
+ * inversion against others who grab those locks then call
+ * work_on_cpu(). You can use your own private workqueue to avoid
+ * this.
*/
long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
{
- struct work_for_cpu wfc;
-
- INIT_WORK(&wfc.work, do_work_for_cpu);
- wfc.fn = fn;
- wfc.arg = arg;
- queue_work_on(cpu, work_on_cpu_wq, &wfc.work);
- flush_work(&wfc.work);
-
- return wfc.ret;
+ return __work_on_cpu(work_on_cpu_wq, cpu, fn, arg);
}
EXPORT_SYMBOL_GPL(work_on_cpu);
#endif /* CONFIG_SMP */
@@ -1022,7 +1061,7 @@ void __init init_workqueues(void)
keventd_wq = create_workqueue("events");
BUG_ON(!keventd_wq);
#ifdef CONFIG_SMP
- work_on_cpu_wq = create_workqueue("work_on_cpu");
+ work_on_cpu_wq = create_singlethread_workqueue("work_on_cpu");
BUG_ON(!work_on_cpu_wq);
#endif
}
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
