Re: [PATCH 01/51] CPU hotplug: Provide lockless versions ofcallback registration functions

From: Gautham R Shenoy
Date: Mon Feb 10 2014 - 05:54:05 EST


On Mon, Feb 10, 2014 at 02:45:55PM +0530, Srivatsa S. Bhat wrote:
> Hi Gautham,
>
> On 02/08/2014 12:41 AM, Gautham R Shenoy wrote:
> > On Thu, Feb 06, 2014 at 07:41:03PM +0100, Oleg Nesterov wrote:
> >> On 02/06, Srivatsa S. Bhat wrote:
> >>>
> >>> The following method of CPU hotplug callback registration is not safe
> >>> due to the possibility of an ABBA deadlock involving the cpu_add_remove_lock
> >>> and the cpu_hotplug.lock.
> >>
> >> Off-topic, but perhaps it also makes sense to add the lockdep annotations
> >> later, to catch other similar problems. Currently get_online_cpus() acquires
> >> nothing from lockdep pov.
> >
> > Well, both get/put_online_cpus() as well as cpu_hotplug_begin/end()
> > take the cpu_hotplug.lock mutex. So ideally the lockdep annotations of
> > mutex_lock/unlock() should have worked.
>
> The reason lockdep doesn't catch the lock-inversion (ABBA) deadlock between
> cpu_hotplug.lock (from get_online_cpus) and cpu_add_remove_lock (from
> cpu_maps_update_begin) is because, in the following path, the
> cpu_add_remove_lock is acquired after *releasing* the cpu_hotplug.lock mutex.
>

Right. I get it now!

> get_online_cpus(); // acquire mutex; update counter; release mutex
>
> register_cpu_notifier(); // acquire cpu_add_remove_lock ...
>
> put_online_cpus();
>
> > If it hasn't, then the
> > following lockdep annotations to cpu-hotplug locking should do the
> > trick.
> >
>
> This patch looks good to me. I have a couple of suggestions though..
>

Thanks. I have incorporated the suggestions. Could you check if the
following looks good ?

---
Add lockdep annotations for get/put_online_cpus() and
cpu_hotplug_begin()/cpu_hotplug_end().

Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
Cc: Srivatsa Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Gautham R. Shenoy <ego@xxxxxxxxxxxxxxxxxx>
---
kernel/cpu.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index deff2e6..33caf5e 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -19,6 +19,7 @@
#include <linux/mutex.h>
#include <linux/gfp.h>
#include <linux/suspend.h>
+#include <linux/lockdep.h>

#include "smpboot.h"

@@ -57,17 +58,30 @@ static struct {
* an ongoing cpu hotplug operation.
*/
int refcount;
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ struct lockdep_map dep_map;
+#endif
} cpu_hotplug = {
.active_writer = NULL,
.lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
.refcount = 0,
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ .dep_map = {.name = "cpu_hotplug.lock" },
+#endif
};

+/* Lockdep annotations for get/put_online_cpus() and cpu_hotplug_begin/end() */
+#define cpuhp_lock_acquire_read() lock_map_acquire_read(&cpu_hotplug.dep_map)
+#define cpuhp_lock_acquire() lock_map_acquire(&cpu_hotplug.dep_map)
+#define cpuhp_lock_release() lock_map_release(&cpu_hotplug.dep_map)
+
void get_online_cpus(void)
{
might_sleep();
if (cpu_hotplug.active_writer == current)
return;
+ cpuhp_lock_acquire_read();
mutex_lock(&cpu_hotplug.lock);
cpu_hotplug.refcount++;
mutex_unlock(&cpu_hotplug.lock);
@@ -87,6 +101,7 @@ void put_online_cpus(void)
if (!--cpu_hotplug.refcount && unlikely(cpu_hotplug.active_writer))
wake_up_process(cpu_hotplug.active_writer);
mutex_unlock(&cpu_hotplug.lock);
+ cpuhp_lock_release();

}
EXPORT_SYMBOL_GPL(put_online_cpus);
@@ -117,6 +132,7 @@ void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;

+ cpuhp_lock_acquire();
for (;;) {
mutex_lock(&cpu_hotplug.lock);
if (likely(!cpu_hotplug.refcount))
@@ -131,6 +147,7 @@ void cpu_hotplug_done(void)
{
cpu_hotplug.active_writer = NULL;
mutex_unlock(&cpu_hotplug.lock);
+ cpuhp_lock_release();
}

/*
--
1.8.3.1

--
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/