Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 30 May 2018 23:39:00 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Mike Leach <mike.leach@linaro.org>
Cc:     Robert Walker <robert.walker@arm.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Kim Phillips <kim.phillips@arm.co>,
        Tor Jeremiassen <tor@ti.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, coresight@lists.linaro.org
Subject: Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed
 packets
Message-ID: <20180530153900.GA10925@leoy-ThinkPad-X240s>
References: <1527497103-3593-1-git-send-email-leo.yan@linaro.org>
 <1527497103-3593-2-git-send-email-leo.yan@linaro.org>
 <20180528221347.GA4109@xps15>
 <5efe804f-364b-6ae1-3fca-ec7f6d8a383d@arm.com>
 <CAJ9a7VhC7EHJ1DtTEH1uERGiBpCJgseboaH0cqgpsU3ZMcgjgQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJ9a7VhC7EHJ1DtTEH1uERGiBpCJgseboaH0cqgpsU3ZMcgjgQ@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mike,

On Wed, May 30, 2018 at 04:04:34PM +0100, Mike Leach wrote:

[...]

> >>> +               /* 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.
> >>
> 
> clarification on the exception packets....
> 
> The Opencsd output exception packet gives you the exception number,
> and optionally the preferred return address. If this address is
> present does depend a lot on the underlying protocol - will normally
> be there with ETMv4.
> Exceptions are marked differently in the underlying protocol - the
> OCSD packets abstract away these differences.
> 
> consider the code:
> 
> 0x1000: <some instructions>
> 0x1100: BR 0x2000
> ....
> 0x2000: <some instructions>
> 0x2020  BZ r4
> 
> Without an exception this would result in the packets
> 
> OCSD_RANGE(0x1000,0x1104, Last instr type=Br, taken)   // recall that
> range packets have start addr inclusive, end addr exclusive.
> OCSD_RANGE(0x2000,0x2024, Last instr type=Br, <taken / not taken -
> depends on condition>
> 
> Now consider an exception occurring before the BR 0x2000
> 
> this will result in:-
> OCSD_RANGE(0x1000, 0x1100, Last instr type=Other)
> OCSD_EXECEPTION(IRQ, ret-addr 0x1100)
> OCSD_RANGE(IRQ_START, IRQ_END+4, Last instr type = BR, taken)    //
> this is more likely to have multiple ranges / branches before any
> return, but simplified here.
> OCSD_EXCEPTION_RETURN()  // present if exception returns are
> explicitly marked in underlying trace - may not always be depending on
> circumstances.
> OCSD_RANGE(0x1100,0x1104, Last=BR, taken) // continue on with short
> range - just the branch
> OCSD_RANGE(0x2000,0x2024, Last instr type=Br, <taken / not taken -
> depends on condition>
> 
> Now consider the exception occurring after the BR, but before any
> other instructions are executed.
> 
> OCSD_RANGE(0x1000,0x1104, Last instr type=Br, taken)   // recall that
> range packets have start addr inclusive, end addr exclusive.
> OCSD_EXECEPTION(IRQ, ret-addr 0x2000)     // here the preferred return
> address is actually the target of the branch.
> OCSD_RANGE(IRQ_START, IRQ_END+4, Last instr type = BR, taken)    //
> this is more likely to have multiple ranges / branches before any
> return, but simplified here.
> OCSD_RANGE(0x2000,0x2024, Last instr type=Br, <taken / not taken -
> depends on condition>
> 
> So in general it is possible to arrive in the IRQ_START range with the
> previous packet having been either a taken branch, a not taken branch,
> or not a branch.
> Care must be taken - whether AutoFDO or normal trace disassembly not
> to assume that having the last range packet as a taken branch means
> that the next range packet is the target, if there is an intervening
> exception packet.

Thanks a lot for detailed explaination.

IIUC, AutoFDO will not have such issue due every range packet will be
handled for it.  On the other hand, as you remind, the branch samples
(and its consumer trace disassembler) is very dependent on the flag
'last_instr_taken_branch'.

According to your explaination, I think we consider the branch is
taken for below situations:

- The new coming packet is exception packet (both for exception entry
  and exit packets);
- The previous packet is expcetion packet;
- The previous packet is normal range packet with
  'last_instr_taken_branch' = true;

So I'd like to use below function to demonstrate my understanding for
exception packets handling.  I also will send out one new patch for
support exception packet for reviewing.

If you have concern or I miss anything, please let me know.

static bool cs_etm__is_taken_branch(struct cs_etm_packet *prev_packet,
                                    struct cs_etm_packet *packet,)
{
        /* The branch is taken for normal range packet with taken branch flag */
        if (prev_packet->sample_type == CS_ETM_RANGE &&
            prev_packet->last_instr_taken_branch)
                return true;

        /* The branch is taken if previous packet is exception packet */
        if (prev_packet->sample_type == CS_ETM_EXCEPTION ||
            prev_packet->sample_type == CS_ETM_EXCEPTION_RET)
                return true;

        /* The branch is taken for an intervening exception packet */
        if (packet->sample_type == CS_ETM_EXCEPTION ||
            packet->sample_type == CS_ETM_EXCEPTION_RET)
                return true;

        return false;
}

[...]

Thanks,
Leo Yan