Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Sep 14, 2022 at 1:36 PM Dave Marchevsky <davemarchevsky@fb.com> 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@fb.com>
> ---
> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
>
> * 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        | 63 ++++++++++++++++-
>  .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>  3 files changed, 137 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 6cd327f1f216..231d9c1364c9 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -351,9 +351,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
>
> -LSKELS := fentry_test.c fexit_test.c fexit_sleep.c \
> -       test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
> -       map_ptr_kern.c core_kern.c core_kern_overflow.c
> +LSKELS := fentry_test.c fexit_test.c fexit_sleep.c atomics.c           \
> +       trace_printk.c trace_vprintk.c map_ptr_kern.c                   \
> +       core_kern.c core_kern_overflow.c test_ringbuf.c                 \
> +       test_ringbuf_map_key.c
> +
>  # Generate both light skeleton and libbpf skeleton for these
>  LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \
>         kfunc_call_test_subprog.c
> diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> index 9a80fe8a6427..e0f8db69cb77 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> @@ -13,6 +13,7 @@
>  #include <linux/perf_event.h>
>  #include <linux/ring_buffer.h>
>  #include "test_ringbuf.lskel.h"
> +#include "test_ringbuf_map_key.lskel.h"
>
>  #define EDONE 7777
>
> @@ -58,6 +59,7 @@ static int process_sample(void *ctx, void *data, size_t len)
>         }
>  }
>
> +static struct test_ringbuf_map_key_lskel *skel_map_key;
>  static struct test_ringbuf_lskel *skel;
>  static struct ring_buffer *ringbuf;
>
> @@ -81,7 +83,7 @@ static void *poll_thread(void *input)
>         return (void *)(long)ring_buffer__poll(ringbuf, timeout);
>  }
>
> -void test_ringbuf(void)
> +void ringbuf_subtest(void)
>  {
>         const size_t rec_sz = BPF_RINGBUF_HDR_SZ + sizeof(struct sample);
>         pthread_t thread;
> @@ -297,3 +299,62 @@ void test_ringbuf(void)
>         ring_buffer__free(ringbuf);
>         test_ringbuf_lskel__destroy(skel);
>  }
> +
> +static int process_map_key_sample(void *ctx, void *data, size_t len)
> +{
> +       struct sample *s;
> +       int err, val;
> +
> +       s = data;
> +       switch (s->seq) {
> +       case 1:
> +               ASSERT_EQ(s->value, 42, "sample_value");
> +               err = bpf_map_lookup_elem(skel_map_key->maps.hash_map.map_fd,
> +                                         s, &val);
> +               ASSERT_OK(err, "hash_map bpf_map_lookup_elem");
> +               ASSERT_EQ(val, 1, "hash_map val");
> +               return -EDONE;
> +       default:
> +               return 0;
> +       }
> +}
> +
> +void ringbuf_map_key_subtest(void)
> +{
> +       int err;
> +
> +       skel_map_key = test_ringbuf_map_key_lskel__open();
> +       if (!ASSERT_OK_PTR(skel_map_key, "test_ringbuf_map_key_lskel__open"))
> +               return;
> +
> +       skel_map_key->maps.ringbuf.max_entries = getpagesize();
> +       skel_map_key->bss->pid = getpid();
> +
> +       err = test_ringbuf_map_key_lskel__load(skel_map_key);
> +       if (!ASSERT_OK(err, "test_ringbuf_map_key_lskel__load"))
> +               goto cleanup;
> +
> +       ringbuf = ring_buffer__new(skel_map_key->maps.ringbuf.map_fd,
> +                                  process_map_key_sample, NULL, NULL);
> +
> +       err = test_ringbuf_map_key_lskel__attach(skel_map_key);
> +       if (!ASSERT_OK(err, "test_ringbuf_map_key_lskel__attach"))
> +               goto cleanup_ringbuf;
> +
> +       syscall(__NR_getpgid);
> +       ASSERT_EQ(skel_map_key->bss->seq, 1, "skel_map_key->bss->seq");
> +       ring_buffer__poll(ringbuf, -1);

Why is there no err == EDONE check here?
Without the check the prog could have skipped
ringbuf_submit and process_map_key_sample() above would not
be called.