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

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

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