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

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

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();
                                                                        
--------------------------------------------------------------------------
> 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.

--
Thanks and Regards
gautham.