Skip to contentSkip to conversation

make schedule_on_each_cpu() look like on_each_cpu()

PATCH In review Peter Zijlstra 2 participants 4 messages Jul 26, 2007

Subsystems (2)
Memory management Workqueue
lore
Share & tools
lore

Peter Zijlstra

author
#0 lore raw

PATCH 1/1 make schedule_on_each_cpu() look like on_each_cpu()

It always bothered me a bit that on_each_cpu() and schedule_on_each_cpu() had wildly different interfaces.

Rectify this and convert the sole in-kernel user to the new interface.

Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Ingo Molnar <[email protected]>
---
 include/linux/workqueue.h |    2 -
 kernel/workqueue.c        |   63 ++++++++++++++++++++++++++++++++++++++--------
 mm/swap.c                 |    4 +-
 3 files changed, 56 insertions(+), 13 deletions(-)
include/linux/workqueue.h +1 -1
Index: linux-2.6/include/linux/workqueue.h
===================================================================
@@ -141,7 +141,7 @@ extern int FASTCALL(schedule_delayed_wor
 					unsigned long delay));
 extern int schedule_delayed_work_on(int cpu, struct delayed_work *work,
 					unsigned long delay);
-extern int schedule_on_each_cpu(work_func_t func);
+extern int schedule_on_each_cpu(void (*func)(void *info), void *info, int retry, int wait);
 extern int current_is_keventd(void);
 extern int keventd_up(void);
kernel/workqueue.c +53 -10

89 lines not on this page · raw message

mm/swap.c +2 -2
Index: linux-2.6/mm/swap.c
===================================================================
@@ -216,7 +216,7 @@ void lru_add_drain(void)
 }
 
 #ifdef CONFIG_NUMA
-static void lru_add_drain_per_cpu(struct work_struct *dummy)
+static void lru_add_drain_per_cpu(void *info)
 {
 	lru_add_drain();
 }

@@ -226,7 +226,7 @@ static void lru_add_drain_per_cpu(struct
  */
 int lru_add_drain_all(void)
 {
-	return schedule_on_each_cpu(lru_add_drain_per_cpu);
+	return schedule_on_each_cpu(lru_add_drain_per_cpu, NULL, 0, 1);
 }
 
 #else
#1 lore raw

kernel/workqueue.c EXPORT_SYMBOL(schedule_delayed_work_on); · line 600

Quoted 11 lines · Peter Zijlstra
kernel/workqueue.c +4 -3
-int schedule_on_each_cpu(work_func_t func)
+int schedule_on_each_cpu(void (*func)(void *info), void *info, int retry, int wait)
 {
 	int cpu;
-	struct work_struct *works;
+	struct schedule_on_each_cpu_work **works;
+	int err = 0;
 
-	works = alloc_percpu(struct work_struct);
+	works = kzalloc(sizeof(void *)*nr_cpu_ids, GFP_KERNEL);

Not a comment, but a question: why do we need nr_cpu_ids at all? num_possible_cpus() looks more "correct" if cpu_possible_map has holes (not sure this can happen in practice).

Another off-topic question: shouldn't we kill the unused "retry" parameter of on_each_cpu() instead of mirroring it here?

kernel/workqueue.c EXPORT_SYMBOL(schedule_delayed_work_on); · line 632

Quoted 5 lines
kernel/workqueue.c +5 -0
+out:
+	for_each_possible_cpu(cpu) {
+		if (works[cpu])
+			kfree(works[cpu]);
+	}

Small nit, kfree(NULL) is OK.

Oleg.

#2 lore raw
Quoted 11 lines · Oleg Nesterov
Quoted 9 lines · Peter Zijlstra
kernel/workqueue.c +3 -2
-int schedule_on_each_cpu(work_func_t func)
+int schedule_on_each_cpu(void (*func)(void *info), void *info, int retry, int wait)
 {
 	int cpu;
-	struct work_struct *works;
+	struct schedule_on_each_cpu_work **works;
+	int err = 0;
Quoted 6 lines

kernel/workqueue.c EXPORT_SYMBOL(schedule_delayed_work_on); · line 600

Quoted 2 lines
kernel/workqueue.c +1 -1
-	works = alloc_percpu(struct work_struct);
+	works = kzalloc(sizeof(void *)*nr_cpu_ids, GFP_KERNEL);

Not a comment, but a question: why do we need nr_cpu_ids at all? num_possible_cpus() looks more "correct" if cpu_possible_map has holes (not sure this can happen in practice).

OOPS, I am stupid, please ignore. Of course, we need the highest CPU number, not num_possible_cpus().

Oleg.

author
#3 lore raw
In reply to Oleg Nesterov
Quoted 7 lines · Oleg Nesterov

kernel/workqueue.c EXPORT_SYMBOL(schedule_delayed_work_on); · line 594

Quoted 3 lines · Peter Zijlstra
kernel/workqueue.c +1 -1
-int schedule_on_each_cpu(work_func_t func)
+int schedule_on_each_cpu(void (*func)(void *info), void *info, int retry, int wait)

Another off-topic question: shouldn't we kill the unused "retry" parameter of on_each_cpu() instead of mirroring it here?

I'm fine with doing so, but that will be a somewhat larger patch. If you (and perhaps Andrew?) think that is preferable I'll prepare such a patch.

Quoted 7 lines

kernel/workqueue.c EXPORT_SYMBOL(schedule_delayed_work_on); · line 632

Quoted 5 lines
kernel/workqueue.c +5 -0
+out:
+	for_each_possible_cpu(cpu) {
+		if (works[cpu])
+			kfree(works[cpu]);
+	}

Small nit, kfree(NULL) is OK.

Habbits die hard :-/