cpu hotplug: simplify and hopefully fix locking

The CPU hotplug locking was quite messy, with a recursive lock to
handle the fact that both the actual up/down sequence wanted to
protect itself from being re-entered, but the callbacks that it
called also tended to want to protect themselves from CPU events.

This splits the lock into two (one to serialize the whole hotplug
sequence, the other to protect against the CPU present bitmaps
changing). The latter still allows recursive usage because some
subsystems (ondemand policy for cpufreq at least) had already gotten
too used to the lax locking, but the locking mistakes are hopefully
now less fundamental, and we now warn about recursive lock usage
when we see it, in the hope that it can be fixed.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 44a11f1..8fb344a 100644 (file)
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -48,7 +48,6 @@ static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
 }
 #endif
-extern int current_in_cpu_hotplug(void);
 
 int cpu_up(unsigned int cpu);
 
@@ -61,10 +60,6 @@ static inline int register_cpu_notifier(struct notifier_block *nb)
 static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
 }
-static inline int current_in_cpu_hotplug(void)
-{
-       return 0;
-}
 
 #endif /* CONFIG_SMP */
 extern struct sysdev_class cpu_sysdev_class;
@@ -73,7 +68,6 @@ extern struct sysdev_class cpu_sysdev_class;
 /* Stop CPUs going up and down. */
 extern void lock_cpu_hotplug(void);
 extern void unlock_cpu_hotplug(void);
-extern int lock_cpu_hotplug_interruptible(void);
 #define hotcpu_notifier(fn, pri) {                             \
        static struct notifier_block fn##_nb =                  \
                { .notifier_call = fn, .priority = pri };       \

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 70fbf2e..f230f9a 100644 (file)
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -16,56 +16,48 @@
 #include <linux/mutex.h>
 
 /* This protects CPUs going up and down... */
-static DEFINE_MUTEX(cpucontrol);
+static DEFINE_MUTEX(cpu_add_remove_lock);
+static DEFINE_MUTEX(cpu_bitmask_lock);
 
 static __cpuinitdata BLOCKING_NOTIFIER_HEAD(cpu_chain);
 
 #ifdef CONFIG_HOTPLUG_CPU
-static struct task_struct *lock_cpu_hotplug_owner;
-static int lock_cpu_hotplug_depth;
 
-static int __lock_cpu_hotplug(int interruptible)
-{
-       int ret = 0;
-
-       if (lock_cpu_hotplug_owner != current) {
-               if (interruptible)
-                       ret = mutex_lock_interruptible(&cpucontrol);
-               else
-                       mutex_lock(&cpucontrol);
-       }
-
-       /*
-        * Set only if we succeed in locking
-        */
-       if (!ret) {
-               lock_cpu_hotplug_depth++;
-               lock_cpu_hotplug_owner = current;
-       }
-
-       return ret;
-}
+/* Crappy recursive lock-takers in cpufreq! Complain loudly about idiots */
+static struct task_struct *recursive;
+static int recursive_depth;
 
 void lock_cpu_hotplug(void)
 {
-       __lock_cpu_hotplug(0);
+       struct task_struct *tsk = current;
+
+       if (tsk == recursive) {
+               static int warnings = 10;
+               if (warnings) {
+                       printk(KERN_ERR "Lukewarm IQ detected in hotplug locking\n");
+                       WARN_ON(1);
+                       warnings--;
+               }
+               recursive_depth++;
+               return;
+       }
+       mutex_lock(&cpu_bitmask_lock);
+       recursive = tsk;
 }
 EXPORT_SYMBOL_GPL(lock_cpu_hotplug);
 
 void unlock_cpu_hotplug(void)
 {
-       if (--lock_cpu_hotplug_depth == 0) {
-               lock_cpu_hotplug_owner = NULL;
-               mutex_unlock(&cpucontrol);
+       WARN_ON(recursive != current);
+       if (recursive_depth) {
+               recursive_depth--;
+               return;
        }
+       mutex_unlock(&cpu_bitmask_lock);
+       recursive = NULL;
 }
 EXPORT_SYMBOL_GPL(unlock_cpu_hotplug);
 
-int lock_cpu_hotplug_interruptible(void)
-{
-       return __lock_cpu_hotplug(1);
-}
-EXPORT_SYMBOL_GPL(lock_cpu_hotplug_interruptible);
 #endif /* CONFIG_HOTPLUG_CPU */
 
 /* Need to know about CPUs going up/down? */
@@ -122,9 +114,7 @@ int cpu_down(unsigned int cpu)
        struct task_struct *p;
        cpumask_t old_allowed, tmp;
 
-       if ((err = lock_cpu_hotplug_interruptible()) != 0)
-               return err;
-
+       mutex_lock(&cpu_add_remove_lock);
        if (num_online_cpus() == 1) {
                err = -EBUSY;
                goto out;
@@ -150,7 +140,10 @@ int cpu_down(unsigned int cpu)
        cpu_clear(cpu, tmp);
        set_cpus_allowed(current, tmp);
 
+       mutex_lock(&cpu_bitmask_lock);
        p = __stop_machine_run(take_cpu_down, NULL, cpu);
+       mutex_unlock(&cpu_bitmask_lock);
+
        if (IS_ERR(p)) {
                /* CPU didn't die: tell everyone.  Can't complain. */
                if (blocking_notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED,
@@ -187,7 +180,7 @@ out_thread:
 out_allowed:
        set_cpus_allowed(current, old_allowed);
 out:
-       unlock_cpu_hotplug();
+       mutex_unlock(&cpu_add_remove_lock);
        return err;
 }
 #endif /*CONFIG_HOTPLUG_CPU*/
@@ -197,9 +190,7 @@ int __devinit cpu_up(unsigned int cpu)
        int ret;
        void *hcpu = (void *)(long)cpu;
 
-       if ((ret = lock_cpu_hotplug_interruptible()) != 0)
-               return ret;
-
+       mutex_lock(&cpu_add_remove_lock);
        if (cpu_online(cpu) || !cpu_present(cpu)) {
                ret = -EINVAL;
                goto out;
@@ -214,7 +205,9 @@ int __devinit cpu_up(unsigned int cpu)
        }
 
        /* Arch-specific enabling code. */
+       mutex_lock(&cpu_bitmask_lock);
        ret = __cpu_up(cpu);
+       mutex_unlock(&cpu_bitmask_lock);
        if (ret != 0)
                goto out_notify;
        BUG_ON(!cpu_online(cpu));
@@ -227,6 +220,6 @@ out_notify:
                blocking_notifier_call_chain(&cpu_chain,
                                CPU_UP_CANCELED, hcpu);
 out:
-       unlock_cpu_hotplug();
+       mutex_unlock(&cpu_add_remove_lock);
        return ret;
 }