Re: [PATCH v1 1/3] perf cs-etm: Refactor instruction size handling

[Leo Yan]

Hi Mathieu,

On Wed, Sep 04, 2019 at 11:06:10AM -0600, Mathieu Poirier wrote:
> On Wed, 4 Sep 2019 at 03:19, Leo Yan <leo.yan@xxxxxxxxxx> wrote:
> >
> > Hi Mathieu,
> >
> > On Tue, Sep 03, 2019 at 04:22:15PM -0600, Mathieu Poirier wrote:
> > > On Fri, Aug 30, 2019 at 02:24:19PM +0800, Leo Yan wrote:
> > > > There has several code pieces need to know the instruction size, but
> > > > now every place calculates the instruction size separately.
> > > >
> > > > This patch refactors to create a new function cs_etm__instr_size() as
> > > > a central place to analyze the instruction length based on ISA type
> > > > and instruction value.
> > > >
> > > > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > > > ---
> > > >  tools/perf/util/cs-etm.c | 44 +++++++++++++++++++++++++++-------------
> > > >  1 file changed, 30 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > > > index b3a5daaf1a8f..882a0718033d 100644
> > > > --- a/tools/perf/util/cs-etm.c
> > > > +++ b/tools/perf/util/cs-etm.c
> > > > @@ -914,6 +914,26 @@ static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
> > > >     return ((instrBytes[1] & 0xF8) >= 0xE8) ? 4 : 2;
> > > >  }
> > > >
> > > > +static inline int cs_etm__instr_size(struct cs_etm_queue *etmq,
> > > > +                                u8 trace_chan_id,
> > > > +                                enum cs_etm_isa isa,
> > > > +                                u64 addr)
> > > > +{
> > > > +   int insn_len;
> > > > +
> > > > +   /*
> > > > +    * T32 instruction size might be 32-bit or 16-bit, decide by calling
> > > > +    * cs_etm__t32_instr_size().
> > > > +    */
> > > > +   if (isa == CS_ETM_ISA_T32)
> > > > +           insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id, addr);
> > > > +   /* Otherwise, A64 and A32 instruction size are always 32-bit. */
> > > > +   else
> > > > +           insn_len = 4;
> > > > +
> > > > +   return insn_len;
> > > > +}
> > > > +
> > > >  static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)
> > > >  {
> > > >     /* Returns 0 for the CS_ETM_DISCONTINUITY packet */
> > > > @@ -938,19 +958,23 @@ static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,
> > > >                                  const struct cs_etm_packet *packet,
> > > >                                  u64 offset)
> > > >  {
> > > > +   int insn_len;
> > > > +
> > > >     if (packet->isa == CS_ETM_ISA_T32) {
> > > >             u64 addr = packet->start_addr;
> > > >
> > > >             while (offset > 0) {
> > > > -                   addr += cs_etm__t32_instr_size(etmq,
> > > > -                                                  trace_chan_id, addr);
> > > > +                   addr += cs_etm__instr_size(etmq, trace_chan_id,
> > > > +                                              packet->isa, addr);
> > > >                     offset--;
> > > >             }
> > > >             return addr;
> > > >     }
> > > >
> > > > -   /* Assume a 4 byte instruction size (A32/A64) */
> > > > -   return packet->start_addr + offset * 4;
> > > > +   /* Return instruction size for A32/A64 */
> > > > +   insn_len = cs_etm__instr_size(etmq, trace_chan_id,
> > > > +                                 packet->isa, packet->start_addr);
> > > > +   return packet->start_addr + offset * insn_len;
> > >
> > > This patch will work but from where I stand it makes things difficult to
> > > understand more than anything else.  It is also adding coupling between function
> > > cs_etm__instr_addr() and cs_etm__instr_size(), meaning the code needs to be
> > > carefully inspected in order to make changes to either one.
> >
> > My purpose is to use a same place to calculate the instruction
> > size, rather than to spread the duplicate codes in several different
> > functions.
> >
> > > Last but not least function cs_etm__instr_size() isn't used in the upcoming
> > > patches.  I really don't see what is gained here.
> >
> > Sorry that I forgot to commit my final change into patch 02.
> >
> > I planed to use cs_etm__instr_size() in patch 02; patch 02 has
> > function cs_etm__add_stack_event(), which also needs to get the
> > instruction size when it sends stack event.
> >
> > After apply patch 02, tools/perf/util/cs-etm.c will have below three
> > functions to caculate instruction size; this is the main reason I want
> > to refactor the code for instruction size.
> >
> >   cs_etm__instr_addr()
> >   cs_etm__copy_insn()
> >   cs_etm__add_stack_event()
> >
> > If this lets code more difficult to understand, will drop it.
> >
> 
> I agree with the consolidation but for that to work function
> cs_etm__instr_addr() needs to be refactored.  Since
> cs_etm__instr_size() is already taking care of checking the ISA type
> the while() loop in cs_etm__instr_addr() can be done regardless of the
> operation mode.  That way cs_etm__instr_size() can be changed at will
> without breaking anything.
> 
> The downside is that we are doing a few more iterations... Which isn't
> that big a deal considering we are in user space.  We can think about
> some optimisation if it is ever proven to be a bottleneck.
> 
> Let me know if you see a problem with that approach.

Yes, your approach is neat; I will try it in next patch version.

Thanks a lot for suggestion!

[...]