lkml.org 
[lkml]   [2014]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 01/51] CPU hotplug: Provide lockless versions of callback registration functions
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.

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

> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> kernel/cpu.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index deff2e6..3d2dd1c 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,21 +58,34 @@ 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
> };
>
> +#define cphp_lock_acquire_read(l, s, t, i) lock_acquire_shared_recursive(l, s, t, NULL, i)
> +#define cphp_lock_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, NULL, i)
> +#define cphp_lock_release(l, n, i) lock_release(l, n, i)
> +

Can you make them cpuhp_* instead of cphp_*? That way it would suit better as
a short-form of "cpu hotplug".

Also, perhaps we could use the lock_map_acquire(), lock_map_acquire_read()
and lock_map_release() macros to make the call-sites look neater.

Regards,
Srivatsa S. Bhat


> void get_online_cpus(void)
> {
> might_sleep();
> if (cpu_hotplug.active_writer == current)
> return;
> + cphp_lock_acquire_read(&cpu_hotplug.dep_map, 0, 0, _RET_IP_);
> mutex_lock(&cpu_hotplug.lock);
> cpu_hotplug.refcount++;
> mutex_unlock(&cpu_hotplug.lock);
>
> +
> }
> EXPORT_SYMBOL_GPL(get_online_cpus);
>
> @@ -79,6 +93,7 @@ void put_online_cpus(void)
> {
> if (cpu_hotplug.active_writer == current)
> return;
> +
> mutex_lock(&cpu_hotplug.lock);
>
> if (WARN_ON(!cpu_hotplug.refcount))
> @@ -87,6 +102,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);
> + cphp_lock_release(&cpu_hotplug.dep_map, 1, _RET_IP_);
>
> }
> EXPORT_SYMBOL_GPL(put_online_cpus);
> @@ -117,6 +133,7 @@ void cpu_hotplug_begin(void)
> {
> cpu_hotplug.active_writer = current;
>
> + cphp_lock_acquire(&cpu_hotplug.dep_map, 0, 0, _RET_IP_);
> for (;;) {
> mutex_lock(&cpu_hotplug.lock);
> if (likely(!cpu_hotplug.refcount))
> @@ -131,6 +148,7 @@ void cpu_hotplug_done(void)
> {
> cpu_hotplug.active_writer = NULL;
> mutex_unlock(&cpu_hotplug.lock);
> + cphp_lock_release(&cpu_hotplug.dep_map, 1, _RET_IP_);
> }
>
> /*
>



\
 
 \ /
  Last update: 2014-02-10 11:21    [W:2.543 / U:0.848 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site