On Thu, Oct 20, 2022 at 9:07 AM Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
>
> Add a test_ringbuf_map_key test prog, borrowing heavily from extant
> test_ringbuf.c. The program tries to use the result of
> bpf_ringbuf_reserve as map_key, which was not possible before previouis
> commits in this series. The test runner added to prog_tests/ringbuf.c
> verifies that the program loads and does basic sanity checks to confirm
> that it runs as expected.
>
> Also, refactor test_ringbuf such that runners for existing test_ringbuf
> and newly-added test_ringbuf_map_key are subtests of 'ringbuf' top-level
> test.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> Acked-by: Yonghong Song <yhs@xxxxxx>
> ---
LGTM.
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> v4->v5: lore.kernel.org/bpf/20220923060614.4025371-2-davemarchevsky@xxxxxx
>
> * Fix some nits (Andrii)
> * migrating prog from fentry -> ksyscall wasn't done as lskel doesn't
> support the latter. Talked to Andrii about it offlist, he's fine with it.
>
> v3->v4: lore.kernel.org/bpf/20220922142208.3009672-2-davemarchevsky@xxxxxx
>
> * Fix some nits (Yonghong)
> * make subtest runner functions static
> * don't goto cleanup if -EDONE check fails
> * add 'workaround' to comment in test to ease future grepping
> * Add Yonghong ack
>
> v2->v3: lore.kernel.org/bpf/20220914123600.927632-2-davemarchevsky@xxxxxx
>
> * Test that ring_buffer__poll returns -EDONE (Alexei)
>
> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@xxxxxx
>
> * Actually run the program instead of just loading (Yonghong)
> * Add a bpf_map_update_elem call to the test (Yonghong)
> * Refactor runner such that existing test and newly-added test are
> subtests of 'ringbuf' top-level test (Yonghong)
> * Remove unused globals in test prog (Yonghong)
>
> tools/testing/selftests/bpf/Makefile | 8 ++-
> .../selftests/bpf/prog_tests/ringbuf.c | 66 ++++++++++++++++-
> .../bpf/progs/test_ringbuf_map_key.c | 70 +++++++++++++++++++
> 3 files changed, 140 insertions(+), 4 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e6cf21fad69f..79edef1dbda4 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -359,9 +359,11 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \
> test_subskeleton.skel.h test_subskeleton_lib.skel.h \
> test_usdt.skel.h
[...]
> diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> new file mode 100644
> index 000000000000..2760bf60d05a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct sample {
> + int pid;
> + int seq;
> + long value;
> + char comm[16];
> +};
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_RINGBUF);
btw, libbpf is smart enough now to auto-fix ringbuf size, so you could
have used __uint(max_entries, 4096) and that would work even on
architectures that have 64KB pages. Just FYI.
> +} ringbuf SEC(".maps");
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_HASH);
> + __uint(max_entries, 1000);
> + __type(key, struct sample);
> + __type(value, int);
> +} hash_map SEC(".maps");
> +
> +/* inputs */
> +int pid = 0;
> +
> +/* inner state */
> +long seq = 0;
> +
> +SEC("fentry/" SYS_PREFIX "sys_getpgid")
it's fine as is, my suggestion to use ksyscall was to 1) avoid using
BPF trampoline (and so have these tests work on s390x) and 2) no have
to use ugly SYS_PREFIX. SEC("kprobe/" SYS_PREFIX "sys_getpgid") would
solve 1), which is more important in practical terms. 2) is a wishlist
:)
I'm not insisting or asking to change this, just pointing out the
rationale for ksyscall suggestion in the first place.
> +int test_ringbuf_mem_map_key(void *ctx)
> +{
> + int cur_pid = bpf_get_current_pid_tgid() >> 32;
> + struct sample *sample, sample_copy;
> + int *lookup_val;
> +
> + if (cur_pid != pid)
> + return 0;
> +
> + sample = bpf_ringbuf_reserve(&ringbuf, sizeof(*sample), 0);
> + if (!sample)
> + return 0;
> +
> + sample->pid = pid;
> + bpf_get_current_comm(sample->comm, sizeof(sample->comm));
> + sample->seq = ++seq;
> + sample->value = 42;
> +
> + /* test using 'sample' (PTR_TO_MEM | MEM_ALLOC) as map key arg
> + */
> + lookup_val = (int *)bpf_map_lookup_elem(&hash_map, sample);
> +
> + /* workaround - memcpy is necessary so that verifier doesn't
> + * complain with:
> + * verifier internal error: more than one arg with ref_obj_id R3
> + * when trying to do bpf_map_update_elem(&hash_map, sample, &sample->seq, BPF_ANY);
> + *
> + * Since bpf_map_lookup_elem above uses 'sample' as key, test using
> + * sample field as value below
> + */
> + __builtin_memcpy(&sample_copy, sample, sizeof(struct sample));
> + bpf_map_update_elem(&hash_map, &sample_copy, &sample->seq, BPF_ANY);
> +
> + bpf_ringbuf_submit(sample, 0);
> + return 0;
> +}
> --
> 2.30.2
>
