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