Re: [PATCH 2/3] work_on_cpu: Use our own workqueue.

[Rusty Russell]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/