Re: [PATCH RESEND bpf-next v3 4/6] selftests/bpf: Add arena-based bitmap data structure

["Kumar Kartikeya Dwivedi" <memxor@gmail.com>]

On Mon Jul 6, 2026 at 9:17 PM CEST, Emil Tsalapatis wrote:
> On Mon Jul 6, 2026 at 3:04 PM EDT, Kumar Kartikeya Dwivedi wrote:
>> On Mon Jul 6, 2026 at 8:27 PM CEST, sashiko-bot wrote:
>>> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>>> - [High] Silent failures and falsely reported lock acquisition in bounded atomic bit operations on loop abort.
>>> - [High] `bmp_empty` and `bmp_subset` return `true` on an aborted loop, defaulting to an unsafe affirmative state.
>>> - [Medium] Bulk operations unconditionally apply a bitmask to the last word even if the processing loop aborts early.
>>> - [Medium] Missing `READ_ONCE()` or `volatile` cast in `bmp_test_bit` exposes lockless polling loops to compiler hoisting.
>>> --
>>>
>>> commit 634ac04142cf736158c99f577c0f903470937bfc
>>> Author: Emil Tsalapatis <emil@etsalapatis.com>
>>> selftests/bpf: Add arena-based bitmap data structure
>>>
>>> This patch adds an arena-based word-aligned bitmap data structure. The
>>> structure is useful as a building block for components like sched-ext
>>> to represent cpumask structures.
>>>
>>>> diff --git a/tools/testing/selftests/bpf/libarena/src/bitmap.bpf.c b/tools/testing/selftests/bpf/libarena/src/bitmap.bpf.c
>>>> index 0000000000000..80e814401fb9e
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/bpf/libarena/src/bitmap.bpf.c
>>> [ ... ]
>>>> +__weak
>>>> +bool bmp_test_bit(u32 bit, struct bitmap __arena *bmp)
>>>> +{
>>>> +	return bmp->bits[BIT_WORD(bit)] & BIT_MASK(bit);
>>>> +}
>>>
>>> [Severity: Medium]
>>> Does this memory access need a READ_ONCE() or a volatile pointer cast?
>>>
>>> If this function is used in a lockless concurrent polling loop, could the
>>> compiler hoist the plain memory read out of the loop and cause an infinite
>>> spin that misses updates from other CPUs?
>>>
>>
>> This one seems worth addressing; I would ignore the rest about can_loop
>> exhaustion. Please send a follow up fix.
>
> The function is __weak so AFAICT the compiler would not optimize the
> call away even if it was reasoning based on the function's body. I
> likewise don't think READ_ONCE() would affect anything (correct me
> if I'm wrong).
>

Hm, right about __weak. But as you note below, changing it to static inline
invalidates the assumption compiler won't use the body to optimize code, so
let's just add it while at it.

> That being said, this function should probably be defined straight in the
> bitmap.h header as static inline. It's dumb to require a full call for
> a bit operation just because we use __weak the convention for libarena
> external API functions.
>
> I'll send a new patch with the call moved to the header and READ/WRITE
> once for the non-atomic get/set/clear ops.
>

SGTM.

>>
>>> [...]