Re: [PATCH 01/10] perf/core, x86: Add PERF_SAMPLE_LBR_TOS

[Liang, Kan]

On 10/8/2019 4:31 AM, Peter Zijlstra wrote:
On Mon, Oct 07, 2019 at 10:59:01AM -0700, kan.li...@linux.intel.com wrote:
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 61448c19a132..ee9ef0c4cb08 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -100,6 +100,7 @@ struct perf_raw_record {
   */
  struct perf_branch_stack {
        __u64                           nr;
+       __u64                           tos;
        struct perf_branch_entry        entries[0];
  };
  
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index bb7b271397a6..fe36ebb7dc2e 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -141,8 +141,9 @@ enum perf_event_sample_format {
        PERF_SAMPLE_TRANSACTION                 = 1U << 17,
        PERF_SAMPLE_REGS_INTR                   = 1U << 18,
        PERF_SAMPLE_PHYS_ADDR                   = 1U << 19,
+       PERF_SAMPLE_LBR_TOS                     = 1U << 20,
  
-	PERF_SAMPLE_MAX = 1U << 20,		/* non-ABI */
+       PERF_SAMPLE_MAX = 1U << 21,               /* non-ABI */
  
  	__PERF_SAMPLE_CALLCHAIN_EARLY		= 1ULL << 63, /* non-ABI; internal use */
  };
@@ -864,6 +865,7 @@ enum perf_event_type {
         *      { u64                   abi; # enum perf_sample_regs_abi
         *        u64                   regs[weight(mask)]; } && 
PERF_SAMPLE_REGS_INTR
         *      { u64                   phys_addr;} && PERF_SAMPLE_PHYS_ADDR
+        *      { u64                   tos;} && PERF_SAMPLE_LBR_TOS
         * };
         */
        PERF_RECORD_SAMPLE                      = 9,

I have problems with the API.. You're introducing the intel specific LBR
naming, and adding a whole new sample type vs extending the existing
BRANCH_STACK (like you really already do with struct perf_branch_stack). >
So why not add a bit to PERF_SAMPLE_BRANCH_* to request the presence of
the TOS field in the PERF_SAMPLE_BRANCH_STACK output?

We never store PERF_SAMPLE_BRANCH_* in a sample. The perf tool cannot 
tell if the sample includes TOS field.
There will be a problem when a new perf tool parsing the data generated 
by an old kernel.


Can we rename the new sample type PERF_SAMPLE_BRANCH_STACK_EXTENSION?

{ u64                   version;
  u64                   tos;}           && PERF_SAMPLE_LBR_TOS

If other platforms want to add their extension, we just need to increase 
the version number. Perf tool will check the version before parsing the 
sample.

Thanks,
Kan