From: James Clark <hidden> Date: 2021-07-29 16:01:07
I'm submitting this as an RFC because there are a few changes I'd
like to get feedback on. The two changes I wanted to make were the
last two for Coresight warnings, but I ended up making some perf-wide
changes along the way.
For #1 (perf tools: Add WARN_ONCE equivalent for UI warnings)
* Does it make sense to add warn once equivalents at all, or
should the once be re-done for each usage?
* Or should there be some kind of generic 'once' wrapper?
For #3 (perf tools: Add disassembly warnings for annotate --stdio)
* If the output is interpreted by any other tools, then adding
these warnings could be an issue, so maybe this change could
be dropped, but no error output at all isn't ideal.
For #4 (perf tools: Add flag for tracking warnings of missing DSOs)
* In theory I could re-use 'annotate_warned', but it might make sense
to rename it in that case, or just leave the new auxtrace_warned and
remove any confusion.
This set applies to perf/core e73f0f0ee7541
Thanks
James
James Clark (6):
perf tools: Add WARN_ONCE equivalent for UI warnings
perf tools: Re-add annotate_warned functionality
perf tools: Add disassembly warnings for annotate --stdio
perf tools: Add flag for tracking warnings of missing DSOs
perf cs-etm: Improve Coresight zero timestamp warning
perf cs-etm: Add warnings for missing DSOs
tools/perf/ui/browsers/annotate.c | 1 +
tools/perf/ui/gtk/annotate.c | 1 +
tools/perf/util/annotate.c | 20 +++++++++++++++++--
.../perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +++++--
tools/perf/util/cs-etm.c | 10 +++++++++-
tools/perf/util/debug.h | 14 +++++++++++++
tools/perf/util/dso.h | 1 +
7 files changed, 49 insertions(+), 5 deletions(-)
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: James Clark <hidden> Date: 2021-07-29 16:00:20
Currently WARN_ONCE prints to stderr and corrupts the TUI. Add
equivalent methods for UI warnings.
Signed-off-by: James Clark <redacted>
---
tools/perf/util/debug.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
From: James Clark <hidden> Date: 2021-07-29 16:00:44
Currently 'perf annotate --stdio' (and --stdio2) will exit without
printing anything if there are disassembly errors. Apply the same
error handler that's used for TUI and GTK modes. This makes comparing
disassembly across the different modes more consistent.
Signed-off-by: James Clark <redacted>
---
tools/perf/util/annotate.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
From: James Clark <hidden> Date: 2021-07-29 16:00:47
Auxtrace support may need DSOs for decoding (for example Arm
Coresight). If one of these is missing it would make sense to
warn once for each one that's missing, but not flood the output
with every address as there could be thousands of lookups.
This flag will allow tracking whether a warning was shown for
each DSO.
Signed-off-by: James Clark <redacted>
---
tools/perf/util/dso.h | 1 +
1 file changed, 1 insertion(+)
From: James Clark <hidden> Date: 2021-07-29 16:00:56
Setting annotate_warned to true on errors was removed in
commit ee51d851392e ("perf annotate: Introduce strerror for handling
symbol__disassemble() errors") which means when 'perf annotate
--skip-missing' is used warnings are shown multiple times for the same
DSO.
Setting this again restores the original behavior of only one warning
each.
Signed-off-by: James Clark <redacted>
---
tools/perf/ui/browsers/annotate.c | 1 +
tools/perf/ui/gtk/annotate.c | 1 +
2 files changed, 2 insertions(+)
From: James Clark <hidden> Date: 2021-07-29 16:01:17
Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.
Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,len=dso__data_read_offset(al.map->dso,machine,offset,buffer,size);-if(len<=0)+if(len<=0){+ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");+if(!al.map->dso->auxtrace_warned){+pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",+address,+al.map->dso->long_name?al.map->dso->long_name:"Unknown");+al.map->dso->auxtrace_warned=true;+}return0;+}returnlen;}
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: James Clark <hidden> Date: 2021-07-29 16:01:37
Only show the warning if the user hasn't already set timeless mode and
improve the text because there was ambiguity around the meaning of '...'
Change the warning to a UI warning instead of printing straight to
stderr because this corrupts the UI when perf report TUI is used. The UI
warning function also handles printing to stderr when in perf script
mode.
Suggested-by: Leo Yan <redacted>
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
@@ -324,8 +324,11 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,*underflow.*/packet_queue->cs_timestamp=0;-WARN_ONCE(true,"Zero Coresight timestamp found at Idx:%"OCSD_TRC_IDX_STR-". Decoding may be improved with --itrace=Z...\n",indx);+if(!cs_etm__etmq_is_timeless(etmq))+pr_warning_once("Zero Coresight timestamp found at Idx:%"OCSD_TRC_IDX_STR+". Decoding may be improved by prepending 'Z' to your current --itrace arguments.\n",+indx);+}elseif(packet_queue->instr_count>elem->timestamp){/**Sanitycheckthattheelem->timestamp-packet_queue->instr_countwouldnot
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Arnaldo Carvalho de Melo <acme@kernel.org> Date: 2021-08-02 14:52:03
Em Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark escreveu:
Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.
Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.
Looks ok to me (the last 3 patches in this series, the rest I applied
already), can I get some Acked-by/Reviewed-by from the CoreSight people?
Thanks,
- Arnaldo
quoted hunk
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,len=dso__data_read_offset(al.map->dso,machine,offset,buffer,size);-if(len<=0)+if(len<=0){+ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");+if(!al.map->dso->auxtrace_warned){+pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",+address,+al.map->dso->long_name?al.map->dso->long_name:"Unknown");+al.map->dso->auxtrace_warned=true;+}return0;+}returnlen;}
On Thu, Jul 29, 2021 at 04:58:04PM +0100, James Clark wrote:
Only show the warning if the user hasn't already set timeless mode and
improve the text because there was ambiguity around the meaning of '...'
Change the warning to a UI warning instead of printing straight to
stderr because this corrupts the UI when perf report TUI is used. The UI
warning function also handles printing to stderr when in perf script
mode.
Suggested-by: Leo Yan <redacted>
Signed-off-by: James Clark <redacted>
@@ -324,8 +324,11 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,*underflow.*/packet_queue->cs_timestamp=0;-WARN_ONCE(true,"Zero Coresight timestamp found at Idx:%"OCSD_TRC_IDX_STR-". Decoding may be improved with --itrace=Z...\n",indx);+if(!cs_etm__etmq_is_timeless(etmq))+pr_warning_once("Zero Coresight timestamp found at Idx:%"OCSD_TRC_IDX_STR+". Decoding may be improved by prepending 'Z' to your current --itrace arguments.\n",+indx);+}elseif(packet_queue->instr_count>elem->timestamp){/**Sanitycheckthattheelem->timestamp-packet_queue->instr_countwouldnot
On Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark wrote:
quoted hunk
Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.
Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,len=dso__data_read_offset(al.map->dso,machine,offset,buffer,size);-if(len<=0)+if(len<=0){+ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");+if(!al.map->dso->auxtrace_warned){+pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",+address,+al.map->dso->long_name?al.map->dso->long_name:"Unknown");+al.map->dso->auxtrace_warned=true;+}
This is very useful.
Just one comment: in particularly if the perf fails to find the kernel
symbols, the user needs to enable config "CONFIG_PROC_KCORE=y" or
specify option "-k /path/to/vmlinux". In this case, using 'perf
archive' is not helpful. So I think the UI warning can be imporved
like:
ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n"
" Enable CONFIG_PROC_KCORE or use option '-k /path/to/vmlinux' for kernel symbols\n");
With this improvement, the patch looks good to me:
Reviewed-by: Leo Yan <redacted>
On Mon, Aug 02, 2021 at 11:51:58AM -0300, Arnaldo Carvalho de Melo wrote:
Em Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark escreveu:
quoted
Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.
Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.
Looks ok to me (the last 3 patches in this series, the rest I applied
already), can I get some Acked-by/Reviewed-by from the CoreSight people?
I have a substantial backlog of patches to go through and I will be away for the
next two weeks. As such it will be some time before I get to review this set.
Regards,
Mathieu
Thanks,
- Arnaldo
quoted
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,len=dso__data_read_offset(al.map->dso,machine,offset,buffer,size);-if(len<=0)+if(len<=0){+ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");+if(!al.map->dso->auxtrace_warned){+pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",+address,+al.map->dso->long_name?al.map->dso->long_name:"Unknown");+al.map->dso->auxtrace_warned=true;+}return0;+}returnlen;}
From: Mike Leach <hidden> Date: 2021-08-03 08:06:42
HI James,
On Mon, 2 Aug 2021 at 18:03, Mathieu Poirier [off-list ref] wrote:
On Mon, Aug 02, 2021 at 11:51:58AM -0300, Arnaldo Carvalho de Melo wrote:
quoted
Em Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark escreveu:
quoted
Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.
The decoder actually outputs a OCSD_GEN_TRC_ELEM_ADDR_NACC packet if
binary memory data is missing.
These packets are currently ignored by perf / cs-etm-decoder.c.
I think this per DSO warning is fine, but perhaps at some point add in
handling for OCSD_GEN_TRC_ELEM_ADDR_NACC - which perhaps is only
active when dumping trace packets.
Regards
Mike
quoted
quoted
Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.
Looks ok to me (the last 3 patches in this series, the rest I applied
already), can I get some Acked-by/Reviewed-by from the CoreSight people?
I have a substantial backlog of patches to go through and I will be away for the
next two weeks. As such it will be some time before I get to review this set.
Regards,
Mathieu
quoted
Thanks,
- Arnaldo
quoted
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,len=dso__data_read_offset(al.map->dso,machine,offset,buffer,size);-if(len<=0)+if(len<=0){+ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");+if(!al.map->dso->auxtrace_warned){+pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",+address,+al.map->dso->long_name?al.map->dso->long_name:"Unknown");+al.map->dso->auxtrace_warned=true;+}return0;+}returnlen;}--
2.28.0
--
- Arnaldo
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Arnaldo Carvalho de Melo <acme@kernel.org> Date: 2021-08-03 13:24:15
Em Mon, Aug 02, 2021 at 11:41:45PM +0800, Leo Yan escreveu:
On Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark wrote:
quoted
Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.
Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,len=dso__data_read_offset(al.map->dso,machine,offset,buffer,size);-if(len<=0)+if(len<=0){+ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");+if(!al.map->dso->auxtrace_warned){+pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",+address,+al.map->dso->long_name?al.map->dso->long_name:"Unknown");+al.map->dso->auxtrace_warned=true;+}
This is very useful.
Just one comment: in particularly if the perf fails to find the kernel
symbols, the user needs to enable config "CONFIG_PROC_KCORE=y" or
specify option "-k /path/to/vmlinux". In this case, using 'perf
archive' is not helpful. So I think the UI warning can be imporved
like:
ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n"
" Enable CONFIG_PROC_KCORE or use option '-k /path/to/vmlinux' for kernel symbols\n");
From: Arnaldo Carvalho de Melo <acme@kernel.org> Date: 2021-08-03 13:25:46
Em Mon, Aug 02, 2021 at 11:17:10PM +0800, Leo Yan escreveu:
On Thu, Jul 29, 2021 at 04:58:04PM +0100, James Clark wrote:
quoted
Only show the warning if the user hasn't already set timeless mode and
improve the text because there was ambiguity around the meaning of '...'
Change the warning to a UI warning instead of printing straight to
stderr because this corrupts the UI when perf report TUI is used. The UI
warning function also handles printing to stderr when in perf script
mode.
Suggested-by: Leo Yan <redacted>
Signed-off-by: James Clark <redacted>
@@ -324,8 +324,11 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,*underflow.*/packet_queue->cs_timestamp=0;-WARN_ONCE(true,"Zero Coresight timestamp found at Idx:%"OCSD_TRC_IDX_STR-". Decoding may be improved with --itrace=Z...\n",indx);+if(!cs_etm__etmq_is_timeless(etmq))+pr_warning_once("Zero Coresight timestamp found at Idx:%"OCSD_TRC_IDX_STR+". Decoding may be improved by prepending 'Z' to your current --itrace arguments.\n",+indx);+}elseif(packet_queue->instr_count>elem->timestamp){/**Sanitycheckthattheelem->timestamp-packet_queue->instr_countwouldnot
On Tue, Aug 03, 2021 at 10:24:09AM -0300, Arnaldo Carvalho de Melo wrote:
Em Mon, Aug 02, 2021 at 11:41:45PM +0800, Leo Yan escreveu:
quoted
On Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark wrote:
quoted
Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.
Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.
Signed-off-by: James Clark <redacted>
---
tools/perf/util/cs-etm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,len=dso__data_read_offset(al.map->dso,machine,offset,buffer,size);-if(len<=0)+if(len<=0){+ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");+if(!al.map->dso->auxtrace_warned){+pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",+address,+al.map->dso->long_name?al.map->dso->long_name:"Unknown");+al.map->dso->auxtrace_warned=true;+}
This is very useful.
Just one comment: in particularly if the perf fails to find the kernel
symbols, the user needs to enable config "CONFIG_PROC_KCORE=y" or
specify option "-k /path/to/vmlinux". In this case, using 'perf
archive' is not helpful. So I think the UI warning can be imporved
like:
ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n"
" Enable CONFIG_PROC_KCORE or use option '-k /path/to/vmlinux' for kernel symbols\n");
one can also use debuginfod-client, which, as time passes, probably will
be the main way of finding DSOs now that we have build-ids in
PERF_RECORD_MMAP2 and debuginfod servers such as:
export DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/
If so, maybe should use more general description for missing DSO.
With this improvement, the patch looks good to me:
Reviewed-by: Leo Yan <redacted>
Does this apply to the other 5 patches in this series?
I finished to reivew patches 01, 04, 05, 06/06, so my review tag can
apply on these patches. Current patch 06/06 needs James to improve for
the comments.
Thanks,
Leo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: James Clark <hidden> Date: 2021-08-05 12:59:55
On 03/08/2021 15:01, Leo Yan wrote:
quoted
one can also use debuginfod-client, which, as time passes, probably will
be the main way of finding DSOs now that we have build-ids in
PERF_RECORD_MMAP2 and debuginfod servers such as:
export DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/
If so, maybe should use more general description for missing DSO.
With this improvement, the patch looks good to me:
Reviewed-by: Leo Yan <redacted>
Does this apply to the other 5 patches in this series?
I finished to reivew patches 01, 04, 05, 06/06, so my review tag can
apply on these patches. Current patch 06/06 needs James to improve for
the comments.
Thanks for the reviews, I've submitted v2.
I also had a play around with debuginfod, it looks promising.
Especially if it's integrated with perf report, which I assume is the plan?
Thanks
James
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Arnaldo Carvalho de Melo <acme@kernel.org> Date: 2021-08-06 18:36:42
Em Thu, Aug 05, 2021 at 01:59:49PM +0100, James Clark escreveu:
On 03/08/2021 15:01, Leo Yan wrote:
quoted
quoted
one can also use debuginfod-client, which, as time passes, probably will
be the main way of finding DSOs now that we have build-ids in
PERF_RECORD_MMAP2 and debuginfod servers such as:
export DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/
If so, maybe should use more general description for missing DSO.
With this improvement, the patch looks good to me:
Reviewed-by: Leo Yan <redacted>
Does this apply to the other 5 patches in this series?
I finished to reivew patches 01, 04, 05, 06/06, so my review tag can
apply on these patches. Current patch 06/06 needs James to improve for
the comments.
Thanks for the reviews, I've submitted v2.
I also had a play around with debuginfod, it looks promising.
Especially if it's integrated with perf report, which I assume is the plan?
Yeah, code is needed for that in the underlying symbols library so that
we can tune its usage, i.e. the threshold of samples to trigger asking
for the auto-download of debuginfo files.
- Arnaldo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel