Re: [PATCH bpf-next v2 2/2] tools/bpf: add self tests for bpf_send_signal_thread()

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Jan 14, 2020 at 1:12 PM Yonghong Song <yhs@fb.com> wrote:
>
> The test_progs send_signal() is amended to test
> bpf_send_signal_thread() as well.
>
>    $ ./test_progs -n 40
>    #40/1 send_signal_tracepoint:OK
>    #40/2 send_signal_perf:OK
>    #40/3 send_signal_nmi:OK
>    #40/4 send_signal_tracepoint_thread:OK
>    #40/5 send_signal_perf_thread:OK
>    #40/6 send_signal_nmi_thread:OK
>    #40 send_signal:OK
>    Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED
>
> Also took this opportunity to rewrite the send_signal test
> using skeleton framework and array mmap to make code
> simpler and more readable.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

thanks for converting, see some stuff you don't need to do with skeleton below

>  .../selftests/bpf/prog_tests/send_signal.c    | 111 ++++++++++--------
>  .../bpf/progs/test_send_signal_kern.c         |  51 ++++----
>  2 files changed, 87 insertions(+), 75 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index b607112c64e7..14ec9cf218ed 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <test_progs.h>
> +#include <sys/mman.h>
> +#include "test_send_signal_kern.skel.h"
>
>  static volatile int sigusr1_received = 0;
>
> @@ -8,18 +10,26 @@ static void sigusr1_handler(int signum)
>         sigusr1_received++;
>  }
>
> +static size_t roundup_page(size_t sz)
> +{
> +       long page_size = sysconf(_SC_PAGE_SIZE);
> +       return (sz + page_size - 1) / page_size * page_size;
> +}
> +
>  static void test_send_signal_common(struct perf_event_attr *attr,
> -                                   int prog_type,
> +                                   bool is_tp, bool signal_thread,
>                                     const char *test_name)
>  {
> -       int err = -1, pmu_fd, prog_fd, info_map_fd, status_map_fd;
> -       const char *file = "./test_send_signal_kern.o";
> -       struct bpf_object *obj = NULL;
> +       const size_t bss_sz = roundup_page(sizeof(struct test_send_signal_kern__bss));
> +       struct test_send_signal_kern__bss *bss_data;
> +       struct test_send_signal_kern *skel;
>         int pipe_c2p[2], pipe_p2c[2];
> -       __u32 key = 0, duration = 0;
> +       struct bpf_map *bss_map;
> +       void *bss_mmaped = NULL;
> +       int err = -1, pmu_fd;
> +       __u32 duration = 0;
>         char buf[256];
>         pid_t pid;
> -       __u64 val;
>
>         if (CHECK(pipe(pipe_c2p), test_name,
>                   "pipe pipe_c2p error: %s\n", strerror(errno)))
> @@ -73,45 +83,50 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>         close(pipe_c2p[1]); /* close write */
>         close(pipe_p2c[0]); /* close read */
>
> -       err = bpf_prog_load(file, prog_type, &obj, &prog_fd);
> -       if (CHECK(err < 0, test_name, "bpf_prog_load error: %s\n",
> -                 strerror(errno)))
> -               goto prog_load_failure;
> +       skel = test_send_signal_kern__open_and_load();
> +       if (CHECK(!skel, "skel_open_and_load", "skeleton open_and_load failed\n"))
> +               goto skel_open_load_failure;
> +
> +       bss_map = skel->maps.bss;
> +       bss_mmaped = mmap(NULL, bss_sz, PROT_READ | PROT_WRITE, MAP_SHARED,
> +                         bpf_map__fd(bss_map), 0);
> +       if (CHECK(bss_mmaped == MAP_FAILED, "bss_mmap",
> +                 ".bss mmap failed: %d\n", errno)) {
> +               bss_mmaped = NULL;
> +               goto destroy_skel;
> +       }
> +
> +       bss_data = bss_mmaped;

You don't need to mmap() manually, it's all done automatically as part
of loading skeleton. Just use skel->bss->{pid,sig,signal_thread}
directly.

>
>         pmu_fd = syscall(__NR_perf_event_open, attr, pid, -1,
>                          -1 /* group id */, 0 /* flags */);
>         if (CHECK(pmu_fd < 0, test_name, "perf_event_open error: %s\n",
>                   strerror(errno))) {
>                 err = -1;
> -               goto close_prog;
> +               goto destroy_skel;
>         }
>
> -       err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
> -       if (CHECK(err < 0, test_name, "ioctl perf_event_ioc_enable error: %s\n",
> -                 strerror(errno)))
> -               goto disable_pmu;
> -
> -       err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
> -       if (CHECK(err < 0, test_name, "ioctl perf_event_ioc_set_bpf error: %s\n",
> -                 strerror(errno)))
> -               goto disable_pmu;
> -
> -       err = -1;
> -       info_map_fd = bpf_object__find_map_fd_by_name(obj, "info_map");
> -       if (CHECK(info_map_fd < 0, test_name, "find map %s error\n", "info_map"))
> -               goto disable_pmu;
> -
> -       status_map_fd = bpf_object__find_map_fd_by_name(obj, "status_map");
> -       if (CHECK(status_map_fd < 0, test_name, "find map %s error\n", "status_map"))
> -               goto disable_pmu;
> +       if (is_tp) {
> +               skel->links.send_signal_tp =
> +                       bpf_program__attach_perf_event(skel->progs.send_signal_tp, pmu_fd);
> +               if (CHECK(IS_ERR(skel->links.send_signal_tp), "attach_perf_event",
> +                       "err %ld\n", PTR_ERR(skel->links.send_signal_tp)))
> +                       goto disable_pmu;

tracepoint handler should be loaded automatically, so here you'll be
leaking already create bpf_link and creating a new one.

> +       } else {
> +               skel->links.send_signal_perf =
> +                       bpf_program__attach_perf_event(skel->progs.send_signal_perf, pmu_fd);
> +               if (CHECK(IS_ERR(skel->links.send_signal_perf), "attach_perf_event",
> +                       "err %ld\n", PTR_ERR(skel->links.send_signal_perf)))
> +                       goto disable_pmu;
> +       }
>
>         /* wait until child signal handler installed */
>         read(pipe_c2p[0], buf, 1);
>
>         /* trigger the bpf send_signal */
> -       key = 0;
> -       val = (((__u64)(SIGUSR1)) << 32) | pid;
> -       bpf_map_update_elem(info_map_fd, &key, &val, 0);
> +       bss_data->pid = pid;
> +       bss_data->sig = SIGUSR1;
> +       bss_data->signal_thread = signal_thread;
>
>         /* notify child that bpf program can send_signal now */
>         write(pipe_p2c[1], buf, 1);
> @@ -132,15 +147,15 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>
>  disable_pmu:
>         close(pmu_fd);
> -close_prog:
> -       bpf_object__close(obj);
> -prog_load_failure:
> +destroy_skel:
> +       test_send_signal_kern__destroy(skel);
> +skel_open_load_failure:
>         close(pipe_c2p[0]);
>         close(pipe_p2c[1]);
>         wait(NULL);
>  }
>
> -static void test_send_signal_tracepoint(void)
> +static void test_send_signal_tracepoint(bool signal_thread)
>  {
>         const char *id_path = "/sys/kernel/debug/tracing/events/syscalls/sys_enter_nanosleep/id";

You probably don't need this as well, because tracepoint BPF programs
should be auto-attached.

>         struct perf_event_attr attr = {
> @@ -168,10 +183,10 @@ static void test_send_signal_tracepoint(void)
>
>         attr.config = strtol(buf, NULL, 0);
>
> -       test_send_signal_common(&attr, BPF_PROG_TYPE_TRACEPOINT, "tracepoint");
> +       test_send_signal_common(&attr, true, signal_thread, "tracepoint");
>  }
>

[...]