convert cpucontrol to be a rwsem

PATCH In review Dominik Brodowski 5 participants 9 messages Nov 1, 2004 – Nov 4, 2004 CPU hotplug

lore

Share & tools

lore

Dominik Brodowski

author

2004-11-01 08:44 UTC #0 lore raw

PATCH 1/1 convert cpucontrol to be a rwsem

[CPU-HOTPLUG] Use a rw-semaphore for serializing and locking

Currently, lock_cpu_hotplug serializes multiple calls to cpufreq->target() on multiple CPUs even though that's unneccessary. Even further, it serializes these calls with totally unrelated other parts of the kernel... some ppc64 event reporting, some cache management, and so on. In my opinion locking should be done subsystem (and normally data-)specific, and disabling CPU hotplug should just do that.

This patch converts the semaphore cpucontrol to be a rwsem which allows us to use it for _both_ variants: locking (write) and (multiple) other parts disabling CPU hotplug (read).

Only problem I see with this approach is that lock_cpu_hotplug_interruptible() needs to disappear as there is no down_write_interruptible() for rw-semaphores.

Signed-off-by Dominik Brodowski <[email protected]>

 include/linux/cpu.h |   19 ++++++++++++++-----
 kernel/cpu.c        |   19 ++++++++-----------
 2 files changed, 22 insertions(+), 16 deletions(-)

include/linux/cpu.h +14 -5

diff -ruN linux-original/include/linux/cpu.h linux/include/linux/cpu.h
@@ -59,10 +59,18 @@
 
 #ifdef CONFIG_HOTPLUG_CPU
 /* Stop CPUs going up and down. */
-extern struct semaphore cpucontrol;
-#define lock_cpu_hotplug()	down(&cpucontrol)
-#define unlock_cpu_hotplug()	up(&cpucontrol)
-#define lock_cpu_hotplug_interruptible() down_interruptible(&cpucontrol)
+extern struct rw_semaphore cpucontrol;
+/* these just disable CPU hotplug events but don't
+ * serialize following code */
+#define disable_cpu_hotplug()	down_read(&cpucontrol)
+#define enable_cpu_hotplug()	up_read(&cpucontrol)
+
+/* these disable CPU hotplug events _and_ serialize
+ * any following code.
+ */
+#define lock_cpu_hotplug()	down_write(&cpucontrol)
+#define unlock_cpu_hotplug()	up_write(&cpucontrol)
+
 #define hotcpu_notifier(fn, pri) {				\
 	static struct notifier_block fn##_nb =			\
 		{ .notifier_call = fn, .priority = pri };	\

@@ -71,9 +79,10 @@
 int cpu_down(unsigned int cpu);
 #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
 #else
+#define disable_cpu_hotplug()	do { } while (0)
+#define enable_cpu_hotplug()	do { } while (0)
 #define lock_cpu_hotplug()	do { } while (0)
 #define unlock_cpu_hotplug()	do { } while (0)
-#define lock_cpu_hotplug_interruptible() 0
 #define hotcpu_notifier(fn, pri)
 
 /* CPUs don't go offline once they're online w/o CONFIG_HOTPLUG_CPU */

kernel/cpu.c +8 ~~-11~~

diff -ruN linux-original/kernel/cpu.c linux/kernel/cpu.c
@@ -17,7 +17,7 @@
 #include <asm/semaphore.h>
 
 /* This protects CPUs going up and down... */
-DECLARE_MUTEX(cpucontrol);
+DECLARE_RWSEM(cpucontrol);
 
 static struct notifier_block *cpu_chain;

@@ -26,19 +26,18 @@
 {
 	int ret;
 
-	if ((ret = down_interruptible(&cpucontrol)) != 0)
-		return ret;
+	down_write(&cpucontrol);
 	ret = notifier_chain_register(&cpu_chain, nb);
-	up(&cpucontrol);
+	up_write(&cpucontrol);
 	return ret;
 }
 EXPORT_SYMBOL(register_cpu_notifier);
 
 void unregister_cpu_notifier(struct notifier_block *nb)
 {
-	down(&cpucontrol);
+	down_write(&cpucontrol);
 	notifier_chain_unregister(&cpu_chain, nb);
-	up(&cpucontrol);
+	up_write(&cpucontrol);
 }
 EXPORT_SYMBOL(unregister_cpu_notifier);

