lkml.org 
[lkml]   [2020]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v4 3/5] perf cs-etm: Correct synthesizing instruction samples
    Reviewed by: Mike Leach <mike.leach@linaro.org>

    On Thu, 13 Feb 2020 at 09:43, Leo Yan <leo.yan@linaro.org> wrote:
    >
    > When 'etm->instructions_sample_period' is less than
    > 'tidq->period_instructions', the function cs_etm__sample() cannot handle
    > this case properly with its logic.
    >
    > Let's see below flow as an example:
    >
    > - If we set itrace option '--itrace=i4', then function cs_etm__sample()
    > has variables with initialized values:
    >
    > tidq->period_instructions = 0
    > etm->instructions_sample_period = 4
    >
    > - When the first packet is coming:
    >
    > packet->instr_count = 10; the number of instructions executed in this
    > packet is 10, thus update period_instructions as below:
    >
    > tidq->period_instructions = 0 + 10 = 10
    > instrs_over = 10 - 4 = 6
    > offset = 10 - 6 - 1 = 3
    > tidq->period_instructions = instrs_over = 6
    >
    > - When the second packet is coming:
    >
    > packet->instr_count = 10; in the second pass, assume 10 instructions
    > in the trace sample again:
    >
    > tidq->period_instructions = 6 + 10 = 16
    > instrs_over = 16 - 4 = 12
    > offset = 10 - 12 - 1 = -3 -> the negative value
    > tidq->period_instructions = instrs_over = 12
    >
    > So after handle these two packets, there have below issues:
    >
    > The first issue is that cs_etm__instr_addr() returns the address within
    > the current trace sample of the instruction related to offset, so the
    > offset is supposed to be always unsigned value. But in fact, function
    > cs_etm__sample() might calculate a negative offset value (in handling
    > the second packet, the offset is -3) and pass to cs_etm__instr_addr()
    > with u64 type with a big positive integer.
    >
    > The second issue is it only synthesizes 2 samples for sample period = 4.
    > In theory, every packet has 10 instructions so the two packets have
    > total 20 instructions, 20 instructions should generate 5 samples
    > (4 x 5 = 20). This is because cs_etm__sample() only calls once
    > cs_etm__synth_instruction_sample() to generate instruction sample per
    > range packet.
    >
    > This patch fixes the logic in function cs_etm__sample(); the basic
    > idea for handling coming packet is:
    >
    > - To synthesize the first instruction sample, it combines the left
    > instructions from the previous packet and the head of the new
    > packet; then generate continuous samples with sample period;
    > - At the tail of the new packet, if it has the rest instructions,
    > these instructions will be left for the sequential sample.
    >
    > Suggested-by: Mike Leach <mike.leach@linaro.org>
    > Signed-off-by: Leo Yan <leo.yan@linaro.org>
    > ---
    > tools/perf/util/cs-etm.c | 87 ++++++++++++++++++++++++++++++++--------
    > 1 file changed, 70 insertions(+), 17 deletions(-)
    >
    > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
    > index b2f31390126a..4b7d6c36ce3c 100644
    > --- a/tools/perf/util/cs-etm.c
    > +++ b/tools/perf/util/cs-etm.c
    > @@ -1356,9 +1356,12 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
    > struct cs_etm_auxtrace *etm = etmq->etm;
    > int ret;
    > u8 trace_chan_id = tidq->trace_chan_id;
    > - u64 instrs_executed = tidq->packet->instr_count;
    > + u64 instrs_prev;
    >
    > - tidq->period_instructions += instrs_executed;
    > + /* Get instructions remainder from previous packet */
    > + instrs_prev = tidq->period_instructions;
    > +
    > + tidq->period_instructions += tidq->packet->instr_count;
    >
    > /*
    > * Record a branch when the last instruction in
    > @@ -1376,26 +1379,76 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
    > * TODO: allow period to be defined in cycles and clock time
    > */
    >
    > - /* Get number of instructions executed after the sample point */
    > - u64 instrs_over = tidq->period_instructions -
    > - etm->instructions_sample_period;
    > + /*
    > + * Below diagram demonstrates the instruction samples
    > + * generation flows:
    > + *
    > + * Instrs Instrs Instrs Instrs
    > + * Sample(n) Sample(n+1) Sample(n+2) Sample(n+3)
    > + * | | | |
    > + * V V V V
    > + * --------------------------------------------------
    > + * ^ ^
    > + * | |
    > + * Period Period
    > + * instructions(Pi) instructions(Pi')
    > + *
    > + * | |
    > + * \---------------- -----------------/
    > + * V
    > + * tidq->packet->instr_count
    > + *
    > + * Instrs Sample(n...) are the synthesised samples occurring
    > + * every etm->instructions_sample_period instructions - as
    > + * defined on the perf command line. Sample(n) is being the
    > + * last sample before the current etm packet, n+1 to n+3
    > + * samples are generated from the current etm packet.
    > + *
    > + * tidq->packet->instr_count represents the number of
    > + * instructions in the current etm packet.
    > + *
    > + * Period instructions (Pi) contains the the number of
    > + * instructions executed after the sample point(n) from the
    > + * previous etm packet. This will always be less than
    > + * etm->instructions_sample_period.
    > + *
    > + * When generate new samples, it combines with two parts
    > + * instructions, one is the tail of the old packet and another
    > + * is the head of the new coming packet, to generate
    > + * sample(n+1); sample(n+2) and sample(n+3) consume the
    > + * instructions with sample period. After sample(n+3), the rest
    > + * instructions will be used by later packet and it is assigned
    > + * to tidq->period_instructions for next round calculation.
    > + */
    >
    > /*
    > - * Calculate the address of the sampled instruction (-1 as
    > - * sample is reported as though instruction has just been
    > - * executed, but PC has not advanced to next instruction)
    > + * Get the initial offset into the current packet instructions;
    > + * entry conditions ensure that instrs_prev is less than
    > + * etm->instructions_sample_period.
    > */
    > - u64 offset = (instrs_executed - instrs_over - 1);
    > - u64 addr = cs_etm__instr_addr(etmq, trace_chan_id,
    > - tidq->packet, offset);
    > + u64 offset = etm->instructions_sample_period - instrs_prev;
    > + u64 addr;
    >
    > - ret = cs_etm__synth_instruction_sample(
    > - etmq, tidq, addr, etm->instructions_sample_period);
    > - if (ret)
    > - return ret;
    > + while (tidq->period_instructions >=
    > + etm->instructions_sample_period) {
    > + /*
    > + * Calculate the address of the sampled instruction (-1
    > + * as sample is reported as though instruction has just
    > + * been executed, but PC has not advanced to next
    > + * instruction)
    > + */
    > + addr = cs_etm__instr_addr(etmq, trace_chan_id,
    > + tidq->packet, offset - 1);
    > + ret = cs_etm__synth_instruction_sample(
    > + etmq, tidq, addr,
    > + etm->instructions_sample_period);
    > + if (ret)
    > + return ret;
    >
    > - /* Carry remaining instructions into next sample period */
    > - tidq->period_instructions = instrs_over;
    > + offset += etm->instructions_sample_period;
    > + tidq->period_instructions -=
    > + etm->instructions_sample_period;
    > + }
    > }
    >
    > if (etm->sample_branches) {
    > --
    > 2.17.1
    >


    --
    Mike Leach
    Principal Engineer, ARM Ltd.
    Manchester Design Centre. UK

    \
     
     \ /
      Last update: 2020-02-13 14:38    [W:9.759 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site