On 22/6/26 23:24, [email protected] 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
