Re: [PATCH V2 1/5] perf/core: Add PERF_SAMPLE_WEIGHT_STRUCT

[Liang, Kan]

On 1/27/2021 2:03 PM, Peter Zijlstra wrote:
> On Wed, Jan 27, 2021 at 07:38:41AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:
> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > index b15e344..13b4019 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -145,12 +145,14 @@ enum perf_event_sample_format {
> >   	PERF_SAMPLE_CGROUP			= 1U << 21,
> >   	PERF_SAMPLE_DATA_PAGE_SIZE		= 1U << 22,
> >   	PERF_SAMPLE_CODE_PAGE_SIZE		= 1U << 23,
> > +	PERF_SAMPLE_WEIGHT_STRUCT		= 1U << 24,
> >   
> > -	PERF_SAMPLE_MAX = 1U << 24,		/* non-ABI */
> > +	PERF_SAMPLE_MAX = 1U << 25,		/* non-ABI */
> >   
> >   	__PERF_SAMPLE_CALLCHAIN_EARLY		= 1ULL << 63, /* non-ABI; internal use */
> >   };
> >   
> > +#define PERF_SAMPLE_WEIGHT_TYPE	(PERF_SAMPLE_WEIGHT | PERF_SAMPLE_WEIGHT_STRUCT)
> >   /*
> >    * values to program into branch_sample_type when PERF_SAMPLE_BRANCH is set
> >    *
> > @@ -890,7 +892,16 @@ enum perf_event_type {
> >   	 * 	  char			data[size];
> >   	 * 	  u64			dyn_size; } && PERF_SAMPLE_STACK_USER
> >   	 *
> > -	 *	{ u64			weight;   } && PERF_SAMPLE_WEIGHT
> > +	 *	{ union perf_sample_weight
> > +	 *	 {
> > +	 *		u64		full; && PERF_SAMPLE_WEIGHT
> > +	 *		struct {
> > +	 *			u32	low_dword;
> > +	 *			u16	high_word;
> > +	 *			u16	higher_word;
> > +	 *		} && PERF_SAMPLE_WEIGHT_STRUCT
> > +	 *	 }
> > +	 *	}
> >   	 *	{ u64			data_src; } && PERF_SAMPLE_DATA_SRC
> >   	 *	{ u64			transaction; } && PERF_SAMPLE_TRANSACTION
> >   	 *	{ u64			abi; # enum perf_sample_regs_abi
> > @@ -1248,4 +1259,13 @@ struct perf_branch_entry {
> >   		reserved:40;
> >   };
> >   
> > +union perf_sample_weight {
> > +	__u64		full;
> > +	struct {
> > +		__u32	low_dword;
> > +		__u16	high_word;
> > +		__u16	higher_word;
> > +	};
> > +};
>
> *urgh*, my naming lives ... anybody got a better suggestion?

I think we need a generic name here, but the problem is that the 
'weight' field has different meanings among architectures.

The 'weight' fields are to store all kinds of latency on X86.
On PowerPC, it stores MMCRA[TECX/TECM], which doesn't look like a latency.

I don't think I can use the name, 'cache_lat' or 'instruction_lat', 
here. Right?
If so, how about 'var'?

u32 var_1_dw;
u16 var_2_w;
u16 var_3_w;


> Also, do we want to care about byte order?

Sure. I will add the big-endian and little-endian support.


Thanks,
Kan