Re: [PATCH bpf-next v12 07/10] selftests/bpf: Add tests to verify global percpu data

On 13/8/26 23:42, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The test `test_global_percpu_data` fails because `bpf_map__set_value_size()` clears the `.percpu` map's BTF type ID when it encounters a non-array as the last variable in the datasec.
> --
> 
> --- Patch [7]: [PATCH bpf-next v12 07/10] selftests/bpf: Add tests to verify global percpu data ---
> commit 0224e1ae61dc47271ec8a72ac29b073935a25d31
> Author: Leon Hwang <[email protected]>
> 
> selftests/bpf: Add tests to verify global percpu data
> 
> This patch adds a selftest to verify global percpu data map initialization
> and modification using APIs like bpf_map__set_value_size() and
> bpf_map_lookup_elem_flags(). It includes both light and standard skeleton
> tests.
> 
>> 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 8466332d7406f..06d163a022dcc 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
> [ ... ]
>> +static void test_global_percpu_data_init(void)
>> +{
> [ ... ]
>> +	err = bpf_map__set_value_size(map, desired_sz);
>> +	if (!ASSERT_OK(err, "bpf_map__set_value_size"))
>> +		goto out;
>> +	if (!ASSERT_EQ(bpf_map__value_size(map), desired_sz, "percpu value size"))
>> +		goto out;
>> +	if (!ASSERT_NEQ(bpf_map__btf_value_type_id(map), 0, "percpu BTF value type"))
>> +		goto out;
> 
> [Severity: Medium]
> Will this test fail here because bpf_map__set_value_size() clears the BTF
> type ID? 
> 
> When bpf_map__set_value_size() is called, it attempts to resize the BTF
> datasec. This requires the last variable in the datasec to be an array.
> Because `struct_data` is defined last in the `.percpu` section in the BPF
> program, map_btf_datasec_resize() returns -EINVAL and clears the BTF type
> ID to 0, causing this assertion to fail.
> 
>> diff --git a/tools/testing/selftests/bpf/progs/test_global_percpu_data.c b/tools/testing/selftests/bpf/progs/test_global_percpu_data.c
>> new file mode 100644
>> index 0000000000000..416841cd35692
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_global_percpu_data.c
> [ ... ]
>> +int nums[7] SEC(".percpu");
>> +bool set SEC(".percpu") = false;
>> +struct {
>> +	char set;
>> +	int i;
>> +	int nums[7];
>> +} struct_data SEC(".percpu") = {
>> +	.set = 0,
>> +	.i = -1,
>> +};
> 
> [Severity: Medium]
> Does placing this struct at the end of the `.percpu` section break the
> BTF resizing logic?
> 
> Could the variables in the `.percpu` section be reordered so that an array
> like `nums` or `cpu_id` is the last variable?
> 

I think I should use a dedicated section to exercise
bpf_map__set_value_size(), e.g.

int arr[1] SEC(".percpu.arr");

Hence, bpf_map__set_value_size() won't fail with new value size
'sizeof(int) * 2'.

Thanks,
Leon