Re: [PATCH v8 10/12] bpf/rqspinlock: Use smp_cond_load_acquire_timeout()

[Ankur Arora <ankur.a.arora@oracle.com>]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Sun, Dec 14, 2025 at 8:51 PM Ankur Arora <ankur.a.arora@oracle.com> wrote:
>>
>>  /**
>>   * resilient_queued_spin_lock_slowpath - acquire the queued spinlock
>>   * @lock: Pointer to queued spinlock structure
>> @@ -415,7 +415,9 @@ int __lockfunc resilient_queued_spin_lock_slowpath(rqspinlock_t *lock, u32 val)
>>          */
>>         if (val & _Q_LOCKED_MASK) {
>>                 RES_RESET_TIMEOUT(ts, RES_DEF_TIMEOUT);
>> -               res_smp_cond_load_acquire(&lock->locked, !VAL || RES_CHECK_TIMEOUT(ts, timeout_err, _Q_LOCKED_MASK) < 0);
>> +               smp_cond_load_acquire_timeout(&lock->locked, !VAL,
>> +                                             (timeout_err = clock_deadlock(lock, _Q_LOCKED_MASK, &ts)),
>> +                                             ts.duration);
>
> I'm pretty sure we already discussed this and pointed out that
> this approach is not acceptable.

Where? I don't see any mail on this at all.

In any case your technical point below is quite reasonable. It's better
to lead with that instead of peremptorily declaring what you find
acceptable and what not.

> We cannot call ktime_get_mono_fast_ns() first.
> That's why RES_CHECK_TIMEOUT() exists and it does
> if (!(ts).spin++)
> before doing the first check_timeout() that will do ktime_get_mono_fast_ns().
> Above is a performance critical lock acquisition path where
> pending is spinning on the lock word waiting for the owner to
> release the lock.
> Adding unconditional ktime_get_mono_fast_ns() will destroy
> performance for quick critical section.

Yes that makes sense.

This is not ideal, but how about something like this:

  #define smp_cond_load_relaxed_timeout(ptr, cond_expr,                 \
                                      time_expr_ns, timeout_ns)         \
  ({                                                                    \
        typeof(ptr) __PTR = (ptr);                                      \
        __unqual_scalar_typeof(*ptr) VAL;                               \
        u32 __n = 0, __spin = SMP_TIMEOUT_POLL_COUNT;                   \
        s64 __timeout = (s64)timeout_ns;                                \
        s64 __time_now, __time_end = 0;                                 \
                                                                        \
        for (;;) {                                                      \
                VAL = READ_ONCE(*__PTR);                                \
                if (cond_expr)                                          \
                        break;                                          \
                cpu_poll_relax(__PTR, VAL, __timeout);                  \
                if (++__n < __spin)                                     \
                        continue;                                       \
                __time_now = (s64)(time_expr_ns);                       \
                if (unlikely(__time_end == 0))                          \
                        __time_end = __time_now + __timeout;            \
                __timeout = __time_end - __time_now;                    \
                if (__time_now <= 0 || __timeout <= 0) {                \
                        VAL = READ_ONCE(*__PTR);                        \
                        break;                                          \
                }                                                       \
                __n = 0;                                                \
        }                                                               \
                                                                        \
        (typeof(*ptr))VAL;                                              \
  })

The problem with this version is that there's no way to know how much
time has passed in the first cpu_poll_relax()). So for the waiting case
this has a builtin overshoot of up to __timeout for the WFET case.
And ~100us for WFE and ~2us for x86 cpu_relax.

Of course we could specify a small __timeout value for the first iteration
which would help WFET.

Anyway let me take another look at this tomorrow.

But, that is more complexity.

Opinions?

--
ankur