On Fri, Feb 7, 2025 at 2:00 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
>
>
> 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
> >>
[...]
> >> +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?
we already have custom skeleton-specific type for .data, .rodata,
.bss, so we should provide one for .percpu as well, yes
>
> > 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).
It's hacky, and it won't work correctly on big-endian architectures.
But you shouldn't need that if we have a struct representing this
.percpu memory image. Just make sure that struct has 8 byte alignment
(from bpftool side during skeleton generation, probably).
[...]
> > 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()?
>
No, look at `cpu` field of `struct bpf_test_run_opts`. We should have
a selftest using it, so you can work backwards from that.
> 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
>
>
