Messages in this thread | | | Date | Mon, 10 Feb 2014 18:58:35 +0530 | From | "Srivatsa S. Bhat" <> | 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
| |