Re: [PATCH bpf-next v4 5/9] selftests/bpf: libarena: Add buddy per-CPU caches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] selftests/bpf: conflicting prototype for buddy_alloc_memory()
- [Low] selftests/bpf: buddy_alloc_nogrow() bypasses buddy_cache_alloc()
- [Low] selftests/bpf: missing static on buddy_cache_alloc() and
  buddy_cache_free()
--

commit 2da1f713f762ba4737b95c532cd5e6f183f3f061
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: Add buddy per-CPU caches

This commit adds per-CPU LIFO array caches to the libarena buddy allocator
for the eight smallest allocation orders so small allocations and frees can
avoid taking the central buddy lock.

> diff --git a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> index 514d0a59eaff5..13c55cf53e524 100644
> --- a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> +++ b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> @@ -38,18 +41,162 @@
>   * and their allocation state. It also includes the freelist
>   * heads for the allocation itself.
>   */
> +static u8 idx_get_order(struct buddy_chunk __arena *chunk, u64 idx);
> +static int idx_is_allocated(struct buddy_chunk __arena *chunk, u64 idx, bool *allocated);
> +static u64 arena_next_pow2(__u64 n);
> +static u64 size_to_order(size_t size);
> +static void __arena *buddy_alloc_memory(struct buddy __arena *buddy, size_t size,
> +					int order, size_t norder);
> +static int buddy_free_unlocked(struct buddy __arena *buddy, u64 addr);

[Severity: Medium]
Does this forward declaration of buddy_alloc_memory() break the build at
this commit?  It omits the fifth parameter (bool grow) from the definition:

tools/testing/selftests/bpf/libarena/src/buddy.bpf.c:
static void __arena *buddy_alloc_memory(struct buddy __arena *buddy, size_t size,
		int order, size_t norder, bool grow)

Although the later commit "selftests/bpf: libarena: Handle NMI reentrancy"
adds bool grow to the prototype, this commit fails to compile with
conflicting types and breaks bisection.  Note also that buddy_alloc_memory(),
arena_next_pow2(), size_to_order() and buddy_free_unlocked() are not called
before their definitions in this patch.

[ ... ]

> +void __arena *buddy_cache_alloc(struct buddy __arena *buddy, size_t order)
> +{
> +	struct buddy_cache __arena *cache;
> +	void __arena *address = NULL;
> +
> +	if (!buddy_order_cached(order))
> +		return NULL;
> +
> +	cache = buddy_cache_enter(buddy);
> +	if (!cache)
> +		return NULL;
> +
> +	address = buddy_cache_pop(cache, order);
> +	buddy_cache_exit(cache);
> +
> +	return address;
> +}
> +
> +/*
> + * Retrieve the order of an allocation from the address. Reject addresses
> + * that do not start an allocated block so that the locked path can report
> + * them, instead of caching memory that overlaps a live allocation. Cached
> + * objects stay marked as allocated, so freeing an object that is already
> + * cached is not detected.
> + */
> +static int buddy_cache_get_order(void __arena *addr)
> +{
> +	struct buddy_chunk __arena *chunk;
> +	bool allocated;
> +	u64 idx;
> +
> +	if ((u64)addr & (BUDDY_MIN_ALLOC_BYTES - 1))
> +		return BUDDY_CHUNK_NUM_ORDERS;
>  
> +	chunk = (void __arena *)((u64)addr & ~BUDDY_CHUNK_OFFSET_MASK);
> +	idx = ((u64)addr & BUDDY_CHUNK_OFFSET_MASK) / BUDDY_MIN_ALLOC_BYTES;
> +
> +	if (idx_is_allocated(chunk, idx, &allocated) || !allocated)
> +		return BUDDY_CHUNK_NUM_ORDERS;
> +
> +	return idx_get_order(chunk, idx);
> +}
> +
> +__always_inline int buddy_cache_free(struct buddy __arena *buddy, void __arena *addr)

[Severity: Low]
Should buddy_cache_alloc() and buddy_cache_free() be marked static like the
other internal cache helpers in buddy.bpf.c?  Neither helper is declared in
buddy.h or referenced outside this file, and omitting static prevents
buddy_cache_alloc() from being inlined as a local helper into buddy_alloc().

[ ... ]

> @@ -819,7 +1012,18 @@ static void __arena *buddy_alloc_memory(struct buddy __arena *buddy, size_t size
>  __weak
>  void __arena *buddy_alloc(struct buddy __arena *buddy, size_t size)
>  {
> -	return buddy_alloc_memory(buddy, size, size_to_order(size), 0, true);
> +	void __arena *address;
> +	int order;
> +
> +	if (!buddy)
> +		return NULL;
> +
> +	order = size_to_order(size);
> +	address = buddy_cache_alloc(buddy, order);
> +	if (address)
> +		return address;
> +
> +	return buddy_alloc_memory(buddy, size, order, 0, true);
>  }
>  
>  __weak

[Severity: Low]
Should buddy_alloc_nogrow() also check buddy_cache_alloc() before falling
back to buddy_alloc_memory()?

tools/testing/selftests/bpf/libarena/src/buddy.bpf.c:
__weak
void __arena *buddy_alloc_nogrow(struct buddy __arena *buddy, size_t size)
{
	return buddy_alloc_memory(buddy, size, size_to_order(size), 0, false);
}

When buddy_free() caches a small object, the object stays marked allocated
and off the chunk freelists.  Because buddy_alloc_nogrow() calls
buddy_alloc_memory() directly, it always takes the central buddy lock and
can return NULL when the chunk freelists are empty even though free objects
of the requested order are sitting in the local per-CPU cache.

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