As there have several discussions for enabling Perf breakpoint signal
testing on arm64 platform; arm64 needs to rely on single-step to execute
the breakpointed instruction and then reinstall the breakpoint exception
handler. But if hook the breakpoint with a signal, the signal handler
will do the stepping rather than the breakpointed instruction, this
causes infinite loops as below:
Kernel space | Userspace
-----------------------------------|--------------------------------
| __test_function() -> hit
| breakpoint
breakpoint_handler() |
`-> user_enable_single_step() |
do_signal() |
| sig_handler() -> Step one
| instruction and
| trap to kernel
single_step_handler() |
`-> reinstall_suspended_bps() |
| __test_function() -> hit
| breakpoint again and
| repeat up flow infinitely
As Will Deacon mentioned [1]: "that we require the overflow handler to
do the stepping on arm/arm64, which is relied upon by GDB/ptrace. The
hw_breakpoint code is a complete disaster so my preference would be to
rip out the perf part and just implement something directly in ptrace,
but it's a pretty horrible job". Though Will commented this on arm
architecture, but the comment also can apply on arm64 architecture.
For complete information, I searched online and found a few years back,
Wang Nan sent one patch 'arm64: Store breakpoint single step state into
pstate' [2]; the patch tried to resolve this issue by avoiding single
stepping in signal handler and defer to enable the signal stepping when
return to __test_function(). The fixing was not merged due to the
concern for missing to handle different usage cases.
Based on the info, the most feasible way is to skip Perf breakpoint
signal testing for arm64 and this could avoid the duplicate
investigation efforts when people see the failure. This patch skips
this case on arm64 platform, which is same with arm architecture.
[1] https://lkml.org/lkml/2018/11/15/205
[2] https://lkml.org/lkml/2015/12/23/477
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/tests/bp_signal.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
index c1c2c13de254..166f411568a5 100644
--- a/tools/perf/tests/bp_signal.c
+++ b/tools/perf/tests/bp_signal.c
@@ -49,14 +49,6 @@ asm (
"__test_function:\n"
"incq (%rdi)\n"
"ret\n");
-#elif defined (__aarch64__)
-extern void __test_function(volatile long *ptr);
-asm (
- ".globl __test_function\n"
- "__test_function:\n"
- "str x30, [x0]\n"
- "ret\n");
-
#else
static void __test_function(volatile long *ptr)
{
@@ -302,10 +294,15 @@ bool test__bp_signal_is_supported(void)
* stepping into the SIGIO handler and getting stuck on the
* breakpointed instruction.
*
+ * Since arm64 has the same issue with arm for the single-step
+ * handling, this case also gets suck on the breakpointed
+ * instruction.
+ *
* Just disable the test for these architectures until these
* issues are resolved.
*/
-#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__)
+#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__) || \
+ defined(__aarch64__)
return false;
#else
return true;
--
2.17.1
Em Fri, Oct 18, 2019 at 04:55:31PM +0800, Leo Yan escreveu:
> As there have several discussions for enabling Perf breakpoint signal
> testing on arm64 platform; arm64 needs to rely on single-step to execute
> the breakpointed instruction and then reinstall the breakpoint exception
> handler. But if hook the breakpoint with a signal, the signal handler
> will do the stepping rather than the breakpointed instruction, this
> causes infinite loops as below:
>
> Kernel space | Userspace
> -----------------------------------|--------------------------------
> | __test_function() -> hit
> | breakpoint
> breakpoint_handler() |
> `-> user_enable_single_step() |
> do_signal() |
> | sig_handler() -> Step one
> | instruction and
> | trap to kernel
> single_step_handler() |
> `-> reinstall_suspended_bps() |
> | __test_function() -> hit
> | breakpoint again and
> | repeat up flow infinitely
>
> As Will Deacon mentioned [1]: "that we require the overflow handler to
> do the stepping on arm/arm64, which is relied upon by GDB/ptrace. The
> hw_breakpoint code is a complete disaster so my preference would be to
> rip out the perf part and just implement something directly in ptrace,
> but it's a pretty horrible job". Though Will commented this on arm
> architecture, but the comment also can apply on arm64 architecture.
>
> For complete information, I searched online and found a few years back,
> Wang Nan sent one patch 'arm64: Store breakpoint single step state into
> pstate' [2]; the patch tried to resolve this issue by avoiding single
> stepping in signal handler and defer to enable the signal stepping when
> return to __test_function(). The fixing was not merged due to the
> concern for missing to handle different usage cases.
>
> Based on the info, the most feasible way is to skip Perf breakpoint
> signal testing for arm64 and this could avoid the duplicate
> investigation efforts when people see the failure. This patch skips
> this case on arm64 platform, which is same with arm architecture.
Ok, applying,
- Arnaldo
> [1] https://lkml.org/lkml/2018/11/15/205
> [2] https://lkml.org/lkml/2015/12/23/477
>
> Signed-off-by: Leo Yan <[email protected]>
> ---
> tools/perf/tests/bp_signal.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
> index c1c2c13de254..166f411568a5 100644
> --- a/tools/perf/tests/bp_signal.c
> +++ b/tools/perf/tests/bp_signal.c
> @@ -49,14 +49,6 @@ asm (
> "__test_function:\n"
> "incq (%rdi)\n"
> "ret\n");
> -#elif defined (__aarch64__)
> -extern void __test_function(volatile long *ptr);
> -asm (
> - ".globl __test_function\n"
> - "__test_function:\n"
> - "str x30, [x0]\n"
> - "ret\n");
> -
> #else
> static void __test_function(volatile long *ptr)
> {
> @@ -302,10 +294,15 @@ bool test__bp_signal_is_supported(void)
> * stepping into the SIGIO handler and getting stuck on the
> * breakpointed instruction.
> *
> + * Since arm64 has the same issue with arm for the single-step
> + * handling, this case also gets suck on the breakpointed
> + * instruction.
> + *
> * Just disable the test for these architectures until these
> * issues are resolved.
> */
> -#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__)
> +#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__) || \
> + defined(__aarch64__)
> return false;
> #else
> return true;
> --
> 2.17.1
--
- Arnaldo
On Fri, Oct 18, 2019 at 02:59:19PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Oct 18, 2019 at 04:55:31PM +0800, Leo Yan escreveu:
> > As there have several discussions for enabling Perf breakpoint signal
> > testing on arm64 platform; arm64 needs to rely on single-step to execute
> > the breakpointed instruction and then reinstall the breakpoint exception
> > handler. But if hook the breakpoint with a signal, the signal handler
> > will do the stepping rather than the breakpointed instruction, this
> > causes infinite loops as below:
> >
> > Kernel space | Userspace
> > -----------------------------------|--------------------------------
> > | __test_function() -> hit
> > | breakpoint
> > breakpoint_handler() |
> > `-> user_enable_single_step() |
> > do_signal() |
> > | sig_handler() -> Step one
> > | instruction and
> > | trap to kernel
> > single_step_handler() |
> > `-> reinstall_suspended_bps() |
> > | __test_function() -> hit
> > | breakpoint again and
> > | repeat up flow infinitely
> >
> > As Will Deacon mentioned [1]: "that we require the overflow handler to
> > do the stepping on arm/arm64, which is relied upon by GDB/ptrace. The
> > hw_breakpoint code is a complete disaster so my preference would be to
> > rip out the perf part and just implement something directly in ptrace,
> > but it's a pretty horrible job". Though Will commented this on arm
> > architecture, but the comment also can apply on arm64 architecture.
> >
> > For complete information, I searched online and found a few years back,
> > Wang Nan sent one patch 'arm64: Store breakpoint single step state into
> > pstate' [2]; the patch tried to resolve this issue by avoiding single
> > stepping in signal handler and defer to enable the signal stepping when
> > return to __test_function(). The fixing was not merged due to the
> > concern for missing to handle different usage cases.
> >
> > Based on the info, the most feasible way is to skip Perf breakpoint
> > signal testing for arm64 and this could avoid the duplicate
> > investigation efforts when people see the failure. This patch skips
> > this case on arm64 platform, which is same with arm architecture.
>
> Ok, applying,
Thanks a lot, Arnaldo.
@Will, @Mark Rultland, very appreciate if you have time to review this
patch and welcome any comments or suggestions! It's good you could
confirm this patch so have more confidence for it.
Thanks,
Leo Yan
> - Arnaldo
>
> > [1] https://lkml.org/lkml/2018/11/15/205
> > [2] https://lkml.org/lkml/2015/12/23/477
> >
> > Signed-off-by: Leo Yan <[email protected]>
> > ---
> > tools/perf/tests/bp_signal.c | 15 ++++++---------
> > 1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
> > index c1c2c13de254..166f411568a5 100644
> > --- a/tools/perf/tests/bp_signal.c
> > +++ b/tools/perf/tests/bp_signal.c
> > @@ -49,14 +49,6 @@ asm (
> > "__test_function:\n"
> > "incq (%rdi)\n"
> > "ret\n");
> > -#elif defined (__aarch64__)
> > -extern void __test_function(volatile long *ptr);
> > -asm (
> > - ".globl __test_function\n"
> > - "__test_function:\n"
> > - "str x30, [x0]\n"
> > - "ret\n");
> > -
> > #else
> > static void __test_function(volatile long *ptr)
> > {
> > @@ -302,10 +294,15 @@ bool test__bp_signal_is_supported(void)
> > * stepping into the SIGIO handler and getting stuck on the
> > * breakpointed instruction.
> > *
> > + * Since arm64 has the same issue with arm for the single-step
> > + * handling, this case also gets suck on the breakpointed
> > + * instruction.
> > + *
> > * Just disable the test for these architectures until these
> > * issues are resolved.
> > */
> > -#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__)
> > +#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__) || \
> > + defined(__aarch64__)
> > return false;
> > #else
> > return true;
> > --
> > 2.17.1
>
> --
>
> - Arnaldo
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 6a5f3d94cb69a185b921cb92c39888dc31009acb
Gitweb: https://git.kernel.org/tip/6a5f3d94cb69a185b921cb92c39888dc31009acb
Author: Leo Yan <[email protected]>
AuthorDate: Fri, 18 Oct 2019 16:55:31 +08:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Sat, 19 Oct 2019 15:35:01 -03:00
perf tests: Disable bp_signal testing for arm64
As there are several discussions for enabling perf breakpoint signal
testing on arm64 platform: arm64 needs to rely on single-step to execute
the breakpointed instruction and then reinstall the breakpoint exception
handler. But if we hook the breakpoint with a signal, the signal
handler will do the stepping rather than the breakpointed instruction,
this causes infinite loops as below:
Kernel space | Userspace
---------------------------------|--------------------------------
| __test_function() -> hit
| breakpoint
breakpoint_handler() |
`-> user_enable_single_step() |
do_signal() |
| sig_handler() -> Step one
| instruction and
| trap to kernel
single_step_handler() |
`-> reinstall_suspended_bps() |
| __test_function() -> hit
| breakpoint again and
| repeat up flow infinitely
As Will Deacon mentioned [1]: "that we require the overflow handler to
do the stepping on arm/arm64, which is relied upon by GDB/ptrace. The
hw_breakpoint code is a complete disaster so my preference would be to
rip out the perf part and just implement something directly in ptrace,
but it's a pretty horrible job". Though Will commented this on arm
architecture, but the comment also can apply on arm64 architecture.
For complete information, I searched online and found a few years back,
Wang Nan sent one patch 'arm64: Store breakpoint single step state into
pstate' [2]; the patch tried to resolve this issue by avoiding single
stepping in signal handler and defer to enable the signal stepping when
return to __test_function(). The fixing was not merged due to the
concern for missing to handle different usage cases.
Based on the info, the most feasible way is to skip Perf breakpoint
signal testing for arm64 and this could avoid the duplicate
investigation efforts when people see the failure. This patch skips
this case on arm64 platform, which is same with arm architecture.
[1] https://lkml.org/lkml/2018/11/15/205
[2] https://lkml.org/lkml/2015/12/23/477
Signed-off-by: Leo Yan <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Brajeswar Ghosh <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Michael Petlan <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Souptick Joarder <[email protected]>
Cc: Will Deacon <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/tests/bp_signal.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
index c1c2c13..166f411 100644
--- a/tools/perf/tests/bp_signal.c
+++ b/tools/perf/tests/bp_signal.c
@@ -49,14 +49,6 @@ asm (
"__test_function:\n"
"incq (%rdi)\n"
"ret\n");
-#elif defined (__aarch64__)
-extern void __test_function(volatile long *ptr);
-asm (
- ".globl __test_function\n"
- "__test_function:\n"
- "str x30, [x0]\n"
- "ret\n");
-
#else
static void __test_function(volatile long *ptr)
{
@@ -302,10 +294,15 @@ bool test__bp_signal_is_supported(void)
* stepping into the SIGIO handler and getting stuck on the
* breakpointed instruction.
*
+ * Since arm64 has the same issue with arm for the single-step
+ * handling, this case also gets suck on the breakpointed
+ * instruction.
+ *
* Just disable the test for these architectures until these
* issues are resolved.
*/
-#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__)
+#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__) || \
+ defined(__aarch64__)
return false;
#else
return true;
On Mon, Oct 21, 2019 at 11:18:54PM -0000, tip-bot2 for Leo Yan wrote:
> The following commit has been merged into the perf/core branch of tip:
>
> Commit-ID: 6a5f3d94cb69a185b921cb92c39888dc31009acb
> Gitweb: https://git.kernel.org/tip/6a5f3d94cb69a185b921cb92c39888dc31009acb
> Author: Leo Yan <[email protected]>
> AuthorDate: Fri, 18 Oct 2019 16:55:31 +08:00
> Committer: Arnaldo Carvalho de Melo <[email protected]>
> CommitterDate: Sat, 19 Oct 2019 15:35:01 -03:00
>
> perf tests: Disable bp_signal testing for arm64
>
> As there are several discussions for enabling perf breakpoint signal
> testing on arm64 platform: arm64 needs to rely on single-step to execute
> the breakpointed instruction and then reinstall the breakpoint exception
> handler. But if we hook the breakpoint with a signal, the signal
> handler will do the stepping rather than the breakpointed instruction,
> this causes infinite loops as below:
>
> Kernel space | Userspace
> ---------------------------------|--------------------------------
> | __test_function() -> hit
> | breakpoint
> breakpoint_handler() |
> `-> user_enable_single_step() |
> do_signal() |
> | sig_handler() -> Step one
> | instruction and
> | trap to kernel
> single_step_handler() |
> `-> reinstall_suspended_bps() |
> | __test_function() -> hit
> | breakpoint again and
> | repeat up flow infinitely
>
> As Will Deacon mentioned [1]: "that we require the overflow handler to
> do the stepping on arm/arm64, which is relied upon by GDB/ptrace. The
> hw_breakpoint code is a complete disaster so my preference would be to
> rip out the perf part and just implement something directly in ptrace,
> but it's a pretty horrible job". Though Will commented this on arm
> architecture, but the comment also can apply on arm64 architecture.
>
> For complete information, I searched online and found a few years back,
> Wang Nan sent one patch 'arm64: Store breakpoint single step state into
> pstate' [2]; the patch tried to resolve this issue by avoiding single
> stepping in signal handler and defer to enable the signal stepping when
> return to __test_function(). The fixing was not merged due to the
> concern for missing to handle different usage cases.
>
> Based on the info, the most feasible way is to skip Perf breakpoint
> signal testing for arm64 and this could avoid the duplicate
> investigation efforts when people see the failure. This patch skips
> this case on arm64 platform, which is same with arm architecture.
>
> [1] https://lkml.org/lkml/2018/11/15/205
> [2] https://lkml.org/lkml/2015/12/23/477
>
> Signed-off-by: Leo Yan <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Brajeswar Ghosh <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Michael Petlan <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Song Liu <[email protected]>
> Cc: Souptick Joarder <[email protected]>
> Cc: Will Deacon <[email protected]>
> Link: http://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> ---
> tools/perf/tests/bp_signal.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
> index c1c2c13..166f411 100644
> --- a/tools/perf/tests/bp_signal.c
> +++ b/tools/perf/tests/bp_signal.c
> @@ -49,14 +49,6 @@ asm (
> "__test_function:\n"
> "incq (%rdi)\n"
> "ret\n");
> -#elif defined (__aarch64__)
> -extern void __test_function(volatile long *ptr);
> -asm (
> - ".globl __test_function\n"
> - "__test_function:\n"
> - "str x30, [x0]\n"
> - "ret\n");
> -
> #else
> static void __test_function(volatile long *ptr)
> {
> @@ -302,10 +294,15 @@ bool test__bp_signal_is_supported(void)
> * stepping into the SIGIO handler and getting stuck on the
> * breakpointed instruction.
> *
> + * Since arm64 has the same issue with arm for the single-step
> + * handling, this case also gets suck on the breakpointed
> + * instruction.
Freudian slip?
Will
On Tue, Oct 22, 2019 at 02:14:25PM +0100, Will Deacon wrote:
[...]
> > diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
> > index c1c2c13..166f411 100644
> > --- a/tools/perf/tests/bp_signal.c
> > +++ b/tools/perf/tests/bp_signal.c
> > @@ -49,14 +49,6 @@ asm (
> > "__test_function:\n"
> > "incq (%rdi)\n"
> > "ret\n");
> > -#elif defined (__aarch64__)
> > -extern void __test_function(volatile long *ptr);
> > -asm (
> > - ".globl __test_function\n"
> > - "__test_function:\n"
> > - "str x30, [x0]\n"
> > - "ret\n");
> > -
> > #else
> > static void __test_function(volatile long *ptr)
> > {
> > @@ -302,10 +294,15 @@ bool test__bp_signal_is_supported(void)
> > * stepping into the SIGIO handler and getting stuck on the
> > * breakpointed instruction.
> > *
> > + * Since arm64 has the same issue with arm for the single-step
> > + * handling, this case also gets suck on the breakpointed
> > + * instruction.
>
> Freudian slip?
:D sorry for typo: s/suck/stuck.
Thanks for review and will send a patch to fix it.
Thanks,
Leo Yan