diff mbox series

perf test: Skip Sigtrap test for arm+aarch64

Message ID 1645114959-119064-1-git-send-email-john.garry@huawei.com (mailing list archive)
State New, archived
Headers show
Series perf test: Skip Sigtrap test for arm+aarch64 | expand

Commit Message

John Garry Feb. 17, 2022, 4:22 p.m. UTC 
Skip the Sigtrap test for arm + arm64, same as was done for s390 in
commit a840974e96fd ("perf test: Test 73 Sig_trap fails on s390").

As described by Will at [0], in the test we get stuck in a loop of handling
the HW breakpoint exception and never making progress. GDB handles this
by stepping over the faulting instruction, but with perf the kernel is
expected to handle the step (which it doesn't for arm).

Dmitry made an attempt to get this work, also mentioned in the same thread
as [0], which was appreciated. But the best thing to do is skip the test
for now.

[0] https://lore.kernel.org/linux-perf-users/20220118124343.GC98966@leoy-ThinkPad-X240s/T/#m13b06c39d2a5100d340f009435df6f4d8ee57b5a

Fixes: Fixes: 5504f67944484 ("perf test sigtrap: Add basic stress test for sigtrap handling")
Signed-off-by: John Garry <john.garry@huawei.com>

Comments

Marco Elver Feb. 17, 2022, 4:45 p.m. UTC | #1 
On Thu, 17 Feb 2022 at 17:28, John Garry <john.garry@huawei.com> wrote:
>
> Skip the Sigtrap test for arm + arm64, same as was done for s390 in
> commit a840974e96fd ("perf test: Test 73 Sig_trap fails on s390").
>
> As described by Will at [0], in the test we get stuck in a loop of handling
> the HW breakpoint exception and never making progress. GDB handles this
> by stepping over the faulting instruction, but with perf the kernel is
> expected to handle the step (which it doesn't for arm).
>
> Dmitry made an attempt to get this work, also mentioned in the same thread
> as [0], which was appreciated. But the best thing to do is skip the test
> for now.
>
> [0] https://lore.kernel.org/linux-perf-users/20220118124343.GC98966@leoy-ThinkPad-X240s/T/#m13b06c39d2a5100d340f009435df6f4d8ee57b5a
>
> Fixes: Fixes: 5504f67944484 ("perf test sigtrap: Add basic stress test for sigtrap handling")
> Signed-off-by: John Garry <john.garry@huawei.com>
>
> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> index 1f147fe6595f..3f0b5c1398b5 100644
> --- a/tools/perf/tests/sigtrap.c
> +++ b/tools/perf/tests/sigtrap.c
> @@ -29,7 +29,8 @@
>   * Just disable the test for these architectures until these issues are
>   * resolved.
>   */
> -#if defined(__powerpc__) || defined(__s390x__)
> +#if defined(__powerpc__) || defined(__s390x__) || \
> +       defined(__arm__) || defined(__aarch64__)
>  #define BP_ACCOUNT_IS_SUPPORTED 0
>  #else
>  #define BP_ACCOUNT_IS_SUPPORTED 1

This is now equivalent to BP_SIGNAL_IS_SUPPORTED
tools/perf/tests/tests.h -- and different from the original
BP_ACCOUNT_IS_SUPPORTED (and makes me wonder why
BP_SIGNAL_IS_SUPPORTED wasn't just used from the beginning). Perhaps
just use BP_SIGNAL_IS_SUPPORTED.

Thanks,
-- Marco
John Garry Feb. 17, 2022, 5:34 p.m. UTC | #2 
On 17/02/2022 16:45, Marco Elver wrote:
> On Thu, 17 Feb 2022 at 17:28, John Garry <john.garry@huawei.com> wrote:

+ Ian, Thomas

>>
>> Skip the Sigtrap test for arm + arm64, same as was done for s390 in
>> commit a840974e96fd ("perf test: Test 73 Sig_trap fails on s390").
>>
>> As described by Will at [0], in the test we get stuck in a loop of handling
>> the HW breakpoint exception and never making progress. GDB handles this
>> by stepping over the faulting instruction, but with perf the kernel is
>> expected to handle the step (which it doesn't for arm).
>>
>> Dmitry made an attempt to get this work, also mentioned in the same thread
>> as [0], which was appreciated. But the best thing to do is skip the test
>> for now.
>>
>> [0] https://lore.kernel.org/linux-perf-users/20220118124343.GC98966@leoy-ThinkPad-X240s/T/#m13b06c39d2a5100d340f009435df6f4d8ee57b5a
>>
>> Fixes: Fixes: 5504f67944484 ("perf test sigtrap: Add basic stress test for sigtrap handling")
>> Signed-off-by: John Garry <john.garry@huawei.com>
>>
>> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
>> index 1f147fe6595f..3f0b5c1398b5 100644
>> --- a/tools/perf/tests/sigtrap.c
>> +++ b/tools/perf/tests/sigtrap.c
>> @@ -29,7 +29,8 @@
>>    * Just disable the test for these architectures until these issues are
>>    * resolved.
>>    */
>> -#if defined(__powerpc__) || defined(__s390x__)
>> +#if defined(__powerpc__) || defined(__s390x__) || \
>> +       defined(__arm__) || defined(__aarch64__)
>>   #define BP_ACCOUNT_IS_SUPPORTED 0
>>   #else
>>   #define BP_ACCOUNT_IS_SUPPORTED 1
> 
> This is now equivalent to BP_SIGNAL_IS_SUPPORTED
> tools/perf/tests/tests.h -- and different from the original
> BP_ACCOUNT_IS_SUPPORTED (and makes me wonder why
> BP_SIGNAL_IS_SUPPORTED wasn't just used from the beginning). Perhaps
> just use BP_SIGNAL_IS_SUPPORTED.
> 

We currently have BP_ACCOUNT_IS_SUPPORTED defined now in 2x locations:

tests/sigtrap.c
tests/bp_account.c

bp_account works for arm64, and we don't want to skip that test. So, as 
long as the macro meaning is appropriate, we can reuse 
BP_SIGNAL_IS_SUPPORTED for sigtrap.c

Thanks,
John
Marco Elver Feb. 17, 2022, 5:40 p.m. UTC | #3 
On Thu, 17 Feb 2022 at 18:34, John Garry <john.garry@huawei.com> wrote:
[...]
> >> -#if defined(__powerpc__) || defined(__s390x__)
> >> +#if defined(__powerpc__) || defined(__s390x__) || \
> >> +       defined(__arm__) || defined(__aarch64__)
> >>   #define BP_ACCOUNT_IS_SUPPORTED 0
> >>   #else
> >>   #define BP_ACCOUNT_IS_SUPPORTED 1
> >
> > This is now equivalent to BP_SIGNAL_IS_SUPPORTED
> > tools/perf/tests/tests.h -- and different from the original
> > BP_ACCOUNT_IS_SUPPORTED (and makes me wonder why
> > BP_SIGNAL_IS_SUPPORTED wasn't just used from the beginning). Perhaps
> > just use BP_SIGNAL_IS_SUPPORTED.
> >
>
> We currently have BP_ACCOUNT_IS_SUPPORTED defined now in 2x locations:
>
> tests/sigtrap.c
> tests/bp_account.c
>
> bp_account works for arm64, and we don't want to skip that test. So, as
> long as the macro meaning is appropriate, we can reuse
> BP_SIGNAL_IS_SUPPORTED for sigtrap.c

BP_ACCOUNT seems to say something about the "breakpoint accounting /
measuring" test. BP_SIGNAL is about the tests that want to use
breakpoints to generate signals.

So it's very much appropriate to use BP_SIGNAL here if, as we have
discovered regardless how they're generated in response to
breakpoints, are broken on arm/arm64. On the day arm/arm64 decides to
fix signals, I'm assuming all tests being skipped with
BP_SIGNAL_IS_SUPPORTED can be re-enabled (or so we hope).

Thanks,
-- Marco
Leo Yan Feb. 18, 2022, 1:58 a.m. UTC | #4 
On Thu, Feb 17, 2022 at 06:40:45PM +0100, Marco Elver wrote:
> On Thu, 17 Feb 2022 at 18:34, John Garry <john.garry@huawei.com> wrote:
> [...]
> > >> -#if defined(__powerpc__) || defined(__s390x__)
> > >> +#if defined(__powerpc__) || defined(__s390x__) || \
> > >> +       defined(__arm__) || defined(__aarch64__)
> > >>   #define BP_ACCOUNT_IS_SUPPORTED 0
> > >>   #else
> > >>   #define BP_ACCOUNT_IS_SUPPORTED 1
> > >
> > > This is now equivalent to BP_SIGNAL_IS_SUPPORTED
> > > tools/perf/tests/tests.h -- and different from the original
> > > BP_ACCOUNT_IS_SUPPORTED (and makes me wonder why
> > > BP_SIGNAL_IS_SUPPORTED wasn't just used from the beginning). Perhaps
> > > just use BP_SIGNAL_IS_SUPPORTED.
> > >
> >
> > We currently have BP_ACCOUNT_IS_SUPPORTED defined now in 2x locations:
> >
> > tests/sigtrap.c
> > tests/bp_account.c
> >
> > bp_account works for arm64, and we don't want to skip that test. So, as
> > long as the macro meaning is appropriate, we can reuse
> > BP_SIGNAL_IS_SUPPORTED for sigtrap.c
> 
> BP_ACCOUNT seems to say something about the "breakpoint accounting /
> measuring" test. BP_SIGNAL is about the tests that want to use
> breakpoints to generate signals.

More general speaking, I think "BP_ACCOUNT_IS_SUPPORTED = 1" means an
architecture can support breakpoint with perf_event.

"BP_SIGNAL_IS_SUPPORTED = 1" means an architecture can support
breakpoint to generate signals with using perf_event.  So
"BP_SIGNAL_IS_SUPPORTED = 1" is subset of "BP_ACCOUNT_IS_SUPPORTED = 1".

> So it's very much appropriate to use BP_SIGNAL here if, as we have
> discovered regardless how they're generated in response to
> breakpoints, are broken on arm/arm64. On the day arm/arm64 decides to
> fix signals, I'm assuming all tests being skipped with
> BP_SIGNAL_IS_SUPPORTED can be re-enabled (or so we hope).

Yeah, I agree that BP_SIGNAL_IS_SUPPORTED is better choice for sigtrap.c.

Thanks,
Leo
diff mbox series

Patch

diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
index 1f147fe6595f..3f0b5c1398b5 100644
--- a/tools/perf/tests/sigtrap.c
+++ b/tools/perf/tests/sigtrap.c
@@ -29,7 +29,8 @@ 
  * Just disable the test for these architectures until these issues are
  * resolved.
  */
-#if defined(__powerpc__) || defined(__s390x__)
+#if defined(__powerpc__) || defined(__s390x__) || \
+	defined(__arm__) || defined(__aarch64__)
 #define BP_ACCOUNT_IS_SUPPORTED 0
 #else
 #define BP_ACCOUNT_IS_SUPPORTED 1