Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed packets

From: Leo Yan
Date: Mon May 28 2018 - 20:26:06 EST

Next message: Jungsub Shin: "Re: Why is the length of max mount option a page size??"
Previous message: Michael Kelley (EOSG): "RE: [PATCH] PCI: hv: Do not wait forever on a device that has disappeared"
In reply to: Mathieu Poirier: "Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed packets"
Next in thread: Mathieu Poirier: "Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed packets"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Mathieu,

On Mon, May 28, 2018 at 04:13:47PM -0600, Mathieu Poirier wrote:
> Leo and/or Robert,
>
> On Mon, May 28, 2018 at 04:45:00PM +0800, Leo Yan wrote:
> > Commit e573e978fb12 ("perf cs-etm: Inject capabilitity for CoreSight
> > traces") reworks the samples generation flow from CoreSight trace to
> > match the correct format so Perf report tool can display the samples
> > properly.
> >
> > But the change has side effect for branch packet handling, it only
> > generate branch samples by checking previous packet flag
> > 'last_instr_taken_branch' is true, this results in below three kinds
> > packets are missed to generate branch samples:
> >
> > - The start tracing packet at the beginning of tracing data;
> > - The exception handling packet;
> > - If one CS_ETM_TRACE_ON packet is inserted, we also miss to handle it
> > for branch samples. CS_ETM_TRACE_ON packet itself can give the info
> > that there have a discontinuity in the trace, on the other hand we
> > also miss to generate proper branch sample for packets before and
> > after CS_ETM_TRACE_ON packet.
> >
> > This patch is to add branch sample handling for up three kinds packets:
> >
> > - In function cs_etm__sample(), check if 'prev_packet->sample_type' is
> > zero and in this case it generates branch sample for the start tracing
> > packet; furthermore, we also need to handle the condition for
> > prev_packet::end_addr is zero in the cs_etm__last_executed_instr();
> >
> > - In function cs_etm__sample(), check if 'prev_packet->exc' is true and
> > generate branch sample for exception handling packet;
> >
> > - If there has one CS_ETM_TRACE_ON packet is coming, we firstly generate
> > branch sample in the function cs_etm__flush(), this can save complete
> > info for the previous CS_ETM_RANGE packet just before CS_ETM_TRACE_ON
> > packet. We also generate branch sample for the new CS_ETM_RANGE
> > packet after CS_ETM_TRACE_ON packet, this have two purposes, the
> > first one purpose is to save the info for the new CS_ETM_RANGE packet,
> > the second purpose is to save CS_ETM_TRACE_ON packet info so we can
> > have hint for a discontinuity in the trace.
> >
> > For CS_ETM_TRACE_ON packet, its fields 'packet->start_addr' and
> > 'packet->end_addr' equal to 0xdeadbeefdeadbeefUL which are emitted in
> > the decoder layer as dummy value. This patch is to convert these
> > values to zeros for more readable; this is accomplished by functions
> > cs_etm__last_executed_instr() and cs_etm__first_executed_instr(). The
> > later one is a new function introduced by this patch.
> >
> > Reviewed-by: Robert Walker <robert.walker@xxxxxxx>
> > Fixes: e573e978fb12 ("perf cs-etm: Inject capabilitity for CoreSight traces")
> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > ---
> > tools/perf/util/cs-etm.c | 93 +++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 73 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index 822ba91..8418173 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -495,6 +495,20 @@ static inline void cs_etm__reset_last_branch_rb(struct cs_etm_queue *etmq)
> > static inline u64 cs_etm__last_executed_instr(struct cs_etm_packet *packet)
> > {
> > /*
> > + * The packet is the start tracing packet if the end_addr is zero,
> > + * returns 0 for this case.
> > + */
> > + if (!packet->end_addr)
> > + return 0;
>
> What is considered to be the "start tracing packet"? Right now the only two
> kind of packets inserted in the decoder packet buffer queue are INST_RANGE and
> TRACE_ON. How can we hit a condition where packet->end-addr == 0?

When the first CS_ETM_RANGE packet is coming, etmq->prev_packet is
initialized by the function cs_etm__alloc_queue(), so
etmq->prev_packet->end_addr is zero:

etmq->prev_packet = zalloc(szp);

As you mentioned, we should only have two kind of packets for
CS_ETM_RANGE and CS_ETM_TRACE_ON. Currently we skip to handle the
first CS_ETM_TRACE_ON packet in function cs_etm__flush(), we also can
refine the function cs_etm__flush() to handle the first coming
CS_ETM_TRACE_ON packet, after that all packets will be CS_ETM_RANGE
and CS_ETM_TRACE_ON and have no chance to hit 'packet->end_addr = 0'.

Does this make sense for you?

--- Packet dumping when first packet coming ---
cs_etm__flush: prev_packet: sample_type=0 exc=0 exc_ret=0 cpu=0 start_addr=0x0 end_addr=0x0 last_instr_taken_branch=0
cs_etm__flush: packet: sample_type=2 exc=0 exc_ret=0 cpu=1 start_addr=0xdeadbeefdeadbeef end_addr=0xdeadbeefdeadbeef last_instr_taken_branch=0

> > +
> > + /*
> > + * The packet is the CS_ETM_TRACE_ON packet if the end_addr is
> > + * magic number 0xdeadbeefdeadbeefUL, returns 0 for this case.
> > + */
> > + if (packet->end_addr == 0xdeadbeefdeadbeefUL)
> > + return 0;
>
> As it is with the above, I find triggering on addresses to be brittle and hard
> to maintain on the long run. Packets all have a sample_type field that should
> be used in cases like this one. That way we know exactly the condition that is
> targeted.

Will do this.

> While working on this set, please spin-off another patch that defines
> CS_ETM_INVAL_ADDR 0xdeadbeefdeadbeefUL and replace all the cases where the
> numeral is used. That way we stop using the hard coded value.

Will do this.

As now this patch is big with more complex logic, so I consider to
split it into small patches:

- Define CS_ETM_INVAL_ADDR;
- Fix for CS_ETM_TRACE_ON packet;
- Fix for exception packet;

Does this make sense for you? I have concern that this patch is a
fixing patch, so not sure after spliting patches will introduce
trouble for applying them for other stable kernels ...

> > +
> > + /*
> > * The packet records the execution range with an exclusive end address
> > *
> > * A64 instructions are constant size, so the last executed
> > @@ -505,6 +519,18 @@ static inline u64 cs_etm__last_executed_instr(struct cs_etm_packet *packet)
> > return packet->end_addr - A64_INSTR_SIZE;
> > }
> >
> > +static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)
> > +{
> > + /*
> > + * The packet is the CS_ETM_TRACE_ON packet if the start_addr is
> > + * magic number 0xdeadbeefdeadbeefUL, returns 0 for this case.
> > + */
> > + if (packet->start_addr == 0xdeadbeefdeadbeefUL)
> > + return 0;
>
> Same comment as above.

Will do this.

> > +
> > + return packet->start_addr;
> > +}
> > +
> > static inline u64 cs_etm__instr_count(const struct cs_etm_packet *packet)
> > {
> > /*
> > @@ -546,7 +572,7 @@ static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq)
> >
> > be = &bs->entries[etmq->last_branch_pos];
> > be->from = cs_etm__last_executed_instr(etmq->prev_packet);
> > - be->to = etmq->packet->start_addr;
> > + be->to = cs_etm__first_executed_instr(etmq->packet);
> > /* No support for mispredict */
> > be->flags.mispred = 0;
> > be->flags.predicted = 1;
> > @@ -701,7 +727,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq)
> > sample.ip = cs_etm__last_executed_instr(etmq->prev_packet);
> > sample.pid = etmq->pid;
> > sample.tid = etmq->tid;
> > - sample.addr = etmq->packet->start_addr;
> > + sample.addr = cs_etm__first_executed_instr(etmq->packet);
> > sample.id = etmq->etm->branches_id;
> > sample.stream_id = etmq->etm->branches_id;
> > sample.period = 1;
> > @@ -897,13 +923,28 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
> > etmq->period_instructions = instrs_over;
> > }
> >
> > - if (etm->sample_branches &&
> > - etmq->prev_packet &&
> > - etmq->prev_packet->sample_type == CS_ETM_RANGE &&
> > - etmq->prev_packet->last_instr_taken_branch) {
> > - ret = cs_etm__synth_branch_sample(etmq);
> > - if (ret)
> > - return ret;
> > + if (etm->sample_branches && etmq->prev_packet) {
> > + bool generate_sample = false;
> > +
> > + /* Generate sample for start tracing packet */
> > + if (etmq->prev_packet->sample_type == 0 ||
>
> What kind of packet is sample_type == 0 ?

Just as explained above, sample_type == 0 is the packet which
initialized in the function cs_etm__alloc_queue().

> > + etmq->prev_packet->sample_type == CS_ETM_TRACE_ON)
> > + generate_sample = true;
> > +
> > + /* Generate sample for exception packet */
> > + if (etmq->prev_packet->exc == true)
> > + generate_sample = true;
>
> Please don't do that. Exception packets have a type of their own and can be
> added to the decoder packet queue the same way INST_RANGE and TRACE_ON packets
> are. Moreover exception packet containt an address that, if I'm reading the
> documenation properly, can be used to keep track of instructions that were
> executed between the last address of the previous range packet and the address
> executed just before the exception occurred. Mike and Rob will have to confirm
> this as the decoder may be doing all that hard work for us.

Sure, will wait for Rob and Mike to confirm for this.

At my side, I dump the packet, the exception packet isn't passed to
cs-etm.c layer, the decoder layer only sets the flag
'packet->exc = true' when exception packet is coming [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c#n364

> > +
> > + /* Generate sample for normal branch packet */
> > + if (etmq->prev_packet->sample_type == CS_ETM_RANGE &&
> > + etmq->prev_packet->last_instr_taken_branch)
> > + generate_sample = true;
> > +
> > + if (generate_sample) {
> > + ret = cs_etm__synth_branch_sample(etmq);
> > + if (ret)
> > + return ret;
> > + }
> > }
> >
> > if (etm->sample_branches || etm->synth_opts.last_branch) {
> > @@ -922,11 +963,16 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
> > static int cs_etm__flush(struct cs_etm_queue *etmq)
> > {
> > int err = 0;
> > + struct cs_etm_auxtrace *etm = etmq->etm;
> > struct cs_etm_packet *tmp;
> >
> > - if (etmq->etm->synth_opts.last_branch &&
> > - etmq->prev_packet &&
> > - etmq->prev_packet->sample_type == CS_ETM_RANGE) {
> > + if (!etmq->prev_packet)
> > + return 0;
> > +
> > + if (etmq->prev_packet->sample_type != CS_ETM_RANGE)
> > + return 0;
> > +
> > + if (etmq->etm->synth_opts.last_branch) {
>
> If you add:
>
> if (!etmq->etm->synth_opts.last_branch)
> return 0;
>
> You can avoid indenting the whole block.

No, here we cannot do like this. Except we need to handle the
condition for 'etmq->etm->synth_opts.last_branch', we also need to
handle 'etm->sample_branches'. These two conditions are saperate and
decide by different command parameters from 'perf script'.

> > /*
> > * Generate a last branch event for the branches left in the
> > * circular buffer at the end of the trace.
> > @@ -939,18 +985,25 @@ static int cs_etm__flush(struct cs_etm_queue *etmq)
> > err = cs_etm__synth_instruction_sample(
> > etmq, addr,
> > etmq->period_instructions);
> > + if (err)
> > + return err;
> > etmq->period_instructions = 0;
> > + }
> >
> > - /*
> > - * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
> > - * the next incoming packet.
> > - */
> > - tmp = etmq->packet;
> > - etmq->packet = etmq->prev_packet;
> > - etmq->prev_packet = tmp;
> > + if (etm->sample_branches) {
> > + err = cs_etm__synth_branch_sample(etmq);
> > + if (err)
> > + return err;
> > }
> >
> > - return err;
> > + /*
> > + * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
> > + * the next incoming packet.
> > + */
> > + tmp = etmq->packet;
> > + etmq->packet = etmq->prev_packet;
> > + etmq->prev_packet = tmp;
>
> Robert, I remember noticing that when you first submitted the code but forgot to
> go back to it. What is the point of swapping the packets? I understand
>
> etmq->prev_packet = etmq->packet;
>
> But not
>
> etmq->packet = tmp;
>
> After all etmq->packet will be clobbered as soon as cs_etm_decoder__get_packet()
> is called, which is alwasy right after either cs_etm__sample() or
> cs_etm__flush().

Yeah, I have the same question for this :)

Thanks for suggestions and reviewing.

> Thanks,
> Mathieu
>
>
>
> > + return 0;
> > }
> >
> > static int cs_etm__run_decoder(struct cs_etm_queue *etmq)
> > --
> > 2.7.4
> >

Next message: Jungsub Shin: "Re: Why is the length of max mount option a page size??"
Previous message: Michael Kelley (EOSG): "RE: [PATCH] PCI: hv: Do not wait forever on a device that has disappeared"
In reply to: Mathieu Poirier: "Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed packets"
Next in thread: Mathieu Poirier: "Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed packets"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]