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:   Mon, 10 Feb 2020 13:46:53 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Mike Leach <mike.leach@linaro.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        linux-kernel@vger.kernel.org,
        Robert Walker <robert.walker@arm.com>,
        Coresight ML <coresight@lists.linaro.org>
Subject: Re: [PATCH v4 1/5] perf cs-etm: Refactor instruction size handling
Message-ID: <20200210054653.GB10753@leoy-ThinkPad-X240s>
References: <20200203020716.31832-1-leo.yan@linaro.org>
 <20200203020716.31832-2-leo.yan@linaro.org>
 <CAJ9a7Vi37DAZ0q78MahmMQqSxD68Rphaw0t5cMsX3gDk8PA3DA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJ9a7Vi37DAZ0q78MahmMQqSxD68Rphaw0t5cMsX3gDk8PA3DA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mike,

On Thu, Feb 06, 2020 at 12:36:43PM +0000, Mike Leach wrote:
> Hi Leo,
> 
> On Mon, 3 Feb 2020 at 02:07, Leo Yan <leo.yan@linaro.org> wrote:
> >
> > cs-etm.c has several functions which need to know instruction size
> > based on address, e.g. cs_etm__instr_addr() and cs_etm__copy_insn()
> > two functions both calculate the instruction size separately with its
> > duplicated code.  Furthermore, adding new features later which might
> > require to calculate instruction size as well.
> >
> > For this reason, this patch refactors the code to introduce a new
> > function cs_etm__instr_size(), this function is central place to
> > calculate the instruction size based on ISA type and instruction
> > address.
> >
> > For a neat implementation, cs_etm__instr_addr() will always execute the
> > loop without checking ISA type, this allows cs_etm__instr_size() and
> > cs_etm__instr_addr() have no any duplicate code with each other and both
> > functions are independent and can be changed separately without breaking
> > anything.  As a side effect, cs_etm__instr_addr() will do a few more
> > iterations for A32/A64 instructions, this would be fine if consider perf
> > is a tool running in the user space.
> >
> 
> I prefer to take the optimisation win where I can - I always do in the
> trace decoder when counting instructions over a range.
> Consider that you can be processing MB of trace data, and most likely
> that will be A64/A32 on a lot of the current and future platforms.
> 
> Therefore I would keep the useful cs_etm__instr_size() function, but
> also keep a single ISA check in cs_etm__instr_addr() to do
> the (addr + offset * 4) calculation for non T32.

Understand.  Will refine the code by following this suggestion.

Thanks,
Leo Yan