@@ -81,8 +80,7 @@
 	struct task_struct *p;
 	cpumask_t old_allowed, tmp;
 
-	if ((err = lock_cpu_hotplug_interruptible()) != 0)
-		return err;
+	lock_cpu_hotplug();
 
 	if (num_online_cpus() == 1) {
 		err = -EBUSY;

@@ -156,8 +154,7 @@
 	int ret;
 	void *hcpu = (void *)(long)cpu;
 
-	if ((ret = down_interruptible(&cpucontrol)) != 0)
-		return ret;
+	down_write(&cpucontrol);
 
 	if (cpu_online(cpu) || !cpu_present(cpu)) {
 		ret = -EINVAL;

@@ -185,6 +182,6 @@
 	if (ret != 0)
 		notifier_call_chain(&cpu_chain, CPU_UP_CANCELED, hcpu);
 out:
-	up(&cpucontrol);
+	up_write(&cpucontrol);
 	return ret;
 }

Nick Piggin

2004-11-01 09:05 UTC #1 lore raw

Quoted 9 lines · Dominik Brodowski

[CPU-HOTPLUG] Use a rw-semaphore for serializing and locking
Currently, lock_cpu_hotplug serializes multiple calls to cpufreq->target() on multiple CPUs even though that's unneccessary. Even further, it serializes these calls with totally unrelated other parts of the kernel... some ppc64 event reporting, some cache management, and so on. In my opinion locking should be done subsystem (and normally data-)specific, and disabling CPU hotplug should just do that.

Quoted 5 lines

This patch converts the semaphore cpucontrol to be a rwsem which allows us to use it for _both_ variants: locking (write) and (multiple) other parts disabling CPU hotplug (read).
Only problem I see with this approach is that lock_cpu_hotplug_interruptible() needs to disappear as there is no down_write_interruptible() for rw-semaphores.

I think I've got a patch somewhere to implement _interruptible operations on rwsems.

Zwane Mwaikambo

2004-11-01 16:27 UTC #2 lore raw

In reply to Dominik Brodowski

Quoted 9 lines · Dominik Brodowski

[CPU-HOTPLUG] Use a rw-semaphore for serializing and locking
Currently, lock_cpu_hotplug serializes multiple calls to cpufreq->target() on multiple CPUs even though that's unneccessary. Even further, it serializes these calls with totally unrelated other parts of the kernel... some ppc64 event reporting, some cache management, and so on. In my opinion locking should be done subsystem (and normally data-)specific, and disabling CPU hotplug should just do that.

Quoted 8 lines

This patch converts the semaphore cpucontrol to be a rwsem which allows us to use it for _both_ variants: locking (write) and (multiple) other parts disabling CPU hotplug (read).
Only problem I see with this approach is that lock_cpu_hotplug_interruptible() needs to disappear as there is no down_write_interruptible() for rw-semaphores.
Signed-off-by Dominik Brodowski <[email protected]>

Agreed it makes a lot more sense, i think there could be some places where we use preempt_disable to protect against cpu offline which could converted, but that can come later.

Thanks,
Zwane

Lee Revell

2004-11-01 18:15 UTC #3 lore raw

Quoted 4 lines · Zwane Mwaikambo

Agreed it makes a lot more sense, i think there could be some places where we use preempt_disable to protect against cpu offline which could converted, but that can come later.

You know I picked up Robert Love's book the other day and was surprised to read we are not supposed to be using preempt_disable, there is a per_cpu interface for exactly this kind of thing. Which is currently recommended?

Lee

Zwane Mwaikambo

2004-11-02 00:17 UTC #4 lore raw

Quoted 10 lines · Lee Revell

Quoted 4 lines · Zwane Mwaikambo
Agreed it makes a lot more sense, i think there could be some places where we use preempt_disable to protect against cpu offline which could converted, but that can come later.
You know I picked up Robert Love's book the other day and was surprised to read we are not supposed to be using preempt_disable, there is a per_cpu interface for exactly this kind of thing. Which is currently recommended?

It's on a case by case basis, preempt_disable has the side effect of ensuring that you run through that specific critical section without being interrupted by scheduling, this happens to also block out various things like RCU and the stop_machine (used by cpu hotplug) code amongst others. I'm curious what is the excert that you're referring to?

Thanks,
Zwane

Rusty Russell

2004-11-02 09:41 UTC #5 lore raw

In reply to Lee Revell

Quoted 10 lines · Lee Revell

Quoted 4 lines · Zwane Mwaikambo
Agreed it makes a lot more sense, i think there could be some places where we use preempt_disable to protect against cpu offline which could converted, but that can come later.
You know I picked up Robert Love's book the other day and was surprised to read we are not supposed to be using preempt_disable, there is a per_cpu interface for exactly this kind of thing. Which is currently recommended?

get_cpu() both ensures that this CPU won't go down, and ensures we won't get scheduled off it. It returns the current processor ID, as well. put_cpu() puts the CPU back.

In my experience it's usually clearer than preempt_disable().

Cheers,
Rusty.

Signature · 1 line

A bad analogy is like a leaky screwdriver -- Richard Braakman

Lee Revell

2004-11-02 15:11 UTC #6 lore raw

Quoted 12 lines · Rusty Russell

Quoted 10 lines · Lee Revell
Quoted 4 lines · Zwane Mwaikambo
Agreed it makes a lot more sense, i think there could be some places where we use preempt_disable to protect against cpu offline which could converted, but that can come later.
You know I picked up Robert Love's book the other day and was surprised to read we are not supposed to be using preempt_disable, there is a per_cpu interface for exactly this kind of thing. Which is currently recommended?

Quoted 5 lines

get_cpu() both ensures that this CPU won't go down, and ensures we won't get scheduled off it. It returns the current processor ID, as well. put_cpu() puts the CPU back.
In my experience it's usually clearer than preempt_disable().

To answer Zwane's earlier question, Love's book covers this on page 136, then all of Appendix B. I also completely missed this the first time...

Lee

Dominik Brodowski

author

2004-11-02 22:32 UTC #7 lore raw

In reply to Zwane Mwaikambo

Quoted 15 lines · Zwane Mwaikambo

Quoted 13 lines · Dominik Brodowski
[CPU-HOTPLUG] Use a rw-semaphore for serializing and locking
Currently, lock_cpu_hotplug serializes multiple calls to cpufreq->target() on multiple CPUs even though that's unneccessary. Even further, it serializes these calls with totally unrelated other parts of the kernel... some ppc64 event reporting, some cache management, and so on. In my opinion locking should be done subsystem (and normally data-)specific, and disabling CPU hotplug should just do that.
This patch converts the semaphore cpucontrol to be a rwsem which allows us to use it for _both_ variants: locking (write) and (multiple) other parts disabling CPU hotplug (read).

Quoted 8 lines

Quoted 4 lines
Only problem I see with this approach is that lock_cpu_hotplug_interruptible() needs to disappear as there is no down_write_interruptible() for rw-semaphores.
Signed-off-by Dominik Brodowski <[email protected]>
Agreed it makes a lot more sense, i think there could be some places where we use preempt_disable to protect against cpu offline which could converted, but that can come later.

Except that we don't want to (and can't[*]) disable preemption in the cpufreq case. Therefore, we __need__ to disable CPU hotplug specifically, and not meddle with other issues like preemption, scheduling, CPUs which are in the allowed_map, and so on. So back to the original patch: Rusty, do you agree with it?

Thanks,
Dominik

[*] calls to cpufreq->target() may sleep.

Rusty Russell

2004-11-04 04:55 UTC #8 lore raw

Quoted 5 lines · Dominik Brodowski

Except that we don't want to (and can't[*]) disable preemption in the cpufreq case. Therefore, we __need__ to disable CPU hotplug specifically, and not meddle with other issues like preemption, scheduling, CPUs which are in the allowed_map, and so on. So back to the original patch: Rusty, do you agree with it?

Sure. I consider it a trivial change. The reason it wasn't a rwsem in the first place is that there weren't many places which needed to grab it, and none were time-sensitive.

Thanks!
Rusty.

Signature · 1 line

A bad analogy is like a leaky screwdriver -- Richard Braakman