Re: [PATCH v5 08/13] xen/spinlock: add missing rspin_is_locked() and rspin_barrier()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2024 16:31, Jürgen Groß wrote:
> On 18.03.24 15:57, Jan Beulich wrote:
>> On 14.03.2024 08:20, Juergen Gross wrote:
>>> --- a/xen/common/spinlock.c
>>> +++ b/xen/common/spinlock.c
>>> @@ -395,14 +395,7 @@ static bool always_inline spin_is_locked_common(const 
>>> spinlock_tickets_t *t)
>>>   
>>>   int _spin_is_locked(const spinlock_t *lock)
>>>   {
>>> -    /*
>>> -     * Recursive locks may be locked by another CPU, yet we return
>>> -     * "false" here, making this function suitable only for use in
>>> -     * ASSERT()s and alike.
>>> -     */
>>> -    return lock->recurse_cpu == SPINLOCK_NO_CPU
>>> -           ? spin_is_locked_common(&lock->tickets)
>>> -           : lock->recurse_cpu == smp_processor_id();
>>> +    return spin_is_locked_common(&lock->tickets);
>>>   }
>>
>> The "only suitable for ASSERT()s and alike" part of the comment wants
>> to survive here, I think.
> 
> Why?
> 
> I could understand you asking for putting such a comment to spinlock.h
> mentioning that any *_is_locked() variant isn't safe, but with
> _spin_is_locked() no longer covering recursive locks the comment's reasoning
> is no longer true.

Hmm. I guess there is a difference in expectations. To me, these
functions in principle ought to report whether the lock is "owned",
not just "locked by some CPU". They don't, hence why they may not be
used for other than ASSERT()s.

As to the reasoning no longer being applicable here: That's why I
asked to only retain the "only ASSERT()s" part of the comment. Yes,
such a comment may also be suitable to have in spinlock.h. What I'd
like to avoid is for it to be lost altogether.

Jan

>>> @@ -465,6 +458,23 @@ void _spin_barrier(spinlock_t *lock)
>>>       spin_barrier_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
>>>   }
>>>   
>>> +bool _rspin_is_locked(const rspinlock_t *lock)
>>> +{
>>> +    /*
>>> +     * Recursive locks may be locked by another CPU, yet we return
>>> +     * "false" here, making this function suitable only for use in
>>> +     * ASSERT()s and alike.
>>> +     */
>>> +    return lock->recurse_cpu == SPINLOCK_NO_CPU
>>> +           ? spin_is_locked_common(&lock->tickets)
>>> +           : lock->recurse_cpu == smp_processor_id();
>>> +}
>>
>> Here otoh I wonder if both the comment and the spin_is_locked_common()
>> part of the condition are actually correct. Oh, the latter needs
>> retaining as long as we have nrspin_*() functions, I suppose. But the
>> comment could surely do with improving a little - at the very least
>> "yet we return "false"" isn't quite right; minimally there's a "may"
>> missing.
> 
> If anything I guess the comment shouldn't gain a "may", but rather say
> "Recursive locks may be locked by another CPU via rspin_lock() ..."
> 
> 
> Juergen