Re: [PATCH bpf-next v10 6/9] selftests/bpf: Add tests to verify global percpu data

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

On 18/7/26 06:47, Emil Tsalapatis wrote:
> On Wed Jul 15, 2026 at 11:32 AM EDT, Leon Hwang wrote:

[...]

>> +
>> +static void test_percpu_data_on_cpus(int map_fd, int prog_fd)
>> +{
>> +	__u64 args[2] = {0x1234ULL, 0x5678ULL};
>> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
>> +		    .ctx_in = args,
>> +		    .ctx_size_in = sizeof(args),
>> +		    .flags = BPF_F_TEST_RUN_ON_CPU,
>> +	);
>> +	int i, err, key = 0, num_online;
>> +	bool *online;
>> +
>> +	err = parse_cpu_mask_file("/sys/devices/system/cpu/online", &online, &num_online);
>> +	if (!ASSERT_OK(err, "parse_cpu_mask_file"))
>> +		return;
>> +
>> +	/* run on every online-CPU */
>> +	for (i = 0; i < num_online; i++) {
>> +		struct test_global_percpu_data__percpu data = {};
>> +		__u64 flags;
>> +
>> +		if (!online[i])
>> +			continue;
>> +
>> +		topts.cpu = i;
>> +		topts.retval = -1;
>> +		err = bpf_prog_test_run_opts(prog_fd, &topts);
>> +		ASSERT_OK(err, "bpf_prog_test_run_opts");
>> +		ASSERT_EQ(topts.retval, 0, "bpf_prog_test_run_opts retval");
>> +
>> +		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"))
>> +			break;
>> +
>> +		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]");
> 
> Can we add a pre-run assert to ensure that the per-cpu data has not
> already been modified by another run? Can we also add some cpuid
> specific assignment to ensure the runs are done on the proper CPU?


Ack.

> 
>> +	}
>> +
>> +	free(online);
>> +}
>> +

[...]

>> diff --git a/tools/testing/selftests/bpf/prog_tests/global_percpu_subskel.c b/tools/testing/selftests/bpf/prog_tests/global_percpu_subskel.c
>> new file mode 100644
>> index 000000000000..8aebd533d86b
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/global_percpu_subskel.c
>> @@ -0,0 +1,37 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include <test_progs.h>
>> +#include "test_global_percpu_data.subskel.h"
>> +
>> +void test_global_percpu_data_subskel(void)
>> +{
>> +	struct test_global_percpu_data *subskel = NULL;
>> +	struct bpf_object *obj;
>> +	int i;
>> +
>> +	obj = bpf_object__open_file("./test_global_percpu_data.bpf.o", NULL);
>> +	if (!ASSERT_OK_PTR(obj, "bpf_object__open_file"))
>> +		return;
>> +
>> +	subskel = test_global_percpu_data__open(obj);
>> +	if (!ASSERT_OK_PTR(subskel, "test_global_percpu_data__open"))
>> +		goto out;
>> +
>> +	if (!ASSERT_OK_PTR(subskel->subskel, "subskel"))
>> +		goto out;
>> +	if (!ASSERT_OK_PTR(subskel->maps.percpu, "maps.percpu"))
>> +		goto out;
>> +	ASSERT_EQ(bpf_map__type(subskel->maps.percpu), BPF_MAP_TYPE_PERCPU_ARRAY,
>> +		  "percpu_map_type");
> 
> Not sure why these assertions would be necessary, wouldn't the test crash or
> quickly fail if they didn't hold?
> 
>> +	ASSERT_GT(subskel->subskel->var_cnt, 0, "var_cnt");
>> +
>> +	for (i = 0; i < subskel->subskel->var_cnt; i++) {
>> +		const struct bpf_var_skeleton *var;
>> +
>> +		var = (void *) subskel->subskel->vars + i * subskel->subskel->var_skel_sz;
>> +		ASSERT_NEQ(var->map, &subskel->maps.percpu, "var");
>> +	}
> 
> If we turn the substest to SYSCALL instead of TRACEPOINT we can read
> and report the return value as the test happens and keep them
> self-contained.


It is to verify that the bpftool-generated subskeleton should not
contain the global percpu variables.

I think I should test it manually by checking the generated .subskel.h.

Will drop this subskel test.

> 
>> +
>> +out:
>> +	test_global_percpu_data__destroy(subskel);
>> +	bpf_object__close(obj);
>> +}
>> 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 000000000000..54380dfb11a5
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_global_percpu_data.c
>> @@ -0,0 +1,33 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include "bpf_misc.h"
>> +
>> +int unused SEC(".percpu.looooooooong");
>> +int data2 SEC(".percpu.data");
>> +int data SEC(".percpu") = -1;
>> +int nums[7] SEC(".percpu");
>> +char run SEC(".percpu") = 0;
> 
> Can we add a comment that those are used to test the names in the
> userspace portion of the test? As they stand they seem unused.

Ack.

> 
>> +struct {
>> +	char set;
>> +	int i;
>> +	int nums[7];
>> +} struct_data SEC(".percpu") = {
>> +	.set = 0,
>> +	.i = -1,
>> +};
>> +
>> +SEC("raw_tp/task_rename")
> 
> Is this actually installed as a tracepoint? I think we run it as a
> program. Is putting it in raw_tp doing anything in any way?


Will drop '/task_rename'.

Thanks,
Leon

> 
>> +__auxiliary
>> +int update_percpu_data(void *ctx)
>> +{
>> +	struct_data.nums[6] = 0xc0de;
>> +	struct_data.set = 1;
>> +	struct_data.i = 1;
>> +	nums[6] = 0xc0de;
>> +	data = 1;
>> +	run = 1;
>> +	return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>