Messages in this thread | | | Date | Mon, 10 Feb 2014 17:35:26 +0530 | From | Gautham R Shenoy <> | Subject | Re: [PATCH 01/51] CPU hotplug: Provide lockless versions of callback registration functions |
| |
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.
| |