[PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[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).

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 <linux@dominikbrodowski.de>

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

diff -ruN linux-original/include/linux/cpu.h linux/include/linux/cpu.h
--- linux-original/include/linux/cpu.h	2004-10-29 17:16:59.000000000 +0200
+++ linux/include/linux/cpu.h	2004-11-01 08:57:07.000000000 +0100
@@ -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 */
diff -ruN linux-original/kernel/cpu.c linux/kernel/cpu.c
--- linux-original/kernel/cpu.c	2004-10-29 17:17:11.000000000 +0200
+++ linux/kernel/cpu.c	2004-11-01 09:00:22.000000000 +0100
@@ -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;
 }

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Nick Piggin]

Dominik Brodowski wrote:
> [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.
> 

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

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Zwane Mwaikambo]

On Mon, 1 Nov 2004, Dominik Brodowski wrote:

> [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 <linux@dominikbrodowski.de>

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

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Lee Revell]

On Mon, 2004-11-01 at 07:00 -0700, Zwane Mwaikambo wrote:
> 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

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Rusty Russell]

On Mon, 2004-11-01 at 13:04 -0500, Lee Revell wrote:
> On Mon, 2004-11-01 at 07:00 -0700, Zwane Mwaikambo wrote:
> > 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.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Zwane Mwaikambo]

On Mon, 1 Nov 2004, Lee Revell wrote:

> On Mon, 2004-11-01 at 07:00 -0700, Zwane Mwaikambo wrote:
> > 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

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Lee Revell]

On Tue, 2004-11-02 at 10:48 +1100, Rusty Russell wrote:
> On Mon, 2004-11-01 at 13:04 -0500, Lee Revell wrote:
> > On Mon, 2004-11-01 at 07:00 -0700, Zwane Mwaikambo wrote:
> > > 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().

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

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Dominik Brodowski]

On Mon, Nov 01, 2004 at 07:00:07AM -0700, Zwane Mwaikambo wrote:
> On Mon, 1 Nov 2004, Dominik Brodowski wrote:
> 
> > [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 <linux@dominikbrodowski.de>
> 
> 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.

Re: [PATCH] [CPU-HOTPLUG] convert cpucontrol to be a rwsem

[Rusty Russell]

On Tue, 2004-11-02 at 23:28 +0100, Dominik Brodowski wrote:
> 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.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman