On 6/2/25 08:09, Andrii Nakryiko wrote:
> On Mon, Jan 27, 2025 at 8:22 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>>
>> If the arch, like s390x, does not support percpu insn, this case won't
>> test global percpu data by checking -EOPNOTSUPP when load prog.
>>
>> The following APIs have been tested for global percpu data:
>> 1. bpf_map__set_initial_value()
>> 2. bpf_map__initial_value()
>> 3. generated percpu struct pointer that points to internal map's data
>> 4. bpf_map__lookup_elem() for global percpu data map
>>
>> cd tools/testing/selftests/bpf; ./test_progs -t global_percpu_data
>> 124 global_percpu_data_init:OK
>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Signed-off-by: Leon Hwang <leon.hwang@xxxxxxxxx>
>> ---
>> .../bpf/prog_tests/global_data_init.c | 89 ++++++++++++++++++-
>> .../bpf/progs/test_global_percpu_data.c | 21 +++++
>> 2 files changed, 109 insertions(+), 1 deletion(-)
>> create mode 100644 tools/testing/selftests/bpf/progs/test_global_percpu_data.c
>>
>> 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..a5d0890444f67 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0
>> #include <test_progs.h>
>> +#include "test_global_percpu_data.skel.h"
>>
>> void test_global_data_init(void)
>> {
>> @@ -8,7 +9,7 @@ void test_global_data_init(void)
>> __u8 *buff = NULL, *newval = NULL;
>> struct bpf_object *obj;
>> struct bpf_map *map;
>> - __u32 duration = 0;
>> + __u32 duration = 0;
>> size_t sz;
>>
>> obj = bpf_object__open_file(file, NULL);
>> @@ -60,3 +61,89 @@ void test_global_data_init(void)
>> free(newval);
>> bpf_object__close(obj);
>> }
>> +
>> +void test_global_percpu_data_init(void)
>> +{
>> + struct test_global_percpu_data *skel = NULL;
>> + u64 *percpu_data = NULL;
>
> there is that test_global_percpu_data__percpu type you are declaring
> in the BPF skeleton, right? We should try using it here.
>
No. bpftool does not generate test_global_percpu_data__percpu. The
struct for global variables is embedded into skeleton struct.
Should we generate type for global variables?
> And for that array access, we should make sure that it's __aligned(8),
> so indexing by CPU index works correctly.
>
Ack.
> Also, you define per-CPU variable as int, but here it is u64, what's
> up with that?
>
Like __aligned(8), it's to make sure 8-bytes aligned. It's better to use
__aligned(8).
>> + struct bpf_map *map;
>> + size_t init_data_sz;
>> + char buff[128] = {};
>> + int init_value = 2;
>> + int key, value_sz;
>> + int prog_fd, err;
>> + int *init_data;
>> + int num_cpus;
>> +
>
> nit: LIBBPF_OPTS below is variable declaration, so there shouldn't be
> an empty line here (and maybe group those int variables a bit more
> tightly?)
>
Ack.
>> + LIBBPF_OPTS(bpf_test_run_opts, topts,
>> + .data_in = buff,
>> + .data_size_in = sizeof(buff),
>> + .repeat = 1,
>> + );
>> +
>> + num_cpus = libbpf_num_possible_cpus();
>> + if (!ASSERT_GT(num_cpus, 0, "libbpf_num_possible_cpus"))
>> + return;
>> +
>> + percpu_data = calloc(num_cpus, sizeof(*percpu_data));
>> + if (!ASSERT_FALSE(percpu_data == NULL, "calloc percpu_data"))
>
> ASSERT_OK_PTR()
>
Ack.
>> + return;
>> +
>> + value_sz = sizeof(*percpu_data) * num_cpus;
>> + memset(percpu_data, 0, value_sz);
>
> you calloc()'ed it, it's already zero-initialized
>
Ack. Thanks. I should check "man calloc" to use it.
>
>> +
>> + skel = test_global_percpu_data__open();
>> + if (!ASSERT_OK_PTR(skel, "test_global_percpu_data__open"))
>> + goto out;
>> +
>> + ASSERT_EQ(skel->percpu->percpu_data, -1, "skel->percpu->percpu_data");
>> +
>> + map = skel->maps.percpu;
>> + err = bpf_map__set_initial_value(map, &init_value,
>> + sizeof(init_value));
>> + if (!ASSERT_OK(err, "bpf_map__set_initial_value"))
>> + goto out;
>> +
>> + init_data = bpf_map__initial_value(map, &init_data_sz);
>> + if (!ASSERT_OK_PTR(init_data, "bpf_map__initial_value"))
>> + goto out;
>> +
>> + ASSERT_EQ(*init_data, init_value, "initial_value");
>> + ASSERT_EQ(init_data_sz, sizeof(init_value), "initial_value size");
>> +
>> + if (!ASSERT_EQ((void *) init_data, (void *) skel->percpu, "skel->percpu"))
>> + goto out;
>> + ASSERT_EQ(skel->percpu->percpu_data, init_value, "skel->percpu->percpu_data");
>> +
>> + err = test_global_percpu_data__load(skel);
>> + if (err == -EOPNOTSUPP)
>> + goto out;
>> + if (!ASSERT_OK(err, "test_global_percpu_data__load"))
>> + goto out;
>> +
>> + ASSERT_EQ(bpf_map__type(map), BPF_MAP_TYPE_PERCPU_ARRAY,
>> + "bpf_map__type");
>> +
>> + prog_fd = bpf_program__fd(skel->progs.update_percpu_data);
>> + err = bpf_prog_test_run_opts(prog_fd, &topts);
>
> at least one of BPF programs (don't remember which one, could be
> raw_tp) supports specifying CPU index to run on, it would be nice to
> loop over CPUs, triggering BPF program on each one and filling per-CPU
> variable with current CPU index. Then we can check that all per-CPU
> values have expected values.
>
Do you mean prog_tests/perf_buffer.c::trigger_on_cpu()?
Your suggestion looks good to me. I'll do it.
>
>> + ASSERT_OK(err, "update_percpu_data");
>> + ASSERT_EQ(topts.retval, 0, "update_percpu_data retval");
>> +
>> + key = 0;
>> + err = bpf_map__lookup_elem(map, &key, sizeof(key), percpu_data,
>> + value_sz, 0);
>> + if (!ASSERT_OK(err, "bpf_map__lookup_elem"))
>> + goto out;
>> +
[...]
Thanks,
Leon
