Re: [PATCH v4 07/12] xen/spinlock: add explicit non-recursive locking functions

[Juergen Gross <jgross@suse.com>]

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

On 13.12.23 09:36, Julien Grall wrote:
> 
> 
> On 13/12/2023 06:17, Juergen Gross wrote:
>> On 12.12.23 19:49, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 12/12/2023 09:47, Juergen Gross wrote:
>>>> -#define spin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, spinlock_t)
>>>> -#define rspin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, rspinlock_t)
>>>> +#define spin_lock_init_prof(s, 
>>>> l)                                             \
>>>> +    __spin_lock_init_prof(s, l, lock, spinlock_t, 0)
>>>> +#define rspin_lock_init_prof(s, 
>>>> l)                                            \
>>>> +    __spin_lock_init_prof(s, l, rlock, rspinlock_t, 1)
>>>>   void _lock_profile_register_struct(
>>>>       int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
>>>> @@ -174,6 +179,7 @@ struct lock_profile_qhead { };
>>>>   #endif
>>>> +
>>>
>>> Spurious change?
>>
>> Indeed, will remove it.
>>
>>>
>>>>   typedef union {
>>>>       uint32_t head_tail;
>>>>       struct {
>>>> @@ -261,4 +267,12 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, 
>>>> unsigned long flags);
>>>>   /* Ensure a lock is quiescent between two critical operations. */
>>>>   #define spin_barrier(l)               _spin_barrier(l)
>>>> +#define nrspin_trylock(l)    spin_trylock(l)
>>>> +#define nrspin_lock(l)       spin_lock(l)
>>>> +#define nrspin_unlock(l)     spin_unlock(l)
>>>> +#define nrspin_lock_irq(l)   spin_lock_irq(l)
>>>> +#define nrspin_unlock_irq(l) spin_unlock_irq(l)
>>>> +#define nrspin_lock_irqsave(l, f)      spin_lock_irqsave(l, f)
>>>> +#define nrspin_unlock_irqrestore(l, f) spin_unlock_irqrestore(l, f)
>>>
>>> There is a comment describing [r]spinlock but not this new variant. Can you 
>>> add one?
>>
>> Okay.
>>
>>> That said, I know this is existing code, but I have to admit this is a bit 
>>> unclear why we are allowing an recursive spinlock to be non-recursive. To me 
>>> it sounds like we are making the typesafe not very safe because it doesn't 
>>> guarantee that we are not mixing the call nrspin_* with rspin_*.
>>
>> This is the current API.
> 
> I know. This is why I wrote "I know this is existing code".
> 
>>
>> If you have locked with nrspin_*, any rspin_* attempt on the same lock will
>> spin until rspin_unlock (nrspin_* will not set recurse_cpu, but take the
>> lock).
>>
>> If you have locked with rspin_*, any nrspin_* attempt on the same lock will
>> spin, too.
>>
>> So I don't see any major problem regarding accidental misuse, which wouldn't
>> be visible by deadlocks (there is no silent misbehavior).
> 
> Right, so this will lead to a deadlock, which is my concern. If we were using 
> rspinlock_* everywhere then the deadlock (at least in the case when you recurse) 
> would in theory not be possible (unless you recurse too much).

A programming error can lead to a deadlock, yes.

My understanding is that there are deliberate use cases of non-recursive locking
as paths using recursive locking of the same lock are not allowed in those
cases.

Just using rspinlock_* instead of nrspinlock_* would silently ignore such cases,
which is far worse than a deadlock, as those cases might introduce e.g. security
holes.

> My point here is to simplify the interface rather than providing because I don't 
> really see the benefits of providing a non-recursive version for recursive 
> spinlock.
> 
> Anyway as I said this is nothing new.

Correct. Nothing I'd like to address in this series.


Juergen

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

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