Re: [PATCH 2/4] perf cs-etm: Use previous thread for branch sample source IP

On Wed, May 24, 2023 at 02:19:56PM +0100, James Clark wrote:

Branch samples currently use the IP of the previous packet as the from IP, and the IP of the current packet as the to IP. But it incorrectly uses the current thread. In some cases like a jump into a different exception level this will attribute to the incorrect process.

It's about the timing that branch has taken or not taken :)

If we think the branch sample as 'branch has taken', then current code is doning right thing, otherwise, we need this fix.

Fix it by tracking the previous thread in the same way the previous packet is tracked.

Signed-off-by: James Clark james.clark@arm.com

tools/perf/util/cs-etm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index ebffc9052561..a997fe79d458 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -86,6 +86,7 @@ struct cs_etm_traceid_queue { size_t last_branch_pos; union perf_event *event_buf; struct thread *thread;

• struct thread *prev_thread; struct branch_stack *last_branch; struct branch_stack *last_branch_rb; struct cs_etm_packet *prev_packet;

@@ -480,6 +481,7 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq, tidq->trace_chan_id = trace_chan_id; tidq->thread = machine__findnew_thread(&etm->session->machines.host, -1, queue->tid);

• tidq->prev_thread = machine__idle_thread(&etm->session->machines.host);

tidq->packet = zalloc(sizeof(struct cs_etm_packet)); if (!tidq->packet) @@ -616,6 +618,8 @@ static void cs_etm__packet_swap(struct cs_etm_auxtrace *etm, tmp = tidq->packet; tidq->packet = tidq->prev_packet; tidq->prev_packet = tmp;

• thread__put(tidq->prev_thread);
  

• tidq->prev_thread = thread__get(tidq->thread);
  

Maybe cs_etm__packet_swap() is not the best place to update "tidq->prev_thread", since swapping packet doesn't mean it's necessarily thread switching; can we move this change into the cs_etm__set_thread()?

Thanks, Leo

} } @@ -791,6 +795,7 @@ static void cs_etm__free_traceid_queues(struct cs_etm_queue *etmq) /* Free this traceid_queue from the array */ tidq = etmq->traceid_queues[idx]; thread__zput(tidq->thread);

• thread__zput(tidq->prev_thread);
  

  zfree(&tidq->event_buf); zfree(&tidq->last_branch); zfree(&tidq->last_branch_rb);

@@ -1450,8 +1455,8 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq, sample.time = cs_etm__resolve_sample_time(etmq, tidq); sample.ip = ip;

• sample.pid = tidq->thread->pid_;
• sample.tid = tidq->thread->tid;

• sample.pid = tidq->prev_thread->pid_;
• sample.tid = tidq->prev_thread->tid; sample.addr = cs_etm__first_executed_instr(tidq->packet); sample.id = etmq->etm->branches_id; sample.stream_id = etmq->etm->branches_id;

-- 2.34.1