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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 1787 bytes --]

On 25.02.22 10:24, Jan Beulich wrote:
> 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.

Interesting.

Seems as if e.g. console_lock is used in both ways, too.

Might be a good idea to at least add some self-deadlock detection
support to debug builds.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]