Re: [PATCH v15 09/16] bpf/rqspinlock: switch check_timeout() to a clock interface

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

> diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
> index 111ec80ea958..1b249c6f0674 100644
> --- a/kernel/bpf/rqspinlock.c
> +++ b/kernel/bpf/rqspinlock.c

[ ... ]

> @@ -196,8 +196,12 @@ static noinline int check_deadlock_ABBA(rqspinlock_t *lock, u32 mask)
>  	return 0;
>  }
>
> -static noinline int check_timeout(rqspinlock_t *lock, u32 mask,
> -				  struct rqspinlock_timeout *ts)
> +/*
> + * Returns current monotonic time in ns on success or, negative errno
> + * value on failure due to timeout expiration or detection of deadlock.
> + */
> +static noinline s64 clock_deadlock(rqspinlock_t *lock, u32 mask,
> +				   struct rqspinlock_timeout *ts)
>  {

This isn't a bug, but could the commit message mention that
check_timeout() is renamed to clock_deadlock()? The subject and body
still refer to check_timeout(), but readers of the patch will see
clock_deadlock() in the diff.

Also, the changelog describes what the new interface does but not why
it's needed. Could it note that the clock return value is what the
following patch feeds to smp_cond_load_acquire_timeout()?

[ ... ]

> @@ -231,15 +239,22 @@ static noinline int check_timeout(rqspinlock_t *lock, u32 mask,
>   * as the macro does internal amortization for us.
>   */
>  #ifndef res_smp_cond_load_acquire
> -#define RES_CHECK_TIMEOUT(ts, ret, mask)                              \
> -	({                                                            \
> -		if (!(ts).spin++)                                     \
> -			(ret) = check_timeout((lock), (mask), &(ts)); \
> -		(ret);                                                \
> +#define RES_CHECK_TIMEOUT(ts, ret, mask)					\
> +	({									\
> +		s64 __timeval_err = 0;						\
> +		if (!(ts).spin++)						\
> +			__timeval_err = clock_deadlock((lock), (mask), &(ts));	\
> +		(ret) = __timeval_err < 0 ? __timeval_err : 0;			\
> +		__timeval_err;							\
>  	})
>  #else
> -#define RES_CHECK_TIMEOUT(ts, ret, mask)			      \
> -	({ (ret) = check_timeout((lock), (mask), &(ts)); })
> +#define RES_CHECK_TIMEOUT(ts, ret, mask)					\
> +	({									\
> +		s64 __timeval_err;						\
> +		__timeval_err = clock_deadlock((lock), (mask), &(ts));		\
> +		(ret) = __timeval_err < 0 ? __timeval_err : 0;			\
> +		__timeval_err;							\
> +	})
>  #endif

This isn't a bug, but since the previous patch removed the last
definition of res_smp_cond_load_acquire, is the #else arm still
reachable, or could it be dropped here rather than updated?

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33438155296