Re: [PATCH 2/6] selftests/bpf: libarena: Inline nonatomic bitmap operations

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message explicitly states that writes are marked with WRITE_ONCE(), but the implementation uses raw volatile pointer casts instead.
--

--- Patch [2]: [PATCH 2/6] selftests/bpf: libarena: Inline nonatomic bitmap operations ---
commit 04b634add8119906bf065c817d1ba95b1411fb60
Author: Emil Tsalapatis <[email protected]>

selftests/bpf: libarena: Inline nonatomic bitmap operations

This commit moves the non-atomic bitmap operations to be static inline
functions in the header to reduce function call overhead. It also updates
these operations to supposedly use READ_ONCE() and WRITE_ONCE() to prevent
compiler optimizations.

> diff --git a/tools/testing/selftests/bpf/libarena/include/libarena/bitmap.h b/tools/testing/selftests/bpf/libarena/include/libarena/bitmap.h
> index 8c5936ae9958c..163e2b83d9431 100644
> --- a/tools/testing/selftests/bpf/libarena/include/libarena/bitmap.h
> +++ b/tools/testing/selftests/bpf/libarena/include/libarena/bitmap.h

[ ... ]

> @@ -33,3 +32,27 @@ void bmp_copy(size_t bits, struct arena_bitmap __arena *dst, struct arena_bitmap
>  bool bmp_intersects(size_t bits, struct arena_bitmap __arena *arg1, struct arena_bitmap __arena *arg2);
>  bool bmp_subset(size_t bits, struct arena_bitmap __arena *big, struct arena_bitmap __arena *small);
>  void bmp_print(size_t bits, struct arena_bitmap __arena *bmp);
> +
> +static __always_inline
> +void __bmp_set_bit(u32 bit, struct arena_bitmap __arena *bmp)
> +{
> +	volatile u64 __arena *word = &bmp->bits[BIT_WORD(bit)];
> +
> +	*word |= BIT_MASK(bit);
> +}

[Severity: Medium]
The commit message explicitly states that writes are marked with WRITE_ONCE(),
but this function uses a raw volatile pointer cast instead. Should this be
updated to use WRITE_ONCE() to match the commit description?

> +
> +static __always_inline
> +void __bmp_clear_bit(u32 bit, struct arena_bitmap __arena *bmp)
> +{
> +	volatile u64 __arena *word = &bmp->bits[BIT_WORD(bit)];
> +
> +	*word &= ~BIT_MASK(bit);
> +}

[Severity: Medium]
Similarly here, a raw volatile pointer cast is used rather than WRITE_ONCE().
Does this code intend to use explicit WRITE_ONCE() as promised in the commit
message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2