Re: [PATCH bpf-next v4 3/9] selftests/bpf: libarena: Make spinlock trylock match arena_spin_unlock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] selftests/bpf: inconsistent return values in
  arena_spin_trylock() variants
--

commit 3875fb781c61e9ab8efcf22a434ad87a4732478e
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: Make spinlock trylock match arena_spin_unlock

This commit updates arena_spin_trylock() to disable preemption on lock
acquisition, splits out arena_spin_trylock_raw() for the lock slowpath,
and adds arena_spin_trylock_irqsave().

> diff --git a/tools/testing/selftests/bpf/libarena/include/bpf_arena_spin_lock.h b/tools/testing/selftests/bpf/libarena/include/bpf_arena_spin_lock.h
> index 055b5892cb636..560ee18fd287f 100644
> --- a/tools/testing/selftests/bpf/libarena/include/bpf_arena_spin_lock.h
> +++ b/tools/testing/selftests/bpf/libarena/include/bpf_arena_spin_lock.h
> @@ -233,7 +233,26 @@ static __always_inline int arena_spin_trylock(arena_spinlock_t __arena *lock)
>  	if (unlikely(val))
>  		return 0;
>  
> -	return likely(atomic_try_cmpxchg_acquire(&lock->val, &val, _Q_LOCKED_VAL));
> +	bpf_preempt_disable();
> +	val = atomic_try_cmpxchg_acquire(&lock->val, &val, _Q_LOCKED_VAL);
> +	if (unlikely(!val))
> +		bpf_preempt_enable();
> +
> +	return val;
> +}

[ ... ]

> @@ -527,6 +546,17 @@ static __always_inline void arena_spin_unlock(arena_spinlock_t __arena *lock)
>  		(__ret);                                 \
>  	})
>  
> +#define arena_spin_trylock_irqsave(lock, flags)			\
> +	({							\
> +		int __ret;					\
> +		bpf_local_irq_save(&(flags));			\
> +		__ret = arena_spin_trylock((lock)) ? 0 : -EBUSY;\
> +		if (__ret)					\
> +			bpf_local_irq_restore(&(flags)); 	\
> +		(__ret);                                 	\
> +	})

[Severity: Low]
Should arena_spin_trylock() and arena_spin_trylock_irqsave() use the same
return value convention?

arena_spin_trylock() returns 1 on success and 0 on failure, whereas
arena_spin_trylock_irqsave() (like arena_spin_lock() and
arena_spin_lock_irqsave()) returns 0 on success and -EBUSY on failure, so
callers switching between the two trylock variants have to invert the
check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182055.7581-1-emil@etsalapatis.com?part=3