Re: [PATCH 2/6] perf/x86: Fix data source decoding for Skylake

[Peter Zijlstra]

On Mon, Jun 05, 2017 at 03:48:34PM -0700, Andi Kleen wrote:

> +void __init intel_pmu_pebs_data_source_skl(void)
> +{
> +	pebs_data_source[0x08] = OP_LH | P(LVLX, L4) | P(SNOOP, HIT);
> +	pebs_data_source[0x09] = OP_LH | P(LVLX, L4) | P(LVLX, REMOTE) | P(SNOOP, HIT);
> +	pebs_data_source[0x0b] = OP_LH | P(LVLX, RAM) | P(LVLX, REMOTE) | P(SNOOP, NONE);
> +	pebs_data_source[0x0c] = OP_LH | P(LVL, NA) | P(LVLX, REMOTE) | P(SNOOPX, FWD);
> +	pebs_data_source[0x0d] = OP_LH | P(LVL, NA) | P(LVLX, REMOTE) | P(SNOOP, HITM);
> +}

Not too happy about that..

  P(LVLX, L4) | P(LVLX, REMOTE)

reads like something that should be PERF_MEM_LVL_REM_CCE1 or something
and

  P(LVLX, RAM) | P(LVLX, REMOTE)

Should certainly be

  PERF_MEM_LVL_REM_RAM1


This new generic 'REMOTE' has too much overlap with the existing things.

I realize the pickle you're in, but urgh..

Stephane?


> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index b1c0b187acfe..4b5deeada34b 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -931,14 +931,18 @@ union perf_mem_data_src {
>  			mem_snoop:5,	/* snoop mode */
>  			mem_lock:2,	/* lock instr */
>  			mem_dtlb:7,	/* tlb access */
> -			mem_rsvd:31;
> +			mem_lvlx:8,	/* memory hierarchy level, ext */
> +			mem_snoopx:2,	/* snoop mode, ext */
> +			mem_rsvd:21;
>  	};
>  };
>  #elif defined(__BIG_ENDIAN_BITFIELD)
>  union perf_mem_data_src {
>  	__u64 val;
>  	struct {
> -		__u64	mem_rsvd:31,
> +		__u64	mem_rsvd:21,
> +			mem_snoopx:2,	/* snoop mode, ext */
> +			mem_lvlx:8,	/* memory hierarchy level, ext */
>  			mem_dtlb:7,	/* tlb access */
>  			mem_lock:2,	/* lock instr */
>  			mem_snoop:5,	/* snoop mode */
> @@ -975,6 +979,13 @@ union perf_mem_data_src {
>  #define PERF_MEM_LVL_UNC	0x2000 /* Uncached memory */
>  #define PERF_MEM_LVL_SHIFT	5
>  
> +#define PERF_MEM_LVLX_REMOTE    0x01 /* Remote */
> +#define PERF_MEM_LVLX_L4	0x02 /* L4 */
> +#define PERF_MEM_LVLX_RAM	0x04 /* Ram */
> +/* 5 free */
> +
> +#define PERF_MEM_LVLX_SHIFT	33
> +
>  /* snoop mode */
>  #define PERF_MEM_SNOOP_NA	0x01 /* not available */
>  #define PERF_MEM_SNOOP_NONE	0x02 /* no snoop */
> @@ -983,6 +994,10 @@ union perf_mem_data_src {
>  #define PERF_MEM_SNOOP_HITM	0x10 /* snoop hit modified */
>  #define PERF_MEM_SNOOP_SHIFT	19
>  
> +#define PERF_MEM_SNOOPX_FWD	0x01 /* forward */
> +/* 1 free */
> +#define PERF_MEM_SNOOPX_SHIFT	41
> +
>  /* locked instruction */
>  #define PERF_MEM_LOCK_NA	0x01 /* not available */
>  #define PERF_MEM_LOCK_LOCKED	0x02 /* locked transaction */
> -- 
> 2.9.4
>