Re: [Xen-devel] [PATCH] x86/flush: use APIC ALLBUT destination shorthand when possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2020 19:14, Roger Pau Monné wrote:
> On Wed, Jan 08, 2020 at 02:54:57PM +0100, Jan Beulich wrote:
>> On 08.01.2020 14:30, Roger Pau Monné  wrote:
>>> On Fri, Jan 03, 2020 at 01:55:51PM +0100, Jan Beulich wrote:
>>>> On 03.01.2020 13:34, Roger Pau Monné wrote:
>>>>> On Fri, Jan 03, 2020 at 01:08:20PM +0100, Jan Beulich wrote:
>>>>>> On 24.12.2019 13:44, Roger Pau Monne wrote:
>>>>>> Further a question on lock nesting: Since the commit message
>>>>>> doesn't say anything in this regard, did you check there are no
>>>>>> TLB flush invocations with the get_cpu_maps() lock held?
>>>>>
>>>>> The CPU maps lock is a recursive one, so it should be fine to attempt
>>>>> a TLB flush with the lock already held.
>>>>
>>>> When already held by the same CPU - sure. It being a recursive
>>>> one (which I paid attention to when writing my earlier reply)
>>>> doesn't make it (together with any other one) immune against
>>>> ABBA deadlocks, though.
>>>
>>> There's no possibility of a deadlock here because get_cpu_maps does a
>>> trylock, so if another cpu is holding the lock the flush will just
>>> fallback to not using the shorthand.
>>
>> Well, with the _exact_ arrangements (flush_lock used only in one
>> place, and cpu_add_remove_lock only used in ways similar to how it
>> is used now), there's no such risk, I agree. But there's nothing
>> at all making sure this doesn't change. Hence, as said, at the very
>> least this needs reasoning about in the description (or a code
>> comment).
> 
> I'm afraid you will have to bear with me, but I'm still not sure how
> the addition of get_cpu_maps in flush_area_mask can lead to deadlocks.
> As said above get_cpu_maps does a trylock, which means that it will
> never deadlock, and that's the only way to lock cpu_add_remove_lock.

That's why I said "cpu_add_remove_lock only used in ways similar to
how it is used now" - any non-trylock addition would break the
assumptions you make afaict. And you can't really expect people
adding another (slightly different) use of an existing lock to be
aware they now need to check whether this introduces deadlock
scenarios on unrelated pre-existing code paths. Hence my request to
at least discuss this in the description (raising awareness, and
hence allowing reviewers to judge whether further precautionary
measures should be taken).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel