| Message ID | 20220907050407.2711513-1-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] perf test: Skip sigtrap test on old kernels | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Sep 6, 2022 at 10:04 PM Namhyung Kim <namhyung@kernel.org> wrote: > > If it runs on an old kernel, perf_event_open would fail because of the > new fields sigtrap and sig_data. Just skipping the test could miss an > actual bug in the kernel. > > Let's check BTF if it has the perf_event_attr.sigtrap field. > > Cc: Marco Elver <elver@google.com> > Cc: Song Liu <songliubraving@fb.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/tests/sigtrap.c | 46 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c > index e32ece90e164..32f08ce0f2b0 100644 > --- a/tools/perf/tests/sigtrap.c > +++ b/tools/perf/tests/sigtrap.c > @@ -16,6 +16,8 @@ > #include <sys/syscall.h> > #include <unistd.h> > > +#include <bpf/btf.h> > + Do we need "#ifdef HAVE_BPF_SKEL" for the include part? Other than this, looks good to me. Acked-by: Song Liu <song@kernel.org> > #include "cloexec.h" > #include "debug.h" > #include "event.h" > @@ -54,6 +56,42 @@ static struct perf_event_attr make_event_attr(void) > return attr; > } > > +static bool attr_has_sigtrap(void) > +{ > + bool ret = false; > + > +#ifdef HAVE_BPF_SKEL > + > + struct btf *btf; > + const struct btf_type *t; > + const struct btf_member *m; > + const char *name; > + int i, id; > + > + /* just assume it doesn't have the field */ > + btf = btf__load_vmlinux_btf(); > + if (btf == NULL) > + return false; > + > + id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); > + if (id < 0) > + goto out; > + > + t = btf__type_by_id(btf, id); > + for (i = 0, m = btf_members(t); i < btf_vlen(t); i++, m++) { > + name = btf__name_by_offset(btf, m->name_off); > + if (!strcmp(name, "sigtrap")) { > + ret = true; > + break; > + } > + } > +out: > + btf__free(btf); > +#endif > + > + return ret; > +} > + > static void > sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __maybe_unused) > { > @@ -139,7 +177,13 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); > if (fd < 0) { > - pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf))); > + if (attr_has_sigtrap()) { > + pr_debug("FAILED sys_perf_event_open(): %s\n", > + str_error_r(errno, sbuf, sizeof(sbuf))); > + } else { > + pr_debug("perf_event_attr doesn't have sigtrap\n"); > + ret = TEST_SKIP; > + } > goto out_restore_sigaction; > } > > -- > 2.37.2.789.g6183377224-goog >
Hi Song, On Tue, Sep 6, 2022 at 10:58 PM Song Liu <song@kernel.org> wrote: > > On Tue, Sep 6, 2022 at 10:04 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > If it runs on an old kernel, perf_event_open would fail because of the > > new fields sigtrap and sig_data. Just skipping the test could miss an > > actual bug in the kernel. > > > > Let's check BTF if it has the perf_event_attr.sigtrap field. > > > > Cc: Marco Elver <elver@google.com> > > Cc: Song Liu <songliubraving@fb.com> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > tools/perf/tests/sigtrap.c | 46 +++++++++++++++++++++++++++++++++++++- > > 1 file changed, 45 insertions(+), 1 deletion(-) > > > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c > > index e32ece90e164..32f08ce0f2b0 100644 > > --- a/tools/perf/tests/sigtrap.c > > +++ b/tools/perf/tests/sigtrap.c > > @@ -16,6 +16,8 @@ > > #include <sys/syscall.h> > > #include <unistd.h> > > > > +#include <bpf/btf.h> > > + > > Do we need "#ifdef HAVE_BPF_SKEL" for the include part? Right, it'd be better to move it under the #ifdef. Will change. > > Other than this, looks good to me. > > Acked-by: Song Liu <song@kernel.org> Thanks for the review! Namhyung
On Tue, Sep 06, 2022 at 10:04:07PM -0700, Namhyung Kim wrote: > If it runs on an old kernel, perf_event_open would fail because of the > new fields sigtrap and sig_data. Just skipping the test could miss an > actual bug in the kernel. > > Let's check BTF if it has the perf_event_attr.sigtrap field. > > Cc: Marco Elver <elver@google.com> > Cc: Song Liu <songliubraving@fb.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/tests/sigtrap.c | 46 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c > index e32ece90e164..32f08ce0f2b0 100644 > --- a/tools/perf/tests/sigtrap.c > +++ b/tools/perf/tests/sigtrap.c > @@ -16,6 +16,8 @@ > #include <sys/syscall.h> > #include <unistd.h> > > +#include <bpf/btf.h> > + > #include "cloexec.h" > #include "debug.h" > #include "event.h" > @@ -54,6 +56,42 @@ static struct perf_event_attr make_event_attr(void) > return attr; > } > > +static bool attr_has_sigtrap(void) > +{ > + bool ret = false; > + > +#ifdef HAVE_BPF_SKEL > + > + struct btf *btf; > + const struct btf_type *t; > + const struct btf_member *m; > + const char *name; > + int i, id; > + > + /* just assume it doesn't have the field */ > + btf = btf__load_vmlinux_btf(); > + if (btf == NULL) > + return false; > + > + id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); > + if (id < 0) > + goto out; > + > + t = btf__type_by_id(btf, id); > + for (i = 0, m = btf_members(t); i < btf_vlen(t); i++, m++) { > + name = btf__name_by_offset(btf, m->name_off); > + if (!strcmp(name, "sigtrap")) { > + ret = true; > + break; > + } > + } > +out: > + btf__free(btf); > +#endif would it be easier to call perf_event_open for simple event with sigtrap set (and perhaps remove_on_exec) ? perf_copy_attr checks on reserved fields jirka > + > + return ret; > +} > + > static void > sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __maybe_unused) > { > @@ -139,7 +177,13 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); > if (fd < 0) { > - pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf))); > + if (attr_has_sigtrap()) { > + pr_debug("FAILED sys_perf_event_open(): %s\n", > + str_error_r(errno, sbuf, sizeof(sbuf))); > + } else { > + pr_debug("perf_event_attr doesn't have sigtrap\n"); > + ret = TEST_SKIP; > + } > goto out_restore_sigaction; > } > > -- > 2.37.2.789.g6183377224-goog >
On Wed, Sep 7, 2022 at 1:32 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Tue, Sep 06, 2022 at 10:04:07PM -0700, Namhyung Kim wrote: > > If it runs on an old kernel, perf_event_open would fail because of the > > new fields sigtrap and sig_data. Just skipping the test could miss an > > actual bug in the kernel. > > > > Let's check BTF if it has the perf_event_attr.sigtrap field. > > > > Cc: Marco Elver <elver@google.com> > > Cc: Song Liu <songliubraving@fb.com> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > tools/perf/tests/sigtrap.c | 46 +++++++++++++++++++++++++++++++++++++- > > 1 file changed, 45 insertions(+), 1 deletion(-) > > > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c > > index e32ece90e164..32f08ce0f2b0 100644 > > --- a/tools/perf/tests/sigtrap.c > > +++ b/tools/perf/tests/sigtrap.c > > @@ -16,6 +16,8 @@ > > #include <sys/syscall.h> > > #include <unistd.h> > > > > +#include <bpf/btf.h> > > + > > #include "cloexec.h" > > #include "debug.h" > > #include "event.h" > > @@ -54,6 +56,42 @@ static struct perf_event_attr make_event_attr(void) > > return attr; > > } > > > > +static bool attr_has_sigtrap(void) > > +{ > > + bool ret = false; > > + > > +#ifdef HAVE_BPF_SKEL > > + > > + struct btf *btf; > > + const struct btf_type *t; > > + const struct btf_member *m; > > + const char *name; > > + int i, id; > > + > > + /* just assume it doesn't have the field */ > > + btf = btf__load_vmlinux_btf(); > > + if (btf == NULL) > > + return false; > > + > > + id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); > > + if (id < 0) > > + goto out; > > + > > + t = btf__type_by_id(btf, id); > > + for (i = 0, m = btf_members(t); i < btf_vlen(t); i++, m++) { > > + name = btf__name_by_offset(btf, m->name_off); > > + if (!strcmp(name, "sigtrap")) { > > + ret = true; > > + break; > > + } > > + } > > +out: > > + btf__free(btf); > > +#endif > > would it be easier to call perf_event_open for simple event with > sigtrap set (and perhaps remove_on_exec) ? perf_copy_attr checks > on reserved fields Hmm.. right. we could do that too. But it might still fail if there's a bug in the path handling in sigtrap even for the simple case. I'm not sure if it's a realistic concern though. :) Thanks, Namhyung > > > > + > > + return ret; > > +} > > + > > static void > > sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __maybe_unused) > > { > > @@ -139,7 +177,13 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m > > > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); > > if (fd < 0) { > > - pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf))); > > + if (attr_has_sigtrap()) { > > + pr_debug("FAILED sys_perf_event_open(): %s\n", > > + str_error_r(errno, sbuf, sizeof(sbuf))); > > + } else { > > + pr_debug("perf_event_attr doesn't have sigtrap\n"); > > + ret = TEST_SKIP; > > + } > > goto out_restore_sigaction; > > } > > > > -- > > 2.37.2.789.g6183377224-goog > >
On Wed, Sep 7, 2022 at 10:50 AM Namhyung Kim <namhyung@kernel.org> wrote: > > On Wed, Sep 7, 2022 at 1:32 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Tue, Sep 06, 2022 at 10:04:07PM -0700, Namhyung Kim wrote: > > > If it runs on an old kernel, perf_event_open would fail because of the > > > new fields sigtrap and sig_data. Just skipping the test could miss an > > > actual bug in the kernel. > > > > > > Let's check BTF if it has the perf_event_attr.sigtrap field. > > > > > > Cc: Marco Elver <elver@google.com> > > > Cc: Song Liu <songliubraving@fb.com> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > > --- > > > tools/perf/tests/sigtrap.c | 46 +++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 45 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c > > > index e32ece90e164..32f08ce0f2b0 100644 > > > --- a/tools/perf/tests/sigtrap.c > > > +++ b/tools/perf/tests/sigtrap.c > > > @@ -16,6 +16,8 @@ > > > #include <sys/syscall.h> > > > #include <unistd.h> > > > > > > +#include <bpf/btf.h> > > > + > > > #include "cloexec.h" > > > #include "debug.h" > > > #include "event.h" > > > @@ -54,6 +56,42 @@ static struct perf_event_attr make_event_attr(void) > > > return attr; > > > } > > > > > > +static bool attr_has_sigtrap(void) > > > +{ > > > + bool ret = false; > > > + > > > +#ifdef HAVE_BPF_SKEL > > > + > > > + struct btf *btf; > > > + const struct btf_type *t; > > > + const struct btf_member *m; > > > + const char *name; > > > + int i, id; > > > + > > > + /* just assume it doesn't have the field */ > > > + btf = btf__load_vmlinux_btf(); > > > + if (btf == NULL) > > > + return false; > > > + > > > + id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); > > > + if (id < 0) > > > + goto out; > > > + > > > + t = btf__type_by_id(btf, id); > > > + for (i = 0, m = btf_members(t); i < btf_vlen(t); i++, m++) { > > > + name = btf__name_by_offset(btf, m->name_off); > > > + if (!strcmp(name, "sigtrap")) { > > > + ret = true; > > > + break; > > > + } > > > + } > > > +out: > > > + btf__free(btf); > > > +#endif > > > > would it be easier to call perf_event_open for simple event with > > sigtrap set (and perhaps remove_on_exec) ? perf_copy_attr checks > > on reserved fields > > Hmm.. right. we could do that too. But it might still fail if there's a > bug in the path handling in sigtrap even for the simple case. I'm not > sure if it's a realistic concern though. :) I think we can do that when libbpf is not available. Thanks, Namhyung
Em Tue, Sep 06, 2022 at 10:04:07PM -0700, Namhyung Kim escreveu: > If it runs on an old kernel, perf_event_open would fail because of the > new fields sigtrap and sig_data. Just skipping the test could miss an > actual bug in the kernel. > > Let's check BTF if it has the perf_event_attr.sigtrap field. > > Cc: Marco Elver <elver@google.com> > Cc: Song Liu <songliubraving@fb.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/tests/sigtrap.c | 46 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c > index e32ece90e164..32f08ce0f2b0 100644 > --- a/tools/perf/tests/sigtrap.c > +++ b/tools/perf/tests/sigtrap.c > @@ -16,6 +16,8 @@ > #include <sys/syscall.h> > #include <unistd.h> > > +#include <bpf/btf.h> > + > #include "cloexec.h" > #include "debug.h" > #include "event.h" > @@ -54,6 +56,42 @@ static struct perf_event_attr make_event_attr(void) > return attr; > } > > +static bool attr_has_sigtrap(void) > +{ > + bool ret = false; > + > +#ifdef HAVE_BPF_SKEL > + > + struct btf *btf; > + const struct btf_type *t; > + const struct btf_member *m; > + const char *name; > + int i, id; > + > + /* just assume it doesn't have the field */ > + btf = btf__load_vmlinux_btf(); Cool, at some point we'll probably move this to some other global place, doing the lazy loading and keeping it in place as probably we'll use it more often :-) Waiting for v2. Thanks for working on this. - Arnaldo > + if (btf == NULL) > + return false; > + > + id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); > + if (id < 0) > + goto out; > + > + t = btf__type_by_id(btf, id); > + for (i = 0, m = btf_members(t); i < btf_vlen(t); i++, m++) { > + name = btf__name_by_offset(btf, m->name_off); > + if (!strcmp(name, "sigtrap")) { > + ret = true; > + break; > + } > + } > +out: > + btf__free(btf); > +#endif > + > + return ret; > +} > + > static void > sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __maybe_unused) > { > @@ -139,7 +177,13 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); > if (fd < 0) { > - pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf))); > + if (attr_has_sigtrap()) { > + pr_debug("FAILED sys_perf_event_open(): %s\n", > + str_error_r(errno, sbuf, sizeof(sbuf))); > + } else { > + pr_debug("perf_event_attr doesn't have sigtrap\n"); > + ret = TEST_SKIP; > + } > goto out_restore_sigaction; > } > > -- > 2.37.2.789.g6183377224-goog
diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c index e32ece90e164..32f08ce0f2b0 100644 --- a/tools/perf/tests/sigtrap.c +++ b/tools/perf/tests/sigtrap.c @@ -16,6 +16,8 @@ #include <sys/syscall.h> #include <unistd.h> +#include <bpf/btf.h> + #include "cloexec.h" #include "debug.h" #include "event.h" @@ -54,6 +56,42 @@ static struct perf_event_attr make_event_attr(void) return attr; } +static bool attr_has_sigtrap(void) +{ + bool ret = false; + +#ifdef HAVE_BPF_SKEL + + struct btf *btf; + const struct btf_type *t; + const struct btf_member *m; + const char *name; + int i, id; + + /* just assume it doesn't have the field */ + btf = btf__load_vmlinux_btf(); + if (btf == NULL) + return false; + + id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); + if (id < 0) + goto out; + + t = btf__type_by_id(btf, id); + for (i = 0, m = btf_members(t); i < btf_vlen(t); i++, m++) { + name = btf__name_by_offset(btf, m->name_off); + if (!strcmp(name, "sigtrap")) { + ret = true; + break; + } + } +out: + btf__free(btf); +#endif + + return ret; +} + static void sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __maybe_unused) { @@ -139,7 +177,13 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); if (fd < 0) { - pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf))); + if (attr_has_sigtrap()) { + pr_debug("FAILED sys_perf_event_open(): %s\n", + str_error_r(errno, sbuf, sizeof(sbuf))); + } else { + pr_debug("perf_event_attr doesn't have sigtrap\n"); + ret = TEST_SKIP; + } goto out_restore_sigaction; }
If it runs on an old kernel, perf_event_open would fail because of the new fields sigtrap and sig_data. Just skipping the test could miss an actual bug in the kernel. Let's check BTF if it has the perf_event_attr.sigtrap field. Cc: Marco Elver <elver@google.com> Cc: Song Liu <songliubraving@fb.com> Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/tests/sigtrap.c | 46 +++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-)