Re: [PATCH 2/2] xen/spinlock: merge recurse_cpu and debug.cpu fields in struct spinlock

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.02.2022 09:55, Juergen Gross wrote:
> On 25.02.22 09:36, Juergen Gross wrote:
>> On 24.02.22 17:11, Jan Beulich wrote:
>>> On 24.02.2022 11:54, Juergen Gross wrote:
>>>> --- a/xen/arch/x86/mm/mm-locks.h
>>>> +++ b/xen/arch/x86/mm/mm-locks.h
>>>> @@ -42,7 +42,7 @@ static inline void mm_lock_init(mm_lock_t *l)
>>>>   static inline bool mm_locked_by_me(const mm_lock_t *l)
>>>>   {
>>>> -    return (l->lock.recurse_cpu == current->processor);
>>>> +    return (l->lock.data.cpu == current->processor);
>>>>   }
>>>
>>> I see a fair risk with this: Behavior will now differ between debug and
>>> non-debug builds. E.g. a livelock because of trying to acquire the same
>>> lock again would not be noticed in a debug build if the acquire is
>>> conditional upon this function's return value. I think this is the main
>>> reason behind having two separate field, despite the apparent redundancy.
>>
>> You are aware that mm_locked_by_me() is used for recursive spinlocks
>> only?
> 
> BTW, it might make sense to add another bool for the debug case to mark
> recursive lock usage. I don't think anything good can come from using a
> lock in both modes (recursive and non-recursive).

But beware of the coexisting paging_lock() and paging_lock_recursive().
Albeit I guess your comment was for spinlocks in general, not the
mm-lock machinery. Yet mentioning this reminds me of the page alloc
lock, which different paths acquire in different ways. So the bit
couldn't be a sticky one.

Jan