Re: [PATCH bpf-next 1/5] selftests/bpf: convert test_get_stack_raw_tp to perf_buffer API

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Song Liu <songliubraving@fb.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 1/5] selftests/bpf: convert test_get_stack_raw_tp to perf_buffer API
Date: Tue, 23 Jul 2019 10:38:51 -0700	[thread overview]
Message-ID: <CAEf4BzYKFTgsf982SEZotZ5+UgP+ErieKXSUoKyj5_gCKrHxTg@mail.gmail.com> (raw)
In-Reply-To: <EBDB05E7-C10F-479F-B2A7-62D59EE4887E@fb.com>

On Tue, Jul 23, 2019 at 2:25 AM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Jul 22, 2019, at 9:31 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Convert test_get_stack_raw_tp test to new perf_buffer API.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > .../bpf/prog_tests/get_stack_raw_tp.c         | 78 ++++++++++---------
> > .../bpf/progs/test_get_stack_rawtp.c          |  2 +-
> > 2 files changed, 44 insertions(+), 36 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> > index c2a0a9d5591b..473889e1b219 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
> > @@ -1,8 +1,15 @@
> > // SPDX-License-Identifier: GPL-2.0
> > +#define _GNU_SOURCE
> > +#include <pthread.h>
> > +#include <sched.h>
> > +#include <sys/socket.h>
> > #include <test_progs.h>
> >
> > #define MAX_CNT_RAWTP 10ull
> > #define MAX_STACK_RAWTP       100
> > +
> > +static int duration = 0;
> > +
>
> Are we using "duration" at all?

Yes, unfortunately in test_progs CHECK macro expects "duration"
variable to be defined. It's very annoying and I'm going to work on
cleaning up and streamlining how we do selftests in bpf, so hopefully
we'll get rid of some of those "artifacts". But for now, yeah,
duration has to be defined somewhere.

>
> > struct get_stack_trace_t {
> >       int pid;
> >       int kern_stack_size;
> > @@ -13,7 +20,7 @@ struct get_stack_trace_t {
> >       struct bpf_stack_build_id user_stack_buildid[MAX_STACK_RAWTP];
> > };
> >
> > -static int get_stack_print_output(void *data, int size)
> > +static void get_stack_print_output(void *ctx, int cpu, void *data, __u32 size)
> > {
> >       bool good_kern_stack = false, good_user_stack = false;
> >       const char *nonjit_func = "___bpf_prog_run";
> > @@ -65,75 +72,76 @@ static int get_stack_print_output(void *data, int size)
> >               if (e->user_stack_size > 0 && e->user_stack_buildid_size > 0)
> >                       good_user_stack = true;
> >       }
> > -     if (!good_kern_stack || !good_user_stack)
> > -             return LIBBPF_PERF_EVENT_ERROR;
> >
> > -     if (cnt == MAX_CNT_RAWTP)
> > -             return LIBBPF_PERF_EVENT_DONE;
> > -
> > -     return LIBBPF_PERF_EVENT_CONT;
> > +     if (!good_kern_stack)
> > +         CHECK(!good_kern_stack, "bad_kern_stack", "bad\n");
>
> Two "bad" is a little weird. How about "kern stack", "bad"?

Heh :) I'll add something more human-readable, like "failed to get
kernel stack".

>
> > +     if (!good_user_stack)
> > +         CHECK(!good_user_stack, "bad_user_stack", "bad\n");
> > }
> >
> > void test_get_stack_raw_tp(void)
> > {

[...]