linux-kernel - Re: [PATCH 01/51] CPU hotplug: Provide lockless versions of callback registration functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52F8D403.3040306@linux.vnet.ibm.com>
Date:	Mon, 10 Feb 2014 18:58:35 +0530
From:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
To:	ego@...ux.vnet.ibm.com
CC:	Oleg Nesterov <oleg@...hat.com>, paulus@...ba.org,
	rusty@...tcorp.com.au, peterz@...radead.org, tglx@...utronix.de,
	akpm@...ux-foundation.org, mingo@...nel.org,
	paulmck@...ux.vnet.ibm.com, tj@...nel.org, walken@...gle.com,
	linux@....linux.org.uk, linux-kernel@...r.kernel.org,
	Toshi Kani <toshi.kani@...com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH 01/51] CPU hotplug: Provide lockless versions of	callback
 registration functions

On 02/10/2014 05:35 PM, Gautham R Shenoy wrote:
> On Mon, Feb 10, 2014 at 04:41:04PM +0530, Srivatsa S. Bhat wrote:
>>> ---
>> [...]
>>> +/* 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();
>>
>> Shouldn't we move this to _after_ the for-loop?
> 
> No if we move this to after the for-loop, we won't be able to catch
> the ABBA dependency that you had mentioned earlier.
> 
> Consider the case
> 
> Thread1:                                              Thread 2:
> ------------------------------------------------------------------------
> get_online_cpus()
> // lockdep knows about this.
>                                                cpu_maps_update_begin()
>                                                //lockdep knows about this.
> 
> register_cpu_notifier()
> |
> |-> cpu_maps_update_begin()
>     //lockdep knows about this. 
> 
> 
>                                               cpu_hotplug_begin()
>                                               |  
>                                               |-->for(;;) {
>                                                       Wait for all the
>                                                       readers to exit. 
>                                      
>                                                       This will never
>                                                       happen now and
>                                                       we're stuck here
>                                                       forever without
>                                                       telling anyone why! 
>                                                  }
> 
> 					         cpuhp_lock_acquire();
>                                                                         

Ok, that is a very convincing explanation!

> --------------------------------------------------------------------------
>> Because, that's when the
>> hotplug writer is really in a state equivalent to exclusive access to the
>> hotplug lock... Else, we might fool lockdep into believing that the hotplug
>> writer has the lock for write, and at the same time several readers have
>> the lock for read as well.. no?
>>
> 
> Well as I understand it, the purpose of lockdep annotations is to
> signal the intent of acquiring a lock as opposed to reporting the
> status that the lock has been acquired.
> 
> The annotation in the earlier patch is consistent with the lockdep
> annotations in rwlocks. Except for the fact that the readers of
> cpu_hotplug.lock can sleep having acquired the lock, there's no
> difference between rwlock semantics and cpu-hotplug lock behaviour.
> Both are unfair to the writer as they allow new readers to acquire the
> lock as long as there's some reader which holds the lock.
>

Ah, ok.. Thanks a lot for the clarification!
 
Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/