diff mbox series

[v3,1/5] perf cs-etm: Swap packets for instruction samples

Message ID 20200203015203.27882-2-leo.yan@linaro.org (mailing list archive)
State New, archived
Headers show
Series perf cs-etm: Fix synthesizing instruction samples | expand

Commit Message

Leo Yan Feb. 3, 2020, 1:51 a.m. UTC
If use option '--itrace=iNNN' with Arm CoreSight trace data, perf tool
fails inject instruction samples; the root cause is the packets are
only switched for branch samples and last branches but not for
instruction samples, so the new coming packets cannot be properly
handled for only synthesizing instruction samples.

To fix this issue, this patch switches packets for instruction samples.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/cs-etm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Mike Leach Feb. 5, 2020, 3:59 p.m. UTC | #1
Hi Leo

On Mon, 3 Feb 2020 at 01:52, Leo Yan <leo.yan@linaro.org> wrote:
>
> If use option '--itrace=iNNN' with Arm CoreSight trace data, perf tool
> fails inject instruction samples; the root cause is the packets are
> only switched for branch samples and last branches but not for
> instruction samples, so the new coming packets cannot be properly
> handled for only synthesizing instruction samples.
>
> To fix this issue, this patch switches packets for instruction samples.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/cs-etm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 5471045ebf5c..3dd5ba34a2c2 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -1404,7 +1404,8 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
>                 }
>         }
>
> -       if (etm->sample_branches || etm->synth_opts.last_branch) {
> +       if (etm->sample_branches || etm->synth_opts.last_branch ||
> +           etm->sample_instructions) {
>                 /*
>                  * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
>                  * the next incoming packet.
> @@ -1476,7 +1477,8 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
>         }
>
>  swap_packet:
> -       if (etm->sample_branches || etm->synth_opts.last_branch) {
> +       if (etm->sample_branches || etm->synth_opts.last_branch ||
> +           etm->sample_instructions) {
>                 /*
>                  * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
>                  * the next incoming packet.
> --
> 2.17.1
>
if is worth putting the 'if <options> { swap packet }' into a separate
function as it appears twice in identical form? Might help if more
options for swap packet are needed later.

Either way

Reviewed by: Mike Leach <mike.leach@linaro.org>
Leo Yan Feb. 6, 2020, 7:43 a.m. UTC | #2
Hi Mike,

On Wed, Feb 05, 2020 at 03:59:40PM +0000, Mike Leach wrote:
> Hi Leo
> 
> On Mon, 3 Feb 2020 at 01:52, Leo Yan <leo.yan@linaro.org> wrote:
> >
> > If use option '--itrace=iNNN' with Arm CoreSight trace data, perf tool
> > fails inject instruction samples; the root cause is the packets are
> > only switched for branch samples and last branches but not for
> > instruction samples, so the new coming packets cannot be properly
> > handled for only synthesizing instruction samples.
> >
> > To fix this issue, this patch switches packets for instruction samples.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/util/cs-etm.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index 5471045ebf5c..3dd5ba34a2c2 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -1404,7 +1404,8 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
> >                 }
> >         }
> >
> > -       if (etm->sample_branches || etm->synth_opts.last_branch) {
> > +       if (etm->sample_branches || etm->synth_opts.last_branch ||
> > +           etm->sample_instructions) {
> >                 /*
> >                  * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
> >                  * the next incoming packet.
> > @@ -1476,7 +1477,8 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
> >         }
> >
> >  swap_packet:
> > -       if (etm->sample_branches || etm->synth_opts.last_branch) {
> > +       if (etm->sample_branches || etm->synth_opts.last_branch ||
> > +           etm->sample_instructions) {
> >                 /*
> >                  * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
> >                  * the next incoming packet.
> > --
> > 2.17.1
> >
> if is worth putting the 'if <options> { swap packet }' into a separate
> function as it appears twice in identical form? Might help if more
> options for swap packet are needed later.

Makes sense.  Will factor out a new function for this.

Thanks for reviewing!
Leo

> Either way
> 
> Reviewed by: Mike Leach <mike.leach@linaro.org>
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
diff mbox series

Patch

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 5471045ebf5c..3dd5ba34a2c2 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -1404,7 +1404,8 @@  static int cs_etm__sample(struct cs_etm_queue *etmq,
 		}
 	}
 
-	if (etm->sample_branches || etm->synth_opts.last_branch) {
+	if (etm->sample_branches || etm->synth_opts.last_branch ||
+	    etm->sample_instructions) {
 		/*
 		 * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
 		 * the next incoming packet.
@@ -1476,7 +1477,8 @@  static int cs_etm__flush(struct cs_etm_queue *etmq,
 	}
 
 swap_packet:
-	if (etm->sample_branches || etm->synth_opts.last_branch) {
+	if (etm->sample_branches || etm->synth_opts.last_branch ||
+	    etm->sample_instructions) {
 		/*
 		 * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
 		 * the next incoming packet.