perf script: Fix LBR skid dump problems in brstackinsn
PATCH 1/1 perf script: Fix LBR skid dump problems in brstackinsn
From: Andi Kleen <[email protected]>
This is a fix for another instance of the skid problem Milian recently found [1]
The LBRs don't freeze at the exact same time as the PMI is triggered. The perf script brstackinsn code that dumps LBR assembler assumes that the last branch in the LBR leads to the sample point. But with skid it's possible that the CPU executes one or more branches before the sample, but which do not appear in the LBR.
What happens then is either that the sample point is before the last LBR branch. In this case the dumper sees a negative length and ignores it. Or it the sample point is long after the last branch. Then the dumper sees a very long block and dumps it upto its block limit (16k bytes), which is noise in the output.
On typical sample session this can happen regularly.
This patch tries to detect and handle the situation. On the last block that is dumped by the LBR dumper we always stop on the first branch. If the block length is negative just scan forward to the first branch. Otherwise scan until a branch is found.
The PT decoder already has a function that uses the instruction decoder to detect branches, so we can just reuse it here.
Then when a terminating branch is found print an indication and stop dumping. This might miss a few instructions, but at least shows no runaway blocks.
--- tools/perf/builtin-script.c | 18 +++++++++++++++++- tools/perf/util/dump-insn.c | 8 ++++++++ tools/perf/util/dump-insn.h | 2 ++ .../intel-pt-decoder/intel-pt-insn-decoder.c | 8 ++++++++ 4 files changed, 35 insertions(+), 1 deletion(-)
tools/perf/builtin-script.c +17 -1
@@ -1049,9 +1049,18 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample,/* * Print final block upto sample +* +* Due to pipeline delays the LBRs might be missing a branch +* or two, which can result in very large or negative blocks +* between final branch and sample. When this happens just +* continue walking after the last TO until we hit a branch. */ start = br->entries[0 ].to; end = sample->ip; +if (end < start) { +/* Missing jump. Scan 128 bytes for the next branch */ + end = start +128 ; + } len =grab_bb (buffer, start, end, machine, thread, &x.is64bit, &x.cpumode, true); printed +=ip__fprintf_sym (start, thread, x.cpumode, x.cpu, &lastsym, attr, fp);if (len <=0 ) {
@@ -1060,7 +1069,6 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, machine, thread, &x.is64bit, &x.cpumode, false);if (len <=0 ) goto out; - printed +=fprintf (fp,"\t%016" PRIx64"\t%s\n" , sample->ip,dump_insn (&x, sample->ip, buffer, len,NULL )); goto out;
@@ -1070,6 +1078,14 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, dump_insn(&x, start + off, buffer + off, len - off, &ilen));if (ilen ==0 )break ; +if (arch_is_branch (buffer + off, len - off, x.is64bit) && + start + off != sample->ip) { +/* +* Hit a missing branch. Just stop. +*/ + printed +=fprintf (fp,"\t... not reaching sample ...\n" ); +break ; + } } out:return printed;
tools/perf/util/dump-insn.c +8 -0
@@ -13,3 +13,11 @@ const char *dump_insn(struct perf_insn *x __maybe_unused, *lenp =0 ;return "?" ; } + +__weak +int arch_is_branch (const unsigned char *buf__maybe_unused , +size_t len__maybe_unused , +int x86_64__maybe_unused ) +{ +return 0 ; +}
tools/perf/util/dump-insn.h +2 -0
@@ -20,4 +20,6 @@ struct perf_insn {const char *dump_insn (struct perf_insn *x,u64 ip,u8 *inbuf,int inlen,int *lenp); +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64); +#endif
tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c +8 -0
@@ -180,6 +180,14 @@ int intel_pt_get_insn(const unsigned char *buf, size_t len, int x86_64,return 0 ; } +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64) +{ +struct intel_pt_insn in; +if (intel_pt_get_insn (buf, len, x86_64, &in) <0 ) +return -1 ; +return in.branch !=INTEL_PT_BR_NO_BRANCH ; +} +const char *dump_insn (struct perf_insn *x,uint64_t ip__maybe_unused ,u8 *inbuf,int inlen,int *lenp) {
Signature · 1 line
2.17.2
Arnaldo Carvalho de Melo
maintainerEm Mon, Nov 19, 2018 at 09:06:17PM -0800, Andi Kleen escreveu:
Quoted 4 lines
From: Andi Kleen <[email protected]>
This is a fix for another instance of the skid problem Milian recently found [1]
Milian, have you tested this?
Adrian, can I have your Reviewed-by or Acked-by?
Thanks,
- Arnaldo
Quoted 130 lines
The LBRs don't freeze at the exact same time as the PMI is triggered. The perf script brstackinsn code that dumps LBR assembler assumes that the last branch in the LBR leads to the sample point. But with skid it's possible that the CPU executes one or more branches before the sample, but which do not appear in the LBR.
What happens then is either that the sample point is before the last LBR branch. In this case the dumper sees a negative length and ignores it. Or it the sample point is long after the last branch. Then the dumper sees a very long block and dumps it upto its block limit (16k bytes), which is noise in the output.
On typical sample session this can happen regularly.
This patch tries to detect and handle the situation. On the last block that is dumped by the LBR dumper we always stop on the first branch. If the block length is negative just scan forward to the first branch. Otherwise scan until a branch is found.
The PT decoder already has a function that uses the instruction decoder to detect branches, so we can just reuse it here.
Then when a terminating branch is found print an indication and stop dumping. This might miss a few instructions, but at least shows no runaway blocks.
--- tools/perf/builtin-script.c | 18 +++++++++++++++++- tools/perf/util/dump-insn.c | 8 ++++++++ tools/perf/util/dump-insn.h | 2 ++ .../intel-pt-decoder/intel-pt-insn-decoder.c | 8 ++++++++ 4 files changed, 35 insertions(+), 1 deletion(-)tools/perf/builtin-script.c +17
-1@@ -1049,9 +1049,18 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample,/* * Print final block upto sample +* +* Due to pipeline delays the LBRs might be missing a branch +* or two, which can result in very large or negative blocks +* between final branch and sample. When this happens just +* continue walking after the last TO until we hit a branch. */ start = br->entries[0 ].to; end = sample->ip; +if (end < start) { +/* Missing jump. Scan 128 bytes for the next branch */ + end = start +128 ; + } len =grab_bb (buffer, start, end, machine, thread, &x.is64bit, &x.cpumode, true); printed +=ip__fprintf_sym (start, thread, x.cpumode, x.cpu, &lastsym, attr, fp);if (len <=0 ) {@@ -1060,7 +1069,6 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, machine, thread, &x.is64bit, &x.cpumode, false);if (len <=0 ) goto out; - printed +=fprintf (fp,"\t%016" PRIx64"\t%s\n" , sample->ip,dump_insn (&x, sample->ip, buffer, len,NULL )); goto out;@@ -1070,6 +1078,14 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, dump_insn(&x, start + off, buffer + off, len - off, &ilen));if (ilen ==0 )break ; +if (arch_is_branch (buffer + off, len - off, x.is64bit) && + start + off != sample->ip) { +/* +* Hit a missing branch. Just stop. +*/ + printed +=fprintf (fp,"\t... not reaching sample ...\n" ); +break ; + } } out:return printed;tools/perf/util/dump-insn.c +8
-0@@ -13,3 +13,11 @@ const char *dump_insn(struct perf_insn *x __maybe_unused, *lenp =0 ;return "?" ; } + +__weak +int arch_is_branch (const unsigned char *buf__maybe_unused , +size_t len__maybe_unused , +int x86_64__maybe_unused ) +{ +return 0 ; +}tools/perf/util/dump-insn.h +2
-0@@ -20,4 +20,6 @@ struct perf_insn {const char *dump_insn (struct perf_insn *x,u64 ip,u8 *inbuf,int inlen,int *lenp); +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64); +#endif tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c +8
-0@@ -180,6 +180,14 @@ int intel_pt_get_insn(const unsigned char *buf, size_t len, int x86_64,return 0 ; } +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64) +{ +struct intel_pt_insn in; +if (intel_pt_get_insn (buf, len, x86_64, &in) <0 ) +return -1 ; +return in.branch !=INTEL_PT_BR_NO_BRANCH ; +} +const char *dump_insn (struct perf_insn *x,uint64_t ip__maybe_unused ,u8 *inbuf,int inlen,int *lenp) {Signature · 1 line
2.17.2
Signature · 1 line
- Arnaldo
Quoted 7 lines · Arnaldo Carvalho de Melo
Em Mon, Nov 19, 2018 at 09:06:17PM -0800, Andi Kleen escreveu:
Quoted 4 lines
From: Andi Kleen <[email protected]>
This is a fix for another instance of the skid problem Milian recently found [1]
Milian, have you tested this?
It won't fix Milians problem, which were with call graphs.
For Milian's issue you would need Milian's patches, which AFAIK haven't been reviewed by anyone.
-Andi
Arnaldo Carvalho de Melo
maintainerEm Thu, Dec 06, 2018 at 12:51:48PM -0800, Andi Kleen escreveu:
Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 5 lines
Em Mon, Nov 19, 2018 at 09:06:17PM -0800, Andi Kleen escreveu:
Quoted 4 lines
From: Andi Kleen <[email protected]>
This is a fix for another instance of the skid problem Milian recently found [1]
I think you forgot to add the reference, i.e. what is the url or message-id that this [1] refers to?
Quoted 6 lines
Quoted 1 line
Milian, have you tested this?
It won't fix Milians problem, which were with call graphs.
For Milian's issue you would need Milian's patches, which AFAIK haven't been reviewed by anyone.
/me trying to find these patches...
- Arnaldo
Quoted 10 lines · Arnaldo Carvalho de Melo
Em Thu, Dec 06, 2018 at 12:51:48PM -0800, Andi Kleen escreveu:
Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 5 lines
Em Mon, Nov 19, 2018 at 09:06:17PM -0800, Andi Kleen escreveu:
Quoted 4 lines
From: Andi Kleen <[email protected]>
This is a fix for another instance of the skid problem Milian recently found [1]
I think you forgot to add the reference, i.e. what is the url or message-id that this [1] refers to?
Hmm, I thought I saw some patches from Milian for this earlier, but now I can't find them. Perhaps I misremember. Milian can point to them if they exist and are not just a figment of my imagination :-)
These were the changes to report the stack frame RIP/RSP in the PEBS handler and use it for unwinding in perf.
-Andi
Quoted 3 lines · On Donnerstag, 6. Dezember 2018 23:52:07 CET Andi Kleen wrote:
Quoted 2 lines · Arnaldo Carvalho de Melo
Em Thu, Dec 06, 2018 at 12:51:48PM -0800, Andi Kleen escreveu:
Quoted 1 line
On Thu, Dec 06, 2018 at 02:01:40PM -0300, Arnaldo Carvalho de Melo
Quoted 6 lines · wrote:
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Em Mon, Nov 19, 2018 at 09:06:17PM -0800, Andi Kleen escreveu:
Quoted 4 lines
From: Andi Kleen <[email protected]>
This is a fix for another instance of the skid problem Milian recently found [1]
Quoted 7 lines
Quoted 2 lines
I think you forgot to add the reference, i.e. what is the url or message-id that this [1] refers to?
Hmm, I thought I saw some patches from Milian for this earlier, but now I can't find them. Perhaps I misremember. Milian can point to them if they exist and are not just a figment of my imagination :-)
I only have very early POC patches, cf.: https://lkml.org/lkml/2018/11/14/608
I've now also pushed that on my WIP branch: https://github.com/milianw/linux/
tree/pebs-callchain-breakage
I haven't had the time since to work on this. The patches as-is are not upstreamable. There are some open questions on my side (see mail referenced above).
Quoted 2 lines
These were the changes to report the stack frame RIP/RSP in the PEBS handler and use it for unwinding in perf.
Yes, I was looking at something different. I've no experience with brstackinsn usage in perf, so I can't really add my tested-by.
Cheers
Signature · 4 lines
Milian Wolff | [email protected] | Senior Software Engineer KDAB (Deutschland) GmbH, a KDAB Group company Tel: +49-30-521325470 KDAB - The Qt, C++ and OpenGL Experts
Quoted 9 lines · Arnaldo Carvalho de Melo
Em Mon, Nov 19, 2018 at 09:06:17PM -0800, Andi Kleen escreveu:
Quoted 4 lines
From: Andi Kleen <[email protected]>
This is a fix for another instance of the skid problem Milian recently found [1]
Milian, have you tested this?
Adrian, can I have your Reviewed-by or Acked-by?
Quoted 136 lines
Thanks,
- Arnaldo
Quoted 130 lines
The LBRs don't freeze at the exact same time as the PMI is triggered. The perf script brstackinsn code that dumps LBR assembler assumes that the last branch in the LBR leads to the sample point. But with skid it's possible that the CPU executes one or more branches before the sample, but which do not appear in the LBR.
What happens then is either that the sample point is before the last LBR branch. In this case the dumper sees a negative length and ignores it. Or it the sample point is long after the last branch. Then the dumper sees a very long block and dumps it upto its block limit (16k bytes), which is noise in the output.
On typical sample session this can happen regularly.
This patch tries to detect and handle the situation. On the last block that is dumped by the LBR dumper we always stop on the first branch. If the block length is negative just scan forward to the first branch. Otherwise scan until a branch is found.
The PT decoder already has a function that uses the instruction decoder to detect branches, so we can just reuse it here.
Then when a terminating branch is found print an indication and stop dumping. This might miss a few instructions, but at least shows no runaway blocks.
--- tools/perf/builtin-script.c | 18 +++++++++++++++++- tools/perf/util/dump-insn.c | 8 ++++++++ tools/perf/util/dump-insn.h | 2 ++ .../intel-pt-decoder/intel-pt-insn-decoder.c | 8 ++++++++ 4 files changed, 35 insertions(+), 1 deletion(-)tools/perf/builtin-script.c +17
-1@@ -1049,9 +1049,18 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample,/* * Print final block upto sample +* +* Due to pipeline delays the LBRs might be missing a branch +* or two, which can result in very large or negative blocks +* between final branch and sample. When this happens just +* continue walking after the last TO until we hit a branch. */ start = br->entries[0 ].to; end = sample->ip; +if (end < start) { +/* Missing jump. Scan 128 bytes for the next branch */ + end = start +128 ; + } len =grab_bb (buffer, start, end, machine, thread, &x.is64bit, &x.cpumode, true); printed +=ip__fprintf_sym (start, thread, x.cpumode, x.cpu, &lastsym, attr, fp);if (len <=0 ) {@@ -1060,7 +1069,6 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, machine, thread, &x.is64bit, &x.cpumode, false);if (len <=0 ) goto out; - printed +=fprintf (fp,"\t%016" PRIx64"\t%s\n" , sample->ip,dump_insn (&x, sample->ip, buffer, len,NULL )); goto out;@@ -1070,6 +1078,14 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, dump_insn(&x, start + off, buffer + off, len - off, &ilen));if (ilen ==0 )break ; +if (arch_is_branch (buffer + off, len - off, x.is64bit) && + start + off != sample->ip) { +/* +* Hit a missing branch. Just stop. +*/ + printed +=fprintf (fp,"\t... not reaching sample ...\n" ); +break ; + } } out:return printed;tools/perf/util/dump-insn.c +8
-0@@ -13,3 +13,11 @@ const char *dump_insn(struct perf_insn *x __maybe_unused, *lenp =0 ;return "?" ; } + +__weak +int arch_is_branch (const unsigned char *buf__maybe_unused , +size_t len__maybe_unused , +int x86_64__maybe_unused ) +{ +return 0 ; +}tools/perf/util/dump-insn.h +2
-0@@ -20,4 +20,6 @@ struct perf_insn {const char *dump_insn (struct perf_insn *x,u64 ip,u8 *inbuf,int inlen,int *lenp); +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64); +#endif tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c +8
-0@@ -180,6 +180,14 @@ int intel_pt_get_insn(const unsigned char *buf, size_t len, int x86_64,return 0 ; } +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64) +{ +struct intel_pt_insn in; +if (intel_pt_get_insn (buf, len, x86_64, &in) <0 ) +return -1 ; +return in.branch !=INTEL_PT_BR_NO_BRANCH ; +} +const char *dump_insn (struct perf_insn *x,uint64_t ip__maybe_unused ,u8 *inbuf,int inlen,int *lenp) {Signature · 1 line
2.17.2
tip-bot for Andi Kleen
merge botperf script: Fix LBR skid dump problems in brstackinsn
Commit-ID: 61f611593f2c90547cb09c0bf6977414454a27e6 Gitweb: https://git.kernel.org/tip/61f611593f2c90547cb09c0bf6977414454a27e6 Author: Andi Kleen <[email protected]> AuthorDate: Mon, 19 Nov 2018 21:06:17 -0800 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Fri, 28 Dec 2018 16:33:02 -0300
perf script: Fix LBR skid dump problems in brstackinsn
This is a fix for another instance of the skid problem Milian recently found [1]
The LBRs don't freeze at the exact same time as the PMI is triggered. The perf script brstackinsn code that dumps LBR assembler assumes that the last branch in the LBR leads to the sample point. But with skid it's possible that the CPU executes one or more branches before the sample, but which do not appear in the LBR.
What happens then is either that the sample point is before the last LBR branch. In this case the dumper sees a negative length and ignores it. Or it the sample point is long after the last branch. Then the dumper sees a very long block and dumps it upto its block limit (16k bytes), which is noise in the output.
On typical sample session this can happen regularly.
This patch tries to detect and handle the situation. On the last block that is dumped by the LBR dumper we always stop on the first branch. If the block length is negative just scan forward to the first branch. Otherwise scan until a branch is found.
The PT decoder already has a function that uses the instruction decoder to detect branches, so we can just reuse it here.
Then when a terminating branch is found print an indication and stop dumping. This might miss a few instructions, but at least shows no runaway blocks.
--- tools/perf/builtin-script.c | 17 ++++++++++++++++- tools/perf/util/dump-insn.c | 8 ++++++++ tools/perf/util/dump-insn.h | 2 ++ .../perf/util/intel-pt-decoder/intel-pt-insn-decoder.c | 8 ++++++++ 4 files changed, 34 insertions(+), 1 deletion(-)
tools/perf/builtin-script.c +16 -1
@@ -1073,9 +1073,18 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample,/* * Print final block upto sample +* +* Due to pipeline delays the LBRs might be missing a branch +* or two, which can result in very large or negative blocks +* between final branch and sample. When this happens just +* continue walking after the last TO until we hit a branch. */ start = br->entries[0 ].to; end = sample->ip; +if (end < start) { +/* Missing jump. Scan 128 bytes for the next branch */ + end = start +128 ; + } len =grab_bb (buffer, start, end, machine, thread, &x.is64bit, &x.cpumode, true); printed +=ip__fprintf_sym (start, thread, x.cpumode, x.cpu, &lastsym, attr, fp);if (len <=0 ) {
@@ -1084,7 +1093,6 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, machine, thread, &x.is64bit, &x.cpumode, false);if (len <=0 ) goto out; - printed +=fprintf (fp,"\t%016" PRIx64"\t%s\n" , sample->ip,dump_insn (&x, sample->ip, buffer, len,NULL ));if (PRINT_FIELD (SRCCODE ))
@@ -1096,6 +1104,13 @@ static int perf_sample__fprintf_brstackinsn(struct perf_sample *sample, dump_insn(&x, start + off, buffer + off, len - off, &ilen));if (ilen ==0 )break ; +if (arch_is_branch (buffer + off, len - off, x.is64bit) && start + off != sample->ip) { +/* +* Hit a missing branch. Just stop. +*/ + printed +=fprintf (fp,"\t... not reaching sample ...\n" ); +break ; + }if (PRINT_FIELD (SRCCODE ))print_srccode (thread, x.cpumode, start + off); }
tools/perf/util/dump-insn.c +8 -0
@@ -13,3 +13,11 @@ const char *dump_insn(struct perf_insn *x __maybe_unused, *lenp =0 ;return "?" ; } + +__weak +int arch_is_branch (const unsigned char *buf__maybe_unused , +size_t len__maybe_unused , +int x86_64__maybe_unused ) +{ +return 0 ; +}
tools/perf/util/dump-insn.h +2 -0
@@ -20,4 +20,6 @@ struct perf_insn {const char *dump_insn (struct perf_insn *x,u64 ip,u8 *inbuf,int inlen,int *lenp); +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64); +#endif
tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c +8 -0
@@ -180,6 +180,14 @@ int intel_pt_get_insn(const unsigned char *buf, size_t len, int x86_64,return 0 ; } +int arch_is_branch (const unsigned char *buf,size_t len,int x86_64) +{ +struct intel_pt_insn in; +if (intel_pt_get_insn (buf, len, x86_64, &in) <0 ) +return -1 ; +return in.branch !=INTEL_PT_BR_NO_BRANCH ; +} +const char *dump_insn (struct perf_insn *x,uint64_t ip__maybe_unused ,u8 *inbuf,int inlen,int *lenp) {