[2/2] perf tests sigtrap: Skip if running on a kernel with sleepable spinlocks

Message ID 20231129154718.326330-3-acme@kernel.org
State New
Headers
Series Skip the 'perf test sigtrap' on kernels with sleepable spinlocks |

Commit Message

Arnaldo Carvalho de Melo Nov. 29, 2023, 3:47 p.m. UTC
  From: Arnaldo Carvalho de Melo <acme@redhat.com>

There are issues as reported that need some more investigation on the
RT kernel front, till that is addressed, skip this test.

This test is already skipped for multiple hardware architectures where
the tested kernel feature is not supported.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Clark Williams <williams@redhat.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Marco Elver <elver@google.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/sigtrap.c | 46 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)
  

Comments

Marco Elver Nov. 29, 2023, 3:57 p.m. UTC | #1
On Wed, 29 Nov 2023 at 16:47, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> There are issues as reported that need some more investigation on the
> RT kernel front, till that is addressed, skip this test.
>
> This test is already skipped for multiple hardware architectures where
> the tested kernel feature is not supported.
>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Clark Williams <williams@redhat.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Acked-by: Marco Elver <elver@google.com>

> ---
>  tools/perf/tests/sigtrap.c | 46 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> index a1bc7c776254ed2f..e6fd934b027a3d0c 100644
> --- a/tools/perf/tests/sigtrap.c
> +++ b/tools/perf/tests/sigtrap.c
> @@ -103,6 +103,34 @@ static bool attr_has_sigtrap(void)
>
>         return __btf_type__find_member_by_name(id, "sigtrap") != NULL;
>  }
> +
> +static bool kernel_with_sleepable_spinlocks(void)
> +{
> +       const struct btf_member *member;
> +       const struct btf_type *type;
> +       const char *type_name;
> +       int id;
> +
> +       if (!btf__available())
> +               return false;
> +
> +       id = btf__find_by_name_kind(btf, "spinlock", BTF_KIND_STRUCT);
> +       if (id < 0)
> +               return false;
> +
> +       // Only RT has a "lock" member for "struct spinlock"
> +       member = __btf_type__find_member_by_name(id, "lock");
> +       if (member == NULL)
> +               return false;
> +
> +       // But check its type as well
> +       type = btf__type_by_id(btf, member->type);
> +       if (!type || !btf_is_struct(type))
> +               return false;
> +
> +       type_name = btf__name_by_offset(btf, type->name_off);
> +       return type_name && !strcmp(type_name, "rt_mutex_base");
> +}
>  #else  /* !HAVE_BPF_SKEL */
>  static bool attr_has_sigtrap(void)
>  {
> @@ -125,6 +153,11 @@ static bool attr_has_sigtrap(void)
>         return ret;
>  }
>
> +static bool kernel_with_sleepable_spinlocks(void)
> +{
> +       return false;
> +}
> +
>  static void btf__exit(void)
>  {
>  }
> @@ -166,7 +199,7 @@ static int run_test_threads(pthread_t *threads, pthread_barrier_t *barrier)
>
>  static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrier)
>  {
> -       int ret;
> +       int ret, expected_sigtraps;
>
>         ctx.iterate_on = 3000;
>
> @@ -175,7 +208,16 @@ static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrie
>         ret = run_test_threads(threads, barrier);
>         TEST_ASSERT_EQUAL("disable failed", ioctl(fd, PERF_EVENT_IOC_DISABLE, 0), 0);
>
> -       TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on);
> +       expected_sigtraps = NUM_THREADS * ctx.iterate_on;
> +
> +       if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable_spinlocks()) {
> +               pr_debug("Expected %d sigtraps, got %d, running on a kernel with sleepable spinlocks.\n",
> +                        expected_sigtraps, ctx.signal_count);
> +               pr_debug("See https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/\n");

No changes from the RT side since? A fix exists, but apparently not
good enough... Sigh.

> +               return TEST_SKIP;
> +       } else
> +               TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, expected_sigtraps);
> +
>         TEST_ASSERT_EQUAL("missing signals or incorrectly delivered", ctx.tids_want_signal, 0);
>         TEST_ASSERT_VAL("unexpected si_addr", ctx.first_siginfo.si_addr == &ctx.iterate_on);
>  #if 0 /* FIXME: enable when libc's signal.h has si_perf_{type,data} */
> --
> 2.41.0
>
  
Arnaldo Carvalho de Melo Nov. 29, 2023, 8:42 p.m. UTC | #2
Em Wed, Nov 29, 2023 at 04:57:47PM +0100, Marco Elver escreveu:
> On Wed, 29 Nov 2023 at 16:47, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> > There are issues as reported that need some more investigation on the
> > RT kernel front, till that is addressed, skip this test.
> >
> > This test is already skipped for multiple hardware architectures where
> > the tested kernel feature is not supported.
> >
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Clark Williams <williams@redhat.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Juri Lelli <juri.lelli@redhat.com>
> > Cc: Marco Elver <elver@google.com>
> > Cc: Mike Galbraith <efault@gmx.de>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Link: https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Acked-by: Marco Elver <elver@google.com>
> 
> > ---
> >  tools/perf/tests/sigtrap.c | 46 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> > index a1bc7c776254ed2f..e6fd934b027a3d0c 100644
> > --- a/tools/perf/tests/sigtrap.c
> > +++ b/tools/perf/tests/sigtrap.c
> > @@ -103,6 +103,34 @@ static bool attr_has_sigtrap(void)
> >
> >         return __btf_type__find_member_by_name(id, "sigtrap") != NULL;
> >  }
> > +
> > +static bool kernel_with_sleepable_spinlocks(void)
> > +{
> > +       const struct btf_member *member;
> > +       const struct btf_type *type;
> > +       const char *type_name;
> > +       int id;
> > +
> > +       if (!btf__available())
> > +               return false;
> > +
> > +       id = btf__find_by_name_kind(btf, "spinlock", BTF_KIND_STRUCT);
> > +       if (id < 0)
> > +               return false;
> > +
> > +       // Only RT has a "lock" member for "struct spinlock"
> > +       member = __btf_type__find_member_by_name(id, "lock");
> > +       if (member == NULL)
> > +               return false;
> > +
> > +       // But check its type as well
> > +       type = btf__type_by_id(btf, member->type);
> > +       if (!type || !btf_is_struct(type))
> > +               return false;
> > +
> > +       type_name = btf__name_by_offset(btf, type->name_off);
> > +       return type_name && !strcmp(type_name, "rt_mutex_base");
> > +}
> >  #else  /* !HAVE_BPF_SKEL */
> >  static bool attr_has_sigtrap(void)
> >  {
> > @@ -125,6 +153,11 @@ static bool attr_has_sigtrap(void)
> >         return ret;
> >  }
> >
> > +static bool kernel_with_sleepable_spinlocks(void)
> > +{
> > +       return false;
> > +}
> > +
> >  static void btf__exit(void)
> >  {
> >  }
> > @@ -166,7 +199,7 @@ static int run_test_threads(pthread_t *threads, pthread_barrier_t *barrier)
> >
> >  static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrier)
> >  {
> > -       int ret;
> > +       int ret, expected_sigtraps;
> >
> >         ctx.iterate_on = 3000;
> >
> > @@ -175,7 +208,16 @@ static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrie
> >         ret = run_test_threads(threads, barrier);
> >         TEST_ASSERT_EQUAL("disable failed", ioctl(fd, PERF_EVENT_IOC_DISABLE, 0), 0);
> >
> > -       TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on);
> > +       expected_sigtraps = NUM_THREADS * ctx.iterate_on;
> > +
> > +       if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable_spinlocks()) {
> > +               pr_debug("Expected %d sigtraps, got %d, running on a kernel with sleepable spinlocks.\n",
> > +                        expected_sigtraps, ctx.signal_count);
> > +               pr_debug("See https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/\n");
> 
> No changes from the RT side since? A fix exists, but apparently not
> good enough... Sigh.

Yeah, my impression, and first attempt at writing that patch wast that
no sigtraps were being sent, but then when I tried with a random, more
recent machine in the Red Hat labs, I got some signals, way less than
the expected ones, but some, maybe this is an interesting data point?

I'll try again to reproduce in the local machine, old i7 lenovo notebook
and at the newer machine, a Xeon(R) Silver 4216, 32 cpu and report here.

- Arnaldo
  
Marco Elver Nov. 30, 2023, 1:28 p.m. UTC | #3
On Thu, 30 Nov 2023 at 14:01, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Wed, Nov 29, 2023 at 05:42:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Nov 29, 2023 at 04:57:47PM +0100, Marco Elver escreveu:
> > > > @@ -175,7 +208,16 @@ static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrie
> > > >         ret = run_test_threads(threads, barrier);
> > > >         TEST_ASSERT_EQUAL("disable failed", ioctl(fd, PERF_EVENT_IOC_DISABLE, 0), 0);
>
> > > > -       TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on);
> > > > +       expected_sigtraps = NUM_THREADS * ctx.iterate_on;
>
> > > > +       if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable_spinlocks()) {
> > > > +               pr_debug("Expected %d sigtraps, got %d, running on a kernel with sleepable spinlocks.\n",
> > > > +                        expected_sigtraps, ctx.signal_count);
> > > > +               pr_debug("See https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/\n");
>
> > > No changes from the RT side since? A fix exists, but apparently not
> > > good enough... Sigh.
>
> > Yeah, my impression, and first attempt at writing that patch wast that
> > no sigtraps were being sent, but then when I tried with a random, more
> > recent machine in the Red Hat labs, I got some signals, way less than
> > the expected ones, but some, maybe this is an interesting data point?
>
> > I'll try again to reproduce in the local machine, old i7 lenovo notebook
> > and at the newer machine, a Xeon(R) Silver 4216, 32 cpu and report here.
>
> So, on the i7 lenovo:
>
> [root@nine ~]# uname -a
> Linux nine 5.14.0-284.30.1.rt14.315.el9_2.x86_64 #1 SMP PREEMPT_RT Fri Aug 25 10:53:59 EDT 2023 x86_64 x86_64 x86_64 GNU/Linux
[...]
>
> I guess I'll try to get hold of the older kernel with 0 sigtraps to see
> if I get the same behaviour (consistent 0 sigtraps) on that kernel on
> the bigger machine :-\

Thanks for checking.

In any case, it looks like it's still broken. If the fix (bf9ad37dc8a
+ small diff by Mike) from [1] still works, what's blocking it from
being upstreamed?

https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/
  

Patch

diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
index a1bc7c776254ed2f..e6fd934b027a3d0c 100644
--- a/tools/perf/tests/sigtrap.c
+++ b/tools/perf/tests/sigtrap.c
@@ -103,6 +103,34 @@  static bool attr_has_sigtrap(void)
 
 	return __btf_type__find_member_by_name(id, "sigtrap") != NULL;
 }
+
+static bool kernel_with_sleepable_spinlocks(void)
+{
+	const struct btf_member *member;
+	const struct btf_type *type;
+	const char *type_name;
+	int id;
+
+	if (!btf__available())
+		return false;
+
+	id = btf__find_by_name_kind(btf, "spinlock", BTF_KIND_STRUCT);
+	if (id < 0)
+		return false;
+
+	// Only RT has a "lock" member for "struct spinlock"
+	member = __btf_type__find_member_by_name(id, "lock");
+	if (member == NULL)
+		return false;
+
+	// But check its type as well
+	type = btf__type_by_id(btf, member->type);
+	if (!type || !btf_is_struct(type))
+		return false;
+
+	type_name = btf__name_by_offset(btf, type->name_off);
+	return type_name && !strcmp(type_name, "rt_mutex_base");
+}
 #else  /* !HAVE_BPF_SKEL */
 static bool attr_has_sigtrap(void)
 {
@@ -125,6 +153,11 @@  static bool attr_has_sigtrap(void)
 	return ret;
 }
 
+static bool kernel_with_sleepable_spinlocks(void)
+{
+	return false;
+}
+
 static void btf__exit(void)
 {
 }
@@ -166,7 +199,7 @@  static int run_test_threads(pthread_t *threads, pthread_barrier_t *barrier)
 
 static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrier)
 {
-	int ret;
+	int ret, expected_sigtraps;
 
 	ctx.iterate_on = 3000;
 
@@ -175,7 +208,16 @@  static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrie
 	ret = run_test_threads(threads, barrier);
 	TEST_ASSERT_EQUAL("disable failed", ioctl(fd, PERF_EVENT_IOC_DISABLE, 0), 0);
 
-	TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on);
+	expected_sigtraps = NUM_THREADS * ctx.iterate_on;
+
+	if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable_spinlocks()) {
+		pr_debug("Expected %d sigtraps, got %d, running on a kernel with sleepable spinlocks.\n",
+			 expected_sigtraps, ctx.signal_count);
+		pr_debug("See https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/\n");
+		return TEST_SKIP;
+	} else
+		TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, expected_sigtraps);
+
 	TEST_ASSERT_EQUAL("missing signals or incorrectly delivered", ctx.tids_want_signal, 0);
 	TEST_ASSERT_VAL("unexpected si_addr", ctx.first_siginfo.si_addr == &ctx.iterate_on);
 #if 0 /* FIXME: enable when libc's signal.h has si_perf_{type,data} */