Re: [PATCH bpf-next] Improve BPF test stability (related to perf events and scheduling)

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

On Fri, Feb 18, 2022 at 4:30 PM Mykola Lysenko <mykolal@fb.com> wrote:
>
> In send_signal, replace sleep with dummy cpu intensive computation
> to increase probability of child process being scheduled. Add few
> more asserts.
>
> In find_vma, reduce sample_freq as higher values may be rejected in
> some qemu setups, remove usleep and increase length of cpu intensive
> computation.
>
> In bpf_cookie, perf_link and perf_branches, reduce sample_freq as
> higher values may be rejected in some qemu setups
>
> Signed-off-by: Mykola Lysenko <mykolal@fb.com>
> ---
>  .../testing/selftests/bpf/prog_tests/bpf_cookie.c  |  2 +-
>  tools/testing/selftests/bpf/prog_tests/find_vma.c  |  5 ++---
>  .../selftests/bpf/prog_tests/perf_branches.c       |  4 ++--
>  tools/testing/selftests/bpf/prog_tests/perf_link.c |  2 +-
>  .../testing/selftests/bpf/prog_tests/send_signal.c | 14 ++++++++++----
>  5 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> index cd10df6cd0fc..0612e79a9281 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> @@ -199,7 +199,7 @@ static void pe_subtest(struct test_bpf_cookie *skel)
>         attr.type = PERF_TYPE_SOFTWARE;
>         attr.config = PERF_COUNT_SW_CPU_CLOCK;
>         attr.freq = 1;
> -       attr.sample_freq = 4000;
> +       attr.sample_freq = 1000;
>         pfd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, PERF_FLAG_FD_CLOEXEC);
>         if (!ASSERT_GE(pfd, 0, "perf_fd"))
>                 goto cleanup;
> diff --git a/tools/testing/selftests/bpf/prog_tests/find_vma.c b/tools/testing/selftests/bpf/prog_tests/find_vma.c
> index b74b3c0c555a..acc41223a112 100644
> --- a/tools/testing/selftests/bpf/prog_tests/find_vma.c
> +++ b/tools/testing/selftests/bpf/prog_tests/find_vma.c
> @@ -30,7 +30,7 @@ static int open_pe(void)
>         attr.type = PERF_TYPE_HARDWARE;
>         attr.config = PERF_COUNT_HW_CPU_CYCLES;
>         attr.freq = 1;
> -       attr.sample_freq = 4000;
> +       attr.sample_freq = 1000;
>         pfd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, PERF_FLAG_FD_CLOEXEC);
>
>         return pfd >= 0 ? pfd : -errno;
> @@ -57,7 +57,7 @@ static void test_find_vma_pe(struct find_vma *skel)
>         if (!ASSERT_OK_PTR(link, "attach_perf_event"))
>                 goto cleanup;
>
> -       for (i = 0; i < 1000000; ++i)
> +       for (i = 0; i < 1000000000; ++i)

1bln seems excessive... maybe 10mln would be enough?

>                 ++j;
>
>         test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);

[...]

> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 776916b61c40..841217bd1df6 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -4,11 +4,12 @@
>  #include <sys/resource.h>
>  #include "test_send_signal_kern.skel.h"
>
> -int sigusr1_received = 0;
> +int sigusr1_received;
> +volatile int volatile_variable;

please make them static

>
>  static void sigusr1_handler(int signum)
>  {
> -       sigusr1_received++;
> +       sigusr1_received = 1;
>  }
>
>  static void test_send_signal_common(struct perf_event_attr *attr,
> @@ -42,7 +43,9 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>                 int old_prio;
>
>                 /* install signal handler and notify parent */
> +               errno = 0;
>                 signal(SIGUSR1, sigusr1_handler);
> +               ASSERT_OK(errno, "signal");

just ASSERT_OK(signal(...), "signal");

>
>                 close(pipe_c2p[0]); /* close read */
>                 close(pipe_p2c[1]); /* close write */
> @@ -63,9 +66,12 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>                 ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
>
>                 /* wait a little for signal handler */
> -               sleep(1);
> +               for (int i = 0; i < 1000000000; i++)

same about 1bln

> +                       volatile_variable++;
>
>                 buf[0] = sigusr1_received ? '2' : '0';
> +               ASSERT_EQ(sigusr1_received, 1, "sigusr1_received");
> +
>                 ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
>
>                 /* wait for parent notification and exit */
> @@ -110,9 +116,9 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>         ASSERT_EQ(read(pipe_c2p[0], buf, 1), 1, "pipe_read");
>
>         /* trigger the bpf send_signal */
> +       skel->bss->signal_thread = signal_thread;
>         skel->bss->pid = pid;
>         skel->bss->sig = SIGUSR1;
> -       skel->bss->signal_thread = signal_thread;
>
>         /* notify child that bpf program can send_signal now */
>         ASSERT_EQ(write(pipe_p2c[1], buf, 1), 1, "pipe_write");
> --
> 2.30.2
>