Re: [PATCH bpf v5 3/3] selftests/bpf: add tests for percpu map flags combination

[Leon Hwang <leon.hwang@linux.dev>]

On 6/10/26 16:46, Masoud Aghasi wrote:
> All possible combinations of (BPF_EXIST, BPF_NOEXIST) and
> (BPF_F_ALL_CPUS, BPF_F_CPU) flags are covered for all percpu map types.
> 
> As BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE does not support BPF_NOEXIST and
> also in order to reduce the amount of code duplicates, I added its
> new test scenarios to the existing cpu_flag_percpu_cgroup_storage test.
> 
> But for other percpu map types, I added new tests to be able to
> exercise all flag combinations and the edge cases such as percpu LRU
> unnecessary deletion issue.
> 
> Signed-off-by: Masoud Aghasi <maghasi@disroot.org>
> ---
>  .../selftests/bpf/prog_tests/percpu_alloc.c   | 249 ++++++++++++++++++
>  1 file changed, 249 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c b/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c
> index 7b4a1e24363b..22d5e7330254 100644
> --- a/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c
> @@ -449,6 +449,90 @@ static void test_lru_percpu_hash_cpu_flag_create(void)
>  	test_percpu_map_cpu_flag_create(BPF_MAP_TYPE_LRU_PERCPU_HASH, 0);
>  }
>  
> +static bool verify_u32_map_value_all_cpus(int map_fd, void *key, u32 *values,
> +					  u32 expected_value, int nr_cpus)

NIT: 'u32' seems unnecessary in the function name.

> +{
> +	int i, err;
> +
> +	err = bpf_map_lookup_elem(map_fd, key, values);
> +	if (!ASSERT_OK(err, "bpf_map_lookup_elem all_cpus"))
> +		return false;
> +
> +	for (i = 0 ; i < nr_cpus ; i++) {
> +		if (!ASSERT_EQ(values[i * 2], expected_value, "bpf_map_lookup_elem value all_cpus"))

'i * 2' looks hard to understand.

Should pass 'void *values, size_t elem_sz', then get value by '*(u32
*)(value + elem_sz * i)'

> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool verify_u32_map_value_first_cpu(int map_fd, void *key, u32 *values,
> +					   u32 expected_value, int nr_cpus)

Ditto.

> +{
> +	int i, err;
> +
> +	err = bpf_map_lookup_elem(map_fd, key, values);
> +	if (!ASSERT_OK(err, "bpf_map_lookup_elem cpu"))
> +		return false;

Can ASSERT_EQ() the first slot here instead of the last? Just for
readability.

> +
> +	for (i = 1 ; i < nr_cpus ; i++) {
> +		if (!ASSERT_NEQ(values[i * 2], expected_value, "bpf_map_lookup_elem value cpu"))

Ditto.

> +			return false;
> +	}
> +
> +	return ASSERT_EQ(values[0], expected_value, "bpf_map_lookup_elem value cpu");
> +}
> +

[...]

> +
> +	if (map_type != BPF_MAP_TYPE_LRU_PERCPU_HASH)
> +		goto out;
> +
> +	/* Percpu LRU hash map should not delete old entries unnecessarily */
> +	flags = BPF_F_ALL_CPUS | BPF_NOEXIST;
> +	key = 2;
> +	err = bpf_map_update_elem(map_fd, &key, values, flags);
> +	if (!ASSERT_OK(err, "bpf_map_update_elem unnecessary_deletion"))

NIT: unnecessary_deletion  ->  lru_percpu_hash all_cpus|noexist

> +		goto out;
> +
> +	flags = BPF_F_ALL_CPUS | BPF_EXIST;
> +	err = bpf_map_update_elem(map_fd, &key, values, flags);
> +	if (!ASSERT_OK(err, "bpf_map_update_elem unnecessary_deletion"))

NIT: unnecessary_deletion  ->  lru_percpu_hash all_cpus|exist

Thanks,
Leon

> +		goto out;
> [...]