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

["Liang, Kan" <kan.liang@linux.intel.com>]

On 11/19/2019 5:51 PM, Stephane Eranian wrote:
> On Tue, Nov 19, 2019 at 2:25 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 11/19/2019 2:02 PM, Stephane Eranian wrote:
>>> On Tue, Nov 19, 2019 at 6:35 AM<kan.liang@linux.intel.com>  wrote:
>>>> From: Kan Liang<kan.liang@linux.intel.com>
>>>>
>>>> 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@linux.intel.com>
>>>> ---
>>>>    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.
>

I plan to use hw_idx to replace tos, which indicates the low level index 
of raw branch records for the most recent branch aka entries[0].
For other architectures whose raw branch records are already stored in 
age order. The hw_idx should be 0.
If we don't know the order of raw branch records, the hw_idx should be 
-1ULL. I will set hw_idx to -1ULL for IBM Power for now. They can change 
it later if needed.

How about the changes as below?

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 011dcbdbccc2..e2de81372433 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -93,14 +93,26 @@ struct perf_raw_record {
  /*
   * branch stack layout:
   *  nr: number of taken branches stored in entries[]
+ *  hw_idx: The low level index of raw branch records
+ *          for the most recent branch.
+ *          -1ULL means invalid.
   *
   * Note that nr can vary from sample to sample
   * branches (to, from) are stored from most recent
   * to least recent, i.e., entries[0] contains the most
   * recent branch.
+ * The entries[] is an abstraction of raw branch records,
+ * which may not be stored in age order in HW, e.g. Intel LBR.
+ * The hw_idx is to expose the low level index of raw
+ * branch record for the most recent branch aka entries[0].
+ * For the architectures whose raw branch records are
+ * already stored in age order, the hw_idx should be 0.
+ * If we don't know the order of raw branch records,
+ * the hw_idx should be -1ULL.
   */
  struct perf_branch_stack {
         __u64                           nr;
+       __u64                           hw_idx;
         struct perf_branch_entry        entries[0];
  };
diff --git a/include/uapi/linux/perf_event.h 
b/include/uapi/linux/perf_event.h
index bb7b271397a6..30f335f0d25e 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -180,7 +180,9 @@ enum perf_branch_sample_type_shift {

         PERF_SAMPLE_BRANCH_TYPE_SAVE_SHIFT      = 16, /* save branch 
type */

-       PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
+       PERF_SAMPLE_BRANCH_HW_INDEX_SHIFT       = 17, /* save low level 
index of raw branch records */
+
+       PERF_SAMPLE_BRANCH_MAX_SHIFT            = 18, /* non-ABI */
  };

  enum perf_branch_sample_type {
@@ -207,6 +209,8 @@ enum perf_branch_sample_type {
         PERF_SAMPLE_BRANCH_TYPE_SAVE    =
                 1U << PERF_SAMPLE_BRANCH_TYPE_SAVE_SHIFT,

+       PERF_SAMPLE_BRANCH_HW_INDEX     = 1U << 
PERF_SAMPLE_BRANCH_HW_INDEX_SHIFT,
+
         PERF_SAMPLE_BRANCH_MAX          = 1U << 
PERF_SAMPLE_BRANCH_MAX_SHIFT,
  };

@@ -849,7 +853,11 @@ enum perf_event_type {
          *        char                  data[size];}&& PERF_SAMPLE_RAW
          *
          *      { u64                   nr;
-        *        { u64 from, to, flags } lbr[nr];} && 
PERF_SAMPLE_BRANCH_STACK
+        *        { u64 from, to, flags } lbr[nr];
+        *
+        *        # only available if PERF_SAMPLE_BRANCH_HW_INDEX is set
+        *        u64                   hw_idx;
+        *      } && PERF_SAMPLE_BRANCH_STACK
          *
          *      { u64                   abi; # enum perf_sample_regs_abi
          *        u64                   regs[weight(mask)]; } && 
PERF_SAMPLE_REGS_USER


Thanks,
Kan