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

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

On 4/10/26 19:10, 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   | 200 ++++++++++++++++++
>  1 file changed, 200 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..1f33ea20d845 100644
> --- a/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c
> @@ -449,6 +449,48 @@ static void test_lru_percpu_hash_cpu_flag_create(void)
>  	test_percpu_map_cpu_flag_create(BPF_MAP_TYPE_LRU_PERCPU_HASH, 0);
>  }
>  
> +static void test_percpu_cgroup_storage_flags_combination(struct bpf_map *map, int nr_cpus,
> +							  struct bpf_cgroup_storage_key *key)
> +{
> +	int err;
> +	u64 flags = 0;
> +	size_t value_sz = sizeof(u32);
> +	size_t elem_sz = roundup(value_sz, 8);
> +	u32 *values = NULL;

Pls keep these lines in inverted Christmas tree style.

> +
> +	values = calloc(nr_cpus, elem_sz);
> +	if (!ASSERT_OK_PTR(values, "calloc values"))
> +		return;
> +
> +	flags = BPF_NOEXIST | BPF_EXIST;
> +	err = bpf_map__update_elem(map, key, sizeof(*key), values, elem_sz * nr_cpus, flags);
> +	if (!ASSERT_EQ(err, -EINVAL, "bpf_map__update_elem noexist|exist"))
> +		goto out;
> +
> +	flags = BPF_F_ALL_CPUS | BPF_NOEXIST;
> +	err = bpf_map__update_elem(map, key, sizeof(*key), values, value_sz, flags);
> +	if (!ASSERT_EQ(err, -EINVAL, "bpf_map__update_elem all_cpus|noexist"))
> +		goto out;
> +
> +	flags = BPF_F_CPU | BPF_NOEXIST;
> +	err = bpf_map__update_elem(map, key, sizeof(*key), values, value_sz, flags);
> +	if (!ASSERT_EQ(err, -EINVAL, "bpf_map__update_elem cpu|noexist"))
> +		goto out;
> +
> +	flags = BPF_F_ALL_CPUS | BPF_EXIST;
> +	err = bpf_map__update_elem(map, key, sizeof(*key), values, value_sz, flags);
> +	if (!ASSERT_OK(err, "bpf_map__update_elem all_cpus|exist"))
> +		goto out;
> +
> +	flags = BPF_F_CPU | BPF_EXIST;
> +	err = bpf_map__update_elem(map, key, sizeof(*key), values, value_sz, flags);
> +	if (!ASSERT_OK(err, "bpf_map__update_elem cpu|exist"))
> +		goto out;

This 'goto out' is unnecessary.

For these two bpf_map__update_elem() calls, better to set values before
updating, then to verify the values after updating.

> +
> +out:
> +	free(values);
> +}
> +
>  static void test_percpu_cgroup_storage_cpu_flag(void)
>  {
>  	struct percpu_alloc_array *skel = NULL;
> @@ -489,6 +531,7 @@ static void test_percpu_cgroup_storage_cpu_flag(void)
>  		goto out;
>  
>  	test_percpu_map_op_cpu_flag(map, &key, sizeof(key), 1, nr_cpus, false);
> +	test_percpu_cgroup_storage_flags_combination(map, nr_cpus, &key);
>  out:
>  	bpf_prog_detach2(-1, cgroup, BPF_CGROUP_INET_EGRESS);
>  	close(cgroup);
> @@ -537,6 +580,157 @@ static void test_hash_cpu_flag(void)
>  	test_map_op_cpu_flag(BPF_MAP_TYPE_HASH);
>  }
>  
> +static void test_percpu_map_flags_combination(enum bpf_map_type map_type)
> +{
> +	size_t value_sz = 8;
> +	void *values = NULL;
> +	u32 max_entries = 3, key = 0;
> +	u64 flags = 0;
> +	int err, map_fd, nr_cpus;
> +	bool is_hash_map = (map_type == BPF_MAP_TYPE_PERCPU_HASH ||
> +			    map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH);

Ditto the style.

We can pass 'bool is_hash_map' via parameter.

> +
> +	nr_cpus = libbpf_num_possible_cpus();
> +	if (!ASSERT_GT(nr_cpus, 0, "libbpf_num_possible_cpus"))
> +		return;
> +
> +	values = calloc(nr_cpus, value_sz);
> +	if (!ASSERT_OK_PTR(values, "calloc values"))
> +		return;
> +
> +	map_fd = bpf_map_create(map_type, "test_flags_combination_map",
> +				sizeof(u32), value_sz, max_entries, NULL);
> +	if (!ASSERT_GE(map_fd, 0, "bpf_map_create")) {
> +		free(values);
> +		return;
> +	}
> +
> +	flags = BPF_NOEXIST | BPF_EXIST;
> +	err = bpf_map_update_elem(map_fd, &key, values, flags);
> +	if (!ASSERT_EQ(err, -EINVAL, "bpf_map_update_elem noexist|exist"))
> +		goto out;
> +
> +	flags = BPF_F_ALL_CPUS | BPF_NOEXIST;
> +	key = 0;
> +	err = bpf_map_update_elem(map_fd, &key, values, flags);
> +	if (is_hash_map) {
> +		if (!ASSERT_OK(err, "bpf_map_update_elem all_cpus|noexist"))
> +			goto out;
> +
> +		err = bpf_map_update_elem(map_fd, &key, values, flags);
> +		if (!ASSERT_EQ(err, -EEXIST, "bpf_map_update_elem all_cpus|noexist"))
> +			goto out;
> +	} else {
> +		if (!ASSERT_EQ(err, -EEXIST, "bpf_map_update_elem all_cpus|noexist"))
> +			goto out;
> +	}

The 'if (!ASSERT_EQ(err, ...))' can be moved after 'if (is_hash_map)'.

if (is_hash_map) {
	if (!ASSERT_EQ(err, ...))
		goto out;

	err = bpf_map_update_elem(...);
}
if (!ASSERT_EQ(err, ...))
	goto out;

> +
> +	flags = BPF_F_CPU | BPF_NOEXIST;
> +	key = 1;
> +	err = bpf_map_update_elem(map_fd, &key, values, flags);
> +	if (is_hash_map) {
> +		if (!ASSERT_OK(err, "bpf_map_update_elem cpu|noexist"))
> +			goto out;
> +
> +		err = bpf_map_update_elem(map_fd, &key, values, flags);
> +		if (!ASSERT_EQ(err, -EEXIST, "bpf_map_update_elem cpu|noexist"))
> +			goto out;
> +	} else {
> +		if (!ASSERT_EQ(err, -EEXIST, "bpf_map_update_elem cpu|noexist"))
> +			goto out;
> +	}
> +
> +	flags = BPF_F_ALL_CPUS | BPF_EXIST;
> +	key = 2;
> +	err = bpf_map_update_elem(map_fd, &key, values, flags);
> +	if (is_hash_map) {
> +		if (!ASSERT_EQ(err, -ENOENT, "bpf_map_update_elem all_cpus|exist"))
> +			goto out;
> +	} else {
> +		if (!ASSERT_OK(err, "bpf_map_update_elem all_cpus|exist"))
> +			goto out;
> +	}
> +
> +	flags = BPF_F_CPU | BPF_EXIST;
> +	err = bpf_map_update_elem(map_fd, &key, values, flags);
> +	if (is_hash_map) {
> +		if (!ASSERT_EQ(err, -ENOENT, "bpf_map_update_elem cpu|exist"))
> +			goto out;
> +	} else {
> +		if (!ASSERT_OK(err, "bpf_map_update_elem cpu|exist"))
> +			goto out;
> +	}
> +
> +	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"))
> +		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"))
> +		goto out;
> +
> +	key = 0;
> +	err = bpf_map_lookup_elem(map_fd, &key, values);
> +	if (!ASSERT_OK(err, "bpf_map_lookup_elem unnecessary_deletion"))
> +		goto out;

This 'goto' is unnecessary.

> +
> +out:
> +	close(map_fd);
> +	free(values);
> +}

After reading test_percpu_map_flags_combination(), two helpers are
needed to verify the updated values. One for updating with BPF_F_CPU,
another one for updating with BPF_F_ALL_CPUS.

Thanks,
Leon

> [...]