Re: [PATCH bpf-next v7 08/11] selftests/bpf: Add tests to verify global percpu data

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

On 22/6/26 23:24, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data_init.c b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> index 8466332d7406..ea7e4e3d91cf 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
> 
> [ ... ]
> 
>> @@ -60,3 +63,184 @@ void test_global_data_init(void)
>> +
>> +static void test_global_percpu_data_init(void)
>> +{
>> +	struct test_global_percpu_data__percpu init_value = {};
>> +	struct test_global_percpu_data__percpu *init_data;
>> +	int key, prog_fd, err, num_cpus, num_online, i;
>> +	struct test_global_percpu_data *skel = NULL;
>> +	__u64 args[2] = {0x1234ULL, 0x5678ULL};
>> +	size_t init_data_sz;
>> +	struct bpf_map *map;
>> +	bool *online;
>> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
>> +		    .ctx_in = args,
>> +		    .ctx_size_in = sizeof(args),
>> +		    .flags = BPF_F_TEST_RUN_ON_CPU,
>> +	);
>> +
>> +	num_cpus = libbpf_num_possible_cpus();
>> +	if (!ASSERT_GT(num_cpus, 0, "libbpf_num_possible_cpus"))
>> +		return;
>> +
>> +	err = parse_cpu_mask_file("/sys/devices/system/cpu/online", &online, &num_online);
>> +	if (!ASSERT_OK(err, "parse_cpu_mask_file"))
>> +		return;
> 
> This isn't a bug, but num_cpus is only read by the ASSERT_GT guard and is
> never used afterwards; the per-CPU loop is driven entirely by num_online
> from parse_cpu_mask_file().  Is the libbpf_num_possible_cpus() check needed
> here, or is num_online sufficient?  The same pattern appears in
> test_global_percpu_data_lskel().
> 

Will drop the unused "num_cpus".

> [ ... ]
> 
>> +static void test_global_percpu_data_lskel(void)
>> +{
> 
> [ ... ]
> 
>> +		flags = ((__u64) i << 32) | BPF_F_CPU;
>> +		err = bpf_map_lookup_elem_flags(map_fd, &key, &data, flags);
>> +		if (!ASSERT_OK(err, "bpf_map_lookup_elem_flags"))
>> +			goto out;
>> +
>> +		ASSERT_EQ(data.data, 1, "data.data");
>> +		ASSERT_TRUE(data.run, "data.run");
>> +		ASSERT_EQ(data.nums[6], 0xc0de, "data.nums[6]");
>> +		ASSERT_EQ(data.struct_data.i, 1, "struct_data.i");
>> +		ASSERT_TRUE(data.struct_data.set, "struct_data.set");
>> +		ASSERT_EQ(data.struct_data.nums[6], 0xc0de, "struct_data.nums[6]");
>> +	}
> 
> This isn't a bug, but this post-run assertion block duplicates the one in
> test_global_percpu_data_init() almost verbatim; the two loops share the same
> online-CPU iteration, the same bpf_prog_test_run_opts setup, and the same six
> data.* checks, differing only in the lookup call (bpf_map_lookup_elem_flags()
> vs bpf_map__lookup_elem()).  Could the six data.* checks be factored into a
> shared helper taking the looked-up value?
> 

Good catch.

Will factor out a helper for the whole 'for' block.

Thanks,
Leon