[PATCH v1 1/3] perf tests: Remove needless headers for bp_account

[PATCH v1 1/3] perf tests: Remove needless headers for bp_account

[Leo Yan]

A few headers are not needed and were introduced by copying from other
test file.  This patch removes the needless headers for the breakpoint
accounting testing.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/tests/bp_account.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tools/perf/tests/bp_account.c b/tools/perf/tests/bp_account.c
index 016bba2c142d..52ff7a462670 100644
--- a/tools/perf/tests/bp_account.c
+++ b/tools/perf/tests/bp_account.c
@@ -10,11 +10,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <sys/ioctl.h>
-#include <time.h>
 #include <fcntl.h>
-#include <signal.h>
-#include <sys/mman.h>
-#include <linux/compiler.h>
 #include <linux/hw_breakpoint.h>
 
 #include "tests.h"
-- 
2.17.1

[PATCH v1 2/3] perf tests bp_account: Add dedicated checking helper is_supported()

[Leo Yan]

Arm architecture supports the breakpoint accounting but it doesn't
support breakpoint overflow signal handling.  The current code uses the
same checking helper, thus it disables both testings (bp_account and
bp_signal) for arm platform.

For handling two testings separately, this patch adds a dedicated
checking helper is_supported() for breakpoint accounting testing, thus
it allows supporting breakpoint accounting testing on arm platform; the
old helper test__bp_signal_is_supported() is only used to checking for
breakpoint overflow signal testing.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/tests/bp_account.c   | 16 ++++++++++++++++
 tools/perf/tests/builtin-test.c |  2 +-
 tools/perf/tests/tests.h        |  1 +
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/bp_account.c b/tools/perf/tests/bp_account.c
index 52ff7a462670..d0b935356274 100644
--- a/tools/perf/tests/bp_account.c
+++ b/tools/perf/tests/bp_account.c
@@ -188,3 +188,19 @@ int test__bp_accounting(struct test *test __maybe_unused, int subtest __maybe_un
 
 	return bp_accounting(wp_cnt, share);
 }
+
+bool test__bp_account_is_supported(void)
+{
+	/*
+	 * PowerPC and S390 do not support creation of instruction
+	 * breakpoints using the perf_event interface.
+	 *
+	 * Just disable the test for these architectures until these
+	 * issues are resolved.
+	 */
+#if defined(__powerpc__) || defined(__s390x__)
+	return false;
+#else
+	return true;
+#endif
+}
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 55774baffc2a..8b286e9b7549 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -121,7 +121,7 @@ static struct test generic_tests[] = {
 	{
 		.desc = "Breakpoint accounting",
 		.func = test__bp_accounting,
-		.is_supported = test__bp_signal_is_supported,
+		.is_supported = test__bp_account_is_supported,
 	},
 	{
 		.desc = "Watchpoint",
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 72912eb473cb..9837b6e93023 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -111,6 +111,7 @@ int test__map_groups__merge_in(struct test *t, int subtest);
 int test__time_utils(struct test *t, int subtest);
 
 bool test__bp_signal_is_supported(void);
+bool test__bp_account_is_supported(void);
 bool test__wp_is_supported(void);
 
 #if defined(__arm__) || defined(__aarch64__)
-- 
2.17.1

[PATCH v1 3/3] perf tests: Disable bp_signal testing for arm64

[Leo Yan]

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 <leo.yan@linaro.org>
---
 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

Re: [PATCH v1 1/3] perf tests: Remove needless headers for bp_account

[Arnaldo Carvalho de Melo]

Em Fri, Oct 18, 2019 at 04:55:29PM +0800, Leo Yan escreveu:
> A few headers are not needed and were introduced by copying from other
> test file.  This patch removes the needless headers for the breakpoint
> accounting testing.

Thanks, applied.
 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/tests/bp_account.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/tools/perf/tests/bp_account.c b/tools/perf/tests/bp_account.c
> index 016bba2c142d..52ff7a462670 100644
> --- a/tools/perf/tests/bp_account.c
> +++ b/tools/perf/tests/bp_account.c
> @@ -10,11 +10,7 @@
>  #include <unistd.h>
>  #include <string.h>
>  #include <sys/ioctl.h>
> -#include <time.h>
>  #include <fcntl.h>
> -#include <signal.h>
> -#include <sys/mman.h>
> -#include <linux/compiler.h>
>  #include <linux/hw_breakpoint.h>
>  
>  #include "tests.h"
> -- 
> 2.17.1

-- 

- Arnaldo

Re: [PATCH v1 2/3] perf tests bp_account: Add dedicated checking helper is_supported()

[Arnaldo Carvalho de Melo]

Em Fri, Oct 18, 2019 at 04:55:30PM +0800, Leo Yan escreveu:
> Arm architecture supports the breakpoint accounting but it doesn't
> support breakpoint overflow signal handling.  The current code uses the
> same checking helper, thus it disables both testings (bp_account and
> bp_signal) for arm platform.
> 
> For handling two testings separately, this patch adds a dedicated
> checking helper is_supported() for breakpoint accounting testing, thus
> it allows supporting breakpoint accounting testing on arm platform; the
> old helper test__bp_signal_is_supported() is only used to checking for
> breakpoint overflow signal testing.

Looks sensible,

Applied, thanks.

- Arnaldo
 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/tests/bp_account.c   | 16 ++++++++++++++++
>  tools/perf/tests/builtin-test.c |  2 +-
>  tools/perf/tests/tests.h        |  1 +
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/bp_account.c b/tools/perf/tests/bp_account.c
> index 52ff7a462670..d0b935356274 100644
> --- a/tools/perf/tests/bp_account.c
> +++ b/tools/perf/tests/bp_account.c
> @@ -188,3 +188,19 @@ int test__bp_accounting(struct test *test __maybe_unused, int subtest __maybe_un
>  
>  	return bp_accounting(wp_cnt, share);
>  }
> +
> +bool test__bp_account_is_supported(void)
> +{
> +	/*
> +	 * PowerPC and S390 do not support creation of instruction
> +	 * breakpoints using the perf_event interface.
> +	 *
> +	 * Just disable the test for these architectures until these
> +	 * issues are resolved.
> +	 */
> +#if defined(__powerpc__) || defined(__s390x__)
> +	return false;
> +#else
> +	return true;
> +#endif
> +}
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 55774baffc2a..8b286e9b7549 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -121,7 +121,7 @@ static struct test generic_tests[] = {
>  	{
>  		.desc = "Breakpoint accounting",
>  		.func = test__bp_accounting,
> -		.is_supported = test__bp_signal_is_supported,
> +		.is_supported = test__bp_account_is_supported,
>  	},
>  	{
>  		.desc = "Watchpoint",
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 72912eb473cb..9837b6e93023 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -111,6 +111,7 @@ int test__map_groups__merge_in(struct test *t, int subtest);
>  int test__time_utils(struct test *t, int subtest);
>  
>  bool test__bp_signal_is_supported(void);
> +bool test__bp_account_is_supported(void);
>  bool test__wp_is_supported(void);
>  
>  #if defined(__arm__) || defined(__aarch64__)
> -- 
> 2.17.1

-- 

- Arnaldo

Re: [PATCH v1 3/3] perf tests: Disable bp_signal testing for arm64

[Arnaldo Carvalho de Melo]

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 <leo.yan@linaro.org>
> ---
>  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

Re: [PATCH v1 3/3] perf tests: Disable bp_signal testing for arm64

[Leo Yan]

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 <leo.yan@linaro.org>
> > ---
> >  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

[tip: perf/core] perf tests bp_account: Add dedicated checking helper is_supported()

[tip-bot2 for Leo Yan]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     e533eadf6596451880f518949cbb964dbd6189ae
Gitweb:        https://git.kernel.org/tip/e533eadf6596451880f518949cbb964dbd6189ae
Author:        Leo Yan <leo.yan@linaro.org>
AuthorDate:    Fri, 18 Oct 2019 16:55:30 +08:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Sat, 19 Oct 2019 15:35:01 -03:00

perf tests bp_account: Add dedicated checking helper is_supported()

The arm architecture supports breakpoint accounting but it doesn't
support breakpoint overflow signal handling.  The current code uses the
same checking helper, thus it disables both testings (bp_account and
bp_signal) for arm platform.

For handling two testings separately, this patch adds a dedicated
checking helper is_supported() for breakpoint accounting testing, thus
it allows supporting breakpoint accounting testing on arm platform; the
old helper test__bp_signal_is_supported() is only used to checking for
breakpoint overflow signal testing.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Brajeswar Ghosh <brajeswar.linux@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Will Deacon <will@kernel.org>
Link: http://lore.kernel.org/lkml/20191018085531.6348-2-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/bp_account.c   | 16 ++++++++++++++++
 tools/perf/tests/builtin-test.c |  2 +-
 tools/perf/tests/tests.h        |  1 +
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/bp_account.c b/tools/perf/tests/bp_account.c
index 52ff7a4..d0b9353 100644
--- a/tools/perf/tests/bp_account.c
+++ b/tools/perf/tests/bp_account.c
@@ -188,3 +188,19 @@ int test__bp_accounting(struct test *test __maybe_unused, int subtest __maybe_un
 
 	return bp_accounting(wp_cnt, share);
 }
+
+bool test__bp_account_is_supported(void)
+{
+	/*
+	 * PowerPC and S390 do not support creation of instruction
+	 * breakpoints using the perf_event interface.
+	 *
+	 * Just disable the test for these architectures until these
+	 * issues are resolved.
+	 */
+#if defined(__powerpc__) || defined(__s390x__)
+	return false;
+#else
+	return true;
+#endif
+}
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 55774ba..8b286e9 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -121,7 +121,7 @@ static struct test generic_tests[] = {
 	{
 		.desc = "Breakpoint accounting",
 		.func = test__bp_accounting,
-		.is_supported = test__bp_signal_is_supported,
+		.is_supported = test__bp_account_is_supported,
 	},
 	{
 		.desc = "Watchpoint",
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 72912eb..9837b6e 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -111,6 +111,7 @@ int test__map_groups__merge_in(struct test *t, int subtest);
 int test__time_utils(struct test *t, int subtest);
 
 bool test__bp_signal_is_supported(void);
+bool test__bp_account_is_supported(void);
 bool test__wp_is_supported(void);
 
 #if defined(__arm__) || defined(__aarch64__)

[tip: perf/core] perf tests: Disable bp_signal testing for arm64

[tip-bot2 for Leo Yan]

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 <leo.yan@linaro.org>
AuthorDate:    Fri, 18 Oct 2019 16:55:31 +08:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
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 <leo.yan@linaro.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Brajeswar Ghosh <brajeswar.linux@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Will Deacon <will@kernel.org>
Link: http://lore.kernel.org/lkml/20191018085531.6348-3-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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;

[tip: perf/core] perf tests: Remove needless headers for bp_account

[tip-bot2 for Leo Yan]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     12d795637ba169650ea10ad6babcc5425255944a
Gitweb:        https://git.kernel.org/tip/12d795637ba169650ea10ad6babcc5425255944a
Author:        Leo Yan <leo.yan@linaro.org>
AuthorDate:    Fri, 18 Oct 2019 16:55:29 +08:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Sat, 19 Oct 2019 15:35:01 -03:00

perf tests: Remove needless headers for bp_account

A few headers are not needed and were introduced by copying from other
test file.  This patch removes the needless headers for the breakpoint
accounting testing.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Brajeswar Ghosh <brajeswar.linux@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Will Deacon <will@kernel.org>
Link: http://lore.kernel.org/lkml/20191018085531.6348-1-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/bp_account.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tools/perf/tests/bp_account.c b/tools/perf/tests/bp_account.c
index 016bba2..52ff7a4 100644
--- a/tools/perf/tests/bp_account.c
+++ b/tools/perf/tests/bp_account.c
@@ -10,11 +10,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <sys/ioctl.h>
-#include <time.h>
 #include <fcntl.h>
-#include <signal.h>
-#include <sys/mman.h>
-#include <linux/compiler.h>
 #include <linux/hw_breakpoint.h>
 
 #include "tests.h"

Re: [tip: perf/core] perf tests: Disable bp_signal testing for arm64

[Will Deacon]

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 <leo.yan@linaro.org>
> AuthorDate:    Fri, 18 Oct 2019 16:55:31 +08:00
> Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
> 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 <leo.yan@linaro.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Brajeswar Ghosh <brajeswar.linux@gmail.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Michael Petlan <mpetlan@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: Will Deacon <will@kernel.org>
> Link: http://lore.kernel.org/lkml/20191018085531.6348-3-leo.yan@linaro.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  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

Re: [tip: perf/core] perf tests: Disable bp_signal testing for arm64

[Leo Yan]

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