Re: [PATCH V4 01/13] perf/core: Add new branch sample type for LBR TOS

[Stephane Eranian]

On Tue, Nov 19, 2019 at 2:25 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 11/19/2019 2:02 PM, Stephane Eranian wrote:
> > On Tue, Nov 19, 2019 at 6:35 AM<kan.liang@xxxxxxxxxxxxxxx>  wrote:
> >> From: Kan Liang<kan.liang@xxxxxxxxxxxxxxx>
> >>
> >> In LBR call stack mode, the depth of reconstructed LBR call stack limits
> >> to the number of LBR registers. With LBR Top-of-Stack (TOS) information,
> >> perf tool may stitch the stacks of two samples. The reconstructed LBR
> >> call stack can break the HW limitation.
> >>
> >> Add a new branch sample type to retrieve LBR TOS. The new type is PMU
> >> specific. Add it at the end of enum perf_branch_sample_type.
> >> Add a macro to retrieve defined bits of branch sample type.
> >> Update perf_copy_attr() to handle the new bit.
> >>
> >> Only when the new branch sample type is set, the TOS information is
> >> dumped into the PERF_SAMPLE_BRANCH_STACK output.
> >> Perf tool should check the attr.branch_sample_type, and apply the
> >> corresponding format for PERF_SAMPLE_BRANCH_STACK samples.
> >> Otherwise, some user case may be broken. For example, users may parse a
> >> perf.data, which include the new branch sample type, with an old version
> >> perf tool (without the check). Users probably get incorrect information
> >> without any warning.
> >>
> >> Signed-off-by: Kan Liang<kan.liang@xxxxxxxxxxxxxxx>
> >> ---
> >>   include/linux/perf_event.h      |  2 ++
> >>   include/uapi/linux/perf_event.h | 16 ++++++++++++++--
> >>   kernel/events/core.c            | 13 ++++++++++++-
> >>   3 files changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> >> index 011dcbdbccc2..761021c7ee8a 100644
> >> --- a/include/linux/perf_event.h
> >> +++ b/include/linux/perf_event.h
> >> @@ -93,6 +93,7 @@ struct perf_raw_record {
> >>   /*
> >>    * branch stack layout:
> >>    *  nr: number of taken branches stored in entries[]
> >> + *  tos: Top-of-Stack (TOS) information. PMU specific data.
> >>    *
> >>    * Note that nr can vary from sample to sample
> >>    * branches (to, from) are stored from most recent
> >> @@ -101,6 +102,7 @@ struct perf_raw_record {
> >>    */
> >>   struct perf_branch_stack {
> >>          __u64                           nr;
> >> +       __u64                           tos; /* PMU specific data */
> >>          struct perf_branch_entry        entries[0];
> >>   };
> >>
> > Same remark as with the other patch. You need to abstract this.
> > The TOS and PMU specific data should be limited to  x86/event/intel/*.[ch].
> >
>
> If we change tos to a generic name, e.g. pmu_specific_data, can we still
> keep it here?
>
It's not just about the name, it is about what it points to?
What value does it return when the hw does not have a TOS?
I added the PERF_SAMPLE_BRANCH_*. I did not just expose the
raw LBR. There is an abstraction layer, so it is easier to map to other
architectures, like IBM Power, for instance. You cannot just add a TOS
and say it is PMU specific. If you do that for all architectures, then it
becomes very messy and hard to understand and use especially for tools.

This is an interface you are trying to define. This needs to be specified
precisely so that tools can make the right assumptions across hw platforms.

Note that the entries[] array is normally already sorted by most
recent to least recent.
So exporting a TOS there is bizarre. The TOS is likely always pointing to the
most recent entry. The TOS you want is exposing a low level index which does not
map to the abstracted branch stack. And that's a problem.  You need to reconcile
your definition of TOS with the branch_sample_entry [] abstraction.

> If not, I think the only way is to introduce a new method, e.g.
> output_br_pmu_data(), at struct pmu.
> When outputting the sample data, the generic code will call
> event->pmu->output_br_pmu_data() to retrieve the TOS in Intel code.
> I think it's too complicated.
>
> Thanks,
> Kan
>
>
>
>