* [PATCH] perf: Don't use SIAR for user addresses
@ 2012-06-05 20:32 Sukadev Bhattiprolu
2012-06-05 22:43 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 2+ messages in thread
From: Sukadev Bhattiprolu @ 2012-06-05 20:32 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mpjohn, Anton Blanchard
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Fri, 1 Jun 2012 20:56:02 -0400
Subject: [PATCH] perf: Don't use SIAR for user-space addresses
With the pipelining on Power7, the SIAR can be off by several instructions
leading to incorrect callgraphs. For user space code at least we can be more
accurate by just using the next instruction pointer.
Based on code/input from Anton Blanchard.
Before this fon Power7, we see callgraphs like:
1.90% sprintft sprintft [.] do_my_sprintf
|
--- 00000011.plt_call.strlen@@GLIBC_2.3+0
do_my_sprintf
main
generic_start_main
__libc_start_main
(nil)
...snip...
|
--- __random
rand
|
|--63.16%-- do_my_sprintf
| main
| generic_start_main
| __libc_start_main
| (nil)
|
--36.84%-- rand
do_my_sprintf
main
generic_start_main
__libc_start_main
(nil)
which seems to indicate the libc __random() calls do_my_sprintf(), instead
of the other way around.
After the fix, the same trace looks like this:
4.08% sprintft sprintft [.] do_my_sprintf
|
--- do_my_sprintf
do_my_sprintf
main
generic_start_main
__libc_start_main
(nil)
Cc: Anton Blanchard <anton@samba.org>
Reported-by: Maynard Johnson <mpjohn@us.ibm.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
arch/powerpc/perf/core-book3s.c | 39 ++++++++++++++++++++++++++++++---------
1 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 8f84bcb..846bd68 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -116,6 +116,26 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
*addrp = mfspr(SPRN_SDAR);
}
+static bool mmcra_sihv(unsigned long mmcra)
+{
+ unsigned long sihv = MMCRA_SIHV;
+
+ if (ppmu->flags & PPMU_ALT_SIPR)
+ sihv = POWER6_MMCRA_SIHV;
+
+ return !!(mmcra & sihv);
+}
+
+static bool mmcra_sipr(unsigned long mmcra)
+{
+ unsigned long sipr = MMCRA_SIPR;
+
+ if (ppmu->flags & PPMU_ALT_SIPR)
+ sipr = POWER6_MMCRA_SIPR;
+
+ return !!(mmcra & sipr);
+}
+
static inline u32 perf_flags_from_msr(struct pt_regs *regs)
{
if (regs->msr & MSR_PR)
@@ -128,8 +148,6 @@ static inline u32 perf_flags_from_msr(struct pt_regs *regs)
static inline u32 perf_get_misc_flags(struct pt_regs *regs)
{
unsigned long mmcra = regs->dsisr;
- unsigned long sihv = MMCRA_SIHV;
- unsigned long sipr = MMCRA_SIPR;
/* Not a PMU interrupt: Make up flags from regs->msr */
if (TRAP(regs) != 0xf00)
@@ -156,15 +174,10 @@ static inline u32 perf_get_misc_flags(struct pt_regs *regs)
return PERF_RECORD_MISC_USER;
}
- if (ppmu->flags & PPMU_ALT_SIPR) {
- sihv = POWER6_MMCRA_SIHV;
- sipr = POWER6_MMCRA_SIPR;
- }
-
/* PR has priority over HV, so order below is important */
- if (mmcra & sipr)
+ if (mmcra_sipr(mmcra))
return PERF_RECORD_MISC_USER;
- if ((mmcra & sihv) && (freeze_events_kernel != MMCR0_FCHV))
+ if (mmcra_sihv(mmcra) && (freeze_events_kernel != MMCR0_FCHV))
return PERF_RECORD_MISC_HYPERVISOR;
return PERF_RECORD_MISC_KERNEL;
}
@@ -1340,6 +1353,14 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
!(mmcra & MMCRA_SAMPLE_ENABLE))
return regs->nip;
+ /*
+ * With pipelining on P7, addresses in the SIAR can be off by several
+ * instructions. For user space use the next instruction pointer as
+ * that will be more accurate.
+ */
+ if (mmcra_sipr(mmcra))
+ return regs->nip;
+
return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] perf: Don't use SIAR for user addresses
2012-06-05 20:32 [PATCH] perf: Don't use SIAR for user addresses Sukadev Bhattiprolu
@ 2012-06-05 22:43 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 2+ messages in thread
From: Benjamin Herrenschmidt @ 2012-06-05 22:43 UTC (permalink / raw)
To: Sukadev Bhattiprolu; +Cc: mpjohn, linuxppc-dev, Anton Blanchard
On Tue, 2012-06-05 at 13:32 -0700, Sukadev Bhattiprolu wrote:
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Fri, 1 Jun 2012 20:56:02 -0400
> Subject: [PATCH] perf: Don't use SIAR for user-space addresses
>
> With the pipelining on Power7, the SIAR can be off by several instructions
> leading to incorrect callgraphs. For user space code at least we can be more
> accurate by just using the next instruction pointer.
>
> Based on code/input from Anton Blanchard.
Had a chat with him, we still want to use SIAR for marked events, which
I think off the top of my head is mmcra & MMCRA_SAMPLE_ENABLE.
There's a few more considerations we discussed, such as wanting also
more precise backtraces in kernel space, but still capture in pHyp when
SIHV is set and have maybe some kind of sysfs option to revert back to
SIAR kernel-wide if we are trying to track down hard irq off stuff.
I'll let Anton followup. I'm still technically not working for a couple
of weeks :-)
Cheers,
Ben.
> Before this fon Power7, we see callgraphs like:
>
> 1.90% sprintft sprintft [.] do_my_sprintf
> |
> --- 00000011.plt_call.strlen@@GLIBC_2.3+0
> do_my_sprintf
> main
> generic_start_main
> __libc_start_main
> (nil)
> ...snip...
>
> |
> --- __random
> rand
> |
> |--63.16%-- do_my_sprintf
> | main
> | generic_start_main
> | __libc_start_main
> | (nil)
> |
> --36.84%-- rand
> do_my_sprintf
> main
> generic_start_main
> __libc_start_main
> (nil)
>
> which seems to indicate the libc __random() calls do_my_sprintf(), instead
> of the other way around.
>
> After the fix, the same trace looks like this:
>
> 4.08% sprintft sprintft [.] do_my_sprintf
> |
> --- do_my_sprintf
> do_my_sprintf
> main
> generic_start_main
> __libc_start_main
> (nil)
>
> Cc: Anton Blanchard <anton@samba.org>
> Reported-by: Maynard Johnson <mpjohn@us.ibm.com>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
> arch/powerpc/perf/core-book3s.c | 39 ++++++++++++++++++++++++++++++---------
> 1 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 8f84bcb..846bd68 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -116,6 +116,26 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
> *addrp = mfspr(SPRN_SDAR);
> }
>
> +static bool mmcra_sihv(unsigned long mmcra)
> +{
> + unsigned long sihv = MMCRA_SIHV;
> +
> + if (ppmu->flags & PPMU_ALT_SIPR)
> + sihv = POWER6_MMCRA_SIHV;
> +
> + return !!(mmcra & sihv);
> +}
> +
> +static bool mmcra_sipr(unsigned long mmcra)
> +{
> + unsigned long sipr = MMCRA_SIPR;
> +
> + if (ppmu->flags & PPMU_ALT_SIPR)
> + sipr = POWER6_MMCRA_SIPR;
> +
> + return !!(mmcra & sipr);
> +}
> +
> static inline u32 perf_flags_from_msr(struct pt_regs *regs)
> {
> if (regs->msr & MSR_PR)
> @@ -128,8 +148,6 @@ static inline u32 perf_flags_from_msr(struct pt_regs *regs)
> static inline u32 perf_get_misc_flags(struct pt_regs *regs)
> {
> unsigned long mmcra = regs->dsisr;
> - unsigned long sihv = MMCRA_SIHV;
> - unsigned long sipr = MMCRA_SIPR;
>
> /* Not a PMU interrupt: Make up flags from regs->msr */
> if (TRAP(regs) != 0xf00)
> @@ -156,15 +174,10 @@ static inline u32 perf_get_misc_flags(struct pt_regs *regs)
> return PERF_RECORD_MISC_USER;
> }
>
> - if (ppmu->flags & PPMU_ALT_SIPR) {
> - sihv = POWER6_MMCRA_SIHV;
> - sipr = POWER6_MMCRA_SIPR;
> - }
> -
> /* PR has priority over HV, so order below is important */
> - if (mmcra & sipr)
> + if (mmcra_sipr(mmcra))
> return PERF_RECORD_MISC_USER;
> - if ((mmcra & sihv) && (freeze_events_kernel != MMCR0_FCHV))
> + if (mmcra_sihv(mmcra) && (freeze_events_kernel != MMCR0_FCHV))
> return PERF_RECORD_MISC_HYPERVISOR;
> return PERF_RECORD_MISC_KERNEL;
> }
> @@ -1340,6 +1353,14 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
> !(mmcra & MMCRA_SAMPLE_ENABLE))
> return regs->nip;
>
> + /*
> + * With pipelining on P7, addresses in the SIAR can be off by several
> + * instructions. For user space use the next instruction pointer as
> + * that will be more accurate.
> + */
> + if (mmcra_sipr(mmcra))
> + return regs->nip;
> +
> return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
> }
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2012-06-05 22:43 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-05 20:32 [PATCH] perf: Don't use SIAR for user addresses Sukadev Bhattiprolu
2012-06-05 22:43 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).