Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2024 17:05, Jürgen Groß wrote:
> On 18.03.24 16:59, Jan Beulich wrote:
>> On 18.03.2024 16:55, Jürgen Groß wrote:
>>> On 18.03.24 15:43, Jan Beulich wrote:
>>>> On 14.03.2024 08:20, Juergen Gross wrote:
>>>>> Instead of special casing rspin_lock_irqsave() and
>>>>> rspin_unlock_irqrestore() for the console lock, add those functions
>>>>> to spinlock handling and use them where needed.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> with two remarks:
>>>>
>>>>> --- a/xen/common/spinlock.c
>>>>> +++ b/xen/common/spinlock.c
>>>>> @@ -475,15 +475,31 @@ void _rspin_lock(rspinlock_t *lock)
>>>>>        lock->recurse_cnt++;
>>>>>    }
>>>>>    
>>>>> +unsigned long _rspin_lock_irqsave(rspinlock_t *lock)
>>>>> +{
>>>>> +    unsigned long flags;
>>>>> +
>>>>> +    local_irq_save(flags);
>>>>> +    _rspin_lock(lock);
>>>>> +
>>>>> +    return flags;
>>>>> +}
>>>>> +
>>>>>    void _rspin_unlock(rspinlock_t *lock)
>>>>>    {
>>>>>        if ( likely(--lock->recurse_cnt == 0) )
>>>>>        {
>>>>>            lock->recurse_cpu = SPINLOCK_NO_CPU;
>>>>> -        spin_unlock(lock);
>>>>> +        _spin_unlock(lock);
>>>>
>>>> This looks like an unrelated change. I think I can guess the purpose, but
>>>> it would be nice if such along-the-way changes could be mentioned in the
>>>> description.
>>>
>>> I think it would be better to move that change to patch 3.
>>
>> Hmm, it would be a secondary change there, too. I was actually meaning to
>> commit patches 2-5, but if things want moving around I guess I better
>> wait with doing so?
> 
> Hmm, maybe just drop this hunk and let patch 7 handle it?

Ah yes, that seem more logical to me. I take it you don't mean "hunk"
though, but really just this one line change.

Jan