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:   Tue, 29 Oct 2019 12:11:59 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Suzuki K Poulose <suzuki.poulose@arm.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 Mailing List <linux-kernel@vger.kernel.org>,
        Mike Leach <mike.leach@linaro.org>,
        Coresight ML <coresight@lists.linaro.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH v3 3/6] perf cs-etm: Support thread stack
Message-ID: <20191029041159.GA25758@leoy-ThinkPad-X240s>
References: <20191005091614.11635-1-leo.yan@linaro.org>
 <20191005091614.11635-4-leo.yan@linaro.org>
 <20191011175353.GA13688@xps15>
 <20191022050304.GB32731@leoy-ThinkPad-X240s>
 <CANLsYkwx1Z2eFz4JqKe9UB8tFqpSdx-kakMHnL1rkUttLZeX1w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANLsYkwx1Z2eFz4JqKe9UB8tFqpSdx-kakMHnL1rkUttLZeX1w@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mathieu,

On Mon, Oct 28, 2019 at 04:43:57PM -0600, Mathieu Poirier wrote:
> On Mon, 21 Oct 2019 at 23:03, Leo Yan <leo.yan@linaro.org> wrote:
> >
> > Hi Mathieu,
> >
> > On Fri, Oct 11, 2019 at 11:53:53AM -0600, Mathieu Poirier wrote:
> > > On Sat, Oct 05, 2019 at 05:16:11PM +0800, Leo Yan wrote:
> > > > Since Arm CoreSight doesn't support thread stack, the decoding cannot
> > > > display symbols with indented spaces to reflect the stack depth.
> > > >
> > > > This patch adds support thread stack for Arm CoreSight, this allows
> > > > 'perf script' to display properly for option '-F,+callindent'.
> > > >
> > > > Before:
> > > >
> > > >   # perf script -F,+callindent
> > > >             main  2808          1          branches: coresight_test1                      ffff8634f5c8 coresight_test1+0x3c (/root/coresight_test/libcstest.so)
> > > >             main  2808          1          branches: printf@plt                           aaaaba8d37ec main+0x28 (/root/coresight_test/main)
> > > >             main  2808          1          branches: printf@plt                           aaaaba8d36bc printf@plt+0xc (/root/coresight_test/main)
> > > >             main  2808          1          branches: _init                                aaaaba8d3650 _init+0x30 (/root/coresight_test/main)
> > > >             main  2808          1          branches: _dl_fixup                            ffff86373b4c _dl_runtime_resolve+0x40 (/lib/aarch64-linux-gnu/ld-2.28.so)
> > > >             main  2808          1          branches: _dl_lookup_symbol_x                  ffff8636e078 _dl_fixup+0xb8 (/lib/aarch64-linux-gnu/ld-2.28.so)
> > > >   [...]
> > > >
> > > > After:
> > > >
> > > >   # perf script -F,+callindent
> > > >             main  2808          1          branches:                 coresight_test1                                      ffff8634f5c8 coresight_test1+0x3c (/root/coresight_test/libcstest.so)
> > > >             main  2808          1          branches:                 printf@plt                                           aaaaba8d37ec main+0x28 (/root/coresight_test/main)
> > > >             main  2808          1          branches:                     printf@plt                                       aaaaba8d36bc printf@plt+0xc (/root/coresight_test/main)
> > > >             main  2808          1          branches:                     _init                                            aaaaba8d3650 _init+0x30 (/root/coresight_test/main)
> > > >             main  2808          1          branches:                     _dl_fixup                                        ffff86373b4c _dl_runtime_resolve+0x40 (/lib/aarch64-linux-gnu/ld-2.28.s
> > > >             main  2808          1          branches:                         _dl_lookup_symbol_x                          ffff8636e078 _dl_fixup+0xb8 (/lib/aarch64-linux-gnu/ld-2.28.so)
> > > >   [...]
> > > >
> > > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > > ---
> > > >  tools/perf/util/cs-etm.c | 44 ++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 44 insertions(+)
> > > >
> > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > > > index 58ceba7b91d5..780abbfd1833 100644
> > > > --- a/tools/perf/util/cs-etm.c
> > > > +++ b/tools/perf/util/cs-etm.c
> > > > @@ -1117,6 +1117,45 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
> > > >                        sample->insn_len, (void *)sample->insn);
> > > >  }
> > > >
> > > > +static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
> > > > +                               struct cs_etm_traceid_queue *tidq)
> > > > +{
> > > > +   struct cs_etm_auxtrace *etm = etmq->etm;
> > > > +   u8 trace_chan_id = tidq->trace_chan_id;
> > > > +   int insn_len;
> > > > +   u64 from_ip, to_ip;
> > > > +
> > > > +   if (etm->synth_opts.thread_stack) {
> > > > +           from_ip = cs_etm__last_executed_instr(tidq->prev_packet);
> > > > +           to_ip = cs_etm__first_executed_instr(tidq->packet);
> > > > +
> > > > +           insn_len = cs_etm__instr_size(etmq, trace_chan_id,
> > > > +                                         tidq->prev_packet->isa, from_ip);
> > > > +
> > > > +           /*
> > > > +            * Create thread stacks by keeping track of calls and returns;
> > > > +            * any call pushes thread stack, return pops the stack, and
> > > > +            * flush stack when the trace is discontinuous.
> > > > +            */
> > > > +           thread_stack__event(tidq->thread, tidq->prev_packet->cpu,
> > > > +                               tidq->prev_packet->flags,
> > > > +                               from_ip, to_ip, insn_len,
> > > > +                               etmq->buffer->buffer_nr);
> > >
> > > Details are a little fuzzy in my head but I'm pretty sure
> > > we want trace_chan_id here.
> >
> > I spent some time to look into this question, and I think we don't
> > need to add extra info for trace_chan_id.
> >
> > The main reason is for CPU wide tracing, if one task is migrated from
> > CPU_a to CPU_b, if we append 'trace_chan_id' for the buffer number, then
> > it will tell the thread_stack that the buffer has been changed (or it
> > will be considered the trace is discontinuous), then thread stack will
> > be flushed.  Actually, this is not what we want; if a task is migrated
> > from one CPU to another, we still need to keep its thread stack if the
> > trace data comes from the same buffer_nr.
> 
> After reviewing the code I conclude that using etmq->buffer->buffer_nr
> is the correct way to proceed.

Thanks for reviewing and confirmation.

> That being said you have sent this new set [1], which is a rework of
> some of the code you have in the current set.  As such the only way
> forward is for you to wait until [1] I has been applied and rebase the
> remaining work in this set on top of it.

Right.

Seems the shared link is incorrect :)  Let's firstly focus on the patch
set: 'perf cs-etm: Fix synthesizing instruction samples' [2] and after
it is merged I will send new patch set for cs-etm callchain support as
soon as possible.

Thanks,
Leo Yan

[2] https://patchwork.kernel.org/cover/11209991/

> Let me know if you have questions.
> 
> Thanks,
> Mathieu
> 
> [1]. https://patchwork.kernel.org/cover/11130213/
> 
> >
> > To be honest, I struggled to understand what's the purpose for
> > 'buffer->buffer_nr', from the code, I think 'buffer->buffer_nr' is
> > mainly used to trace the splitted buffers (e.g. the buffers are splitted
> > into different queues so the trace data coming from different trace
> > chunk?).  Now I observe 'buffer->buffer_nr' is always zero since the
> > buffer is not used with splitted mode.  If later we support 1:1 map
> > between tracers and sinks, then we need to set 'buffer->buffer_nr' so
> > can reflect the correct buffer mapping, but we don't need to use
> > trace_chan_id as extra info at here.
> >
> > Please let me know what you think about this?  If you agree with this,
> > I will send out patch v4 soon with addressing other comments.
> >
> > Thanks,
> > Leo Yan
> >
> > > > +   } else {
> > > > +           /*
> > > > +            * The thread stack can be output via thread_stack__process();
> > > > +            * thus the detailed information about paired calls and returns
> > > > +            * will be facilitated by Python script for the db-export.
> > > > +            *
> > > > +            * Need to set trace buffer number and flush thread stack if the
> > > > +            * trace buffer number has been alternate.
> > > > +            */
> > > > +           thread_stack__set_trace_nr(tidq->thread,
> > > > +                                      tidq->prev_packet->cpu,
> > > > +                                      etmq->buffer->buffer_nr);
> > >
> > > Same here.
> > >
> > > > +   }
> > > > +}
> > > > +
> > > >  static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
> > > >                                         struct cs_etm_traceid_queue *tidq,
> > > >                                         u64 addr, u64 period)
> > > > @@ -1393,6 +1432,9 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
> > > >             tidq->period_instructions = instrs_over;
> > > >     }
> > > >
> > > > +   if (tidq->prev_packet->last_instr_taken_branch)
> > > > +           cs_etm__add_stack_event(etmq, tidq);
> > > > +
> > > >     if (etm->sample_branches) {
> > > >             bool generate_sample = false;
> > > >
> > > > @@ -2593,6 +2635,8 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
> > > >             itrace_synth_opts__set_default(&etm->synth_opts,
> > > >                             session->itrace_synth_opts->default_no_sample);
> > > >             etm->synth_opts.callchain = false;
> > > > +           etm->synth_opts.thread_stack =
> > > > +                           session->itrace_synth_opts->thread_stack;
> > > >     }
> > > >
> > > >     err = cs_etm__synth_events(etm, session);
> > > > --
> > > > 2.17.1
> > > >