Re: [PATCH bpf-next v1 14/22] rqspinlock: Add macros for rqspinlock usage

[Waiman Long <llong@redhat.com>]

On 1/8/25 3:41 PM, Kumar Kartikeya Dwivedi wrote:
> On Wed, 8 Jan 2025 at 22:26, Waiman Long <llong@redhat.com> wrote:
>> On 1/7/25 8:59 AM, Kumar Kartikeya Dwivedi wrote:
>>> Introduce helper macros that wrap around the rqspinlock slow path and
>>> provide an interface analogous to the raw_spin_lock API. Note that
>>> in case of error conditions, preemption and IRQ disabling is
>>> automatically unrolled before returning the error back to the caller.
>>>
>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>>> ---
>>>    include/asm-generic/rqspinlock.h | 58 ++++++++++++++++++++++++++++++++
>>>    1 file changed, 58 insertions(+)
>>>
>>> diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
>>> index dc436ab01471..53be8426373c 100644
>>> --- a/include/asm-generic/rqspinlock.h
>>> +++ b/include/asm-generic/rqspinlock.h
>>> @@ -12,8 +12,10 @@
>>>    #include <linux/types.h>
>>>    #include <vdso/time64.h>
>>>    #include <linux/percpu.h>
>>> +#include <asm/qspinlock.h>
>>>
>>>    struct qspinlock;
>>> +typedef struct qspinlock rqspinlock_t;
>>>
>>>    extern int resilient_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val, u64 timeout);
>>>
>>> @@ -82,4 +84,60 @@ static __always_inline void release_held_lock_entry(void)
>>>        this_cpu_dec(rqspinlock_held_locks.cnt);
>>>    }
>>>
>>> +/**
>>> + * res_spin_lock - acquire a queued spinlock
>>> + * @lock: Pointer to queued spinlock structure
>>> + */
>>> +static __always_inline int res_spin_lock(rqspinlock_t *lock)
>>> +{
>>> +     int val = 0;
>>> +
>>> +     if (likely(atomic_try_cmpxchg_acquire(&lock->val, &val, _Q_LOCKED_VAL))) {
>>> +             grab_held_lock_entry(lock);
>>> +             return 0;
>>> +     }
>>> +     return resilient_queued_spin_lock_slowpath(lock, val, RES_DEF_TIMEOUT);
>>> +}
>>> +
>>> +static __always_inline void res_spin_unlock(rqspinlock_t *lock)
>>> +{
>>> +     struct rqspinlock_held *rqh = this_cpu_ptr(&rqspinlock_held_locks);
>>> +
>>> +     if (unlikely(rqh->cnt > RES_NR_HELD))
>>> +             goto unlock;
>>> +     WRITE_ONCE(rqh->locks[rqh->cnt - 1], NULL);
>>> +     /*
>>> +      * Release barrier, ensuring ordering. See release_held_lock_entry.
>>> +      */
>>> +unlock:
>>> +     queued_spin_unlock(lock);
>>> +     this_cpu_dec(rqspinlock_held_locks.cnt);
>>> +}
>>> +
>>> +#define raw_res_spin_lock_init(lock) ({ *(lock) = (struct qspinlock)__ARCH_SPIN_LOCK_UNLOCKED; })
>>> +
>>> +#define raw_res_spin_lock(lock)                    \
>>> +     ({                                         \
>>> +             int __ret;                         \
>>> +             preempt_disable();                 \
>>> +             __ret = res_spin_lock(lock);       \
>>> +             if (__ret)                         \
>>> +                     preempt_enable();          \
>>> +             __ret;                             \
>>> +     })
>>> +
>>> +#define raw_res_spin_unlock(lock) ({ res_spin_unlock(lock); preempt_enable(); })
>>> +
>>> +#define raw_res_spin_lock_irqsave(lock, flags)    \
>>> +     ({                                        \
>>> +             int __ret;                        \
>>> +             local_irq_save(flags);            \
>>> +             __ret = raw_res_spin_lock(lock);  \
>>> +             if (__ret)                        \
>>> +                     local_irq_restore(flags); \
>>> +             __ret;                            \
>>> +     })
>>> +
>>> +#define raw_res_spin_unlock_irqrestore(lock, flags) ({ raw_res_spin_unlock(lock); local_irq_restore(flags); })
>>> +
>>>    #endif /* __ASM_GENERIC_RQSPINLOCK_H */
>> Lockdep calls aren't included in the helper functions. That means all
>> the *res_spin_lock*() calls will be outside the purview of lockdep. That
>> also means a multi-CPU circular locking dependency involving a mixture
>> of qspinlocks and rqspinlocks may not be detectable.
> Yes, this is true, but I am not sure whether lockdep fits well in this
> case, or how to map its semantics.
> Some BPF users (e.g. in patch 17) expect and rely on rqspinlock to
> return errors on AA deadlocks, as nesting is possible, so we'll get
> false alarms with it. Lockdep also needs to treat rqspinlock as a
> trylock, since it's essentially fallible, and IIUC it skips diagnosing
> in those cases.
Yes, we can certainly treat rqspinlock as a trylock.

> Most of the users use rqspinlock because it is expected a deadlock may
> be constructed at runtime (either due to BPF programs or by attaching
> programs to the kernel), so lockdep splats will not be helpful on
> debug kernels.

In most cases, lockdep will report a cyclic locking dependency 
(potential deadlock) before a real deadlock happens as it requires the 
right combination of events happening in a specific sequence. So lockdep 
can report a deadlock while the runtime check of rqspinlock may not see 
it and there is no locking stall. Also rqspinlock will not see the other 
locks held in the current context.


> Say if a mix of both qspinlock and rqspinlock were involved in an ABBA
> situation, as long as rqspinlock is being acquired on one of the
> threads, it will still timeout even if check_deadlock fails to
> establish presence of a deadlock. This will mean the qspinlock call on
> the other side will make progress as long as the kernel unwinds locks
> correctly on failures (by handling rqspinlock errors and releasing
> held locks on the way out).

That is true only if the latest lock to be acquired is a rqspinlock. If 
all the rqspinlocks in the circular path have already been acquired, no 
unwinding is possible.

That is probably not an issue with the limited rqspinlock conversion in 
this patch series. In the future when more and more locks are converted 
to use rqspinlock, this scenario may happen.

Cheers,
Longman