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