On Thu, 17 Feb 2022 at 17:28, John Garry <[email protected]> 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 <[email protected]>
>
> 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
On 17/02/2022 16:45, Marco Elver wrote:
> On Thu, 17 Feb 2022 at 17:28, John Garry <[email protected]> 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 <[email protected]>
>>
>> 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
On Thu, 17 Feb 2022 at 18:34, John Garry <[email protected]> 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
On Thu, Feb 17, 2022 at 06:40:45PM +0100, Marco Elver wrote:
> On Thu, 17 Feb 2022 at 18:34, John Garry <[email protected]> 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