Re: [PATCH V5 09/12] perf: make events stream always parsable

[Adrian Hunter <adrian.hunter@intel.com>]

On 16/07/13 18:09, Peter Zijlstra wrote:
> On Tue, Jul 16, 2013 at 09:49:31AM +0300, Adrian Hunter wrote:
> 
>> If you want the ID at the first position in the ID sample, it is do-able.
>> It means perf tools will have to be changed to calculate the variable start
>> position of the ID sample, and then parse the ID sample forwards from there.
>> Please advise.
> 
> Oh urgh I see.. how about I merge something like the below first?

I put in my V7 series.

> 
> Re-reading your patch; you add something to struct sample_id; which means that
> events which 'forget' to set perf_event_attr::sample_id_all while setting
> PERF_SAMPLE_IDENTIFIER are 'funny'. Should we refuse them?

No, that is OK.

> 
> 
> ---
> Subject: perf: Update perf_event_type documentation
> 
> Due to a discussion with Adrian I had a good look at the perf_event_type record
> layout and found the documentation to be somewhat unclear.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  include/uapi/linux/perf_event.h | 21 +++++++++++++++++++--
>  kernel/events/core.c            | 31 ++++++++++++++++---------------
>  2 files changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 0b1df41..335016c 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -478,6 +478,16 @@ enum perf_event_type {
>  	 * file will be supported by older perf tools, with these new optional
>  	 * fields being ignored.
>  	 *
> +	 * struct sample_id {
> +	 * 	{ u32			pid, tid; } && PERF_SAMPLE_TID
> +	 * 	{ u64			time;     } && PERF_SAMPLE_TIME
> +	 * 	{ u64			id;       } && PERF_SAMPLE_ID
> +	 * 	{ u64			stream_id;} && PERF_SAMPLE_STREAM_ID
> +	 * 	{ u32			cpu, res; } && PERF_SAMPLE_CPU
> +	 * } && perf_event_attr::sample_id_all
> +	 */
> +
> +	/*
>  	 * The MMAP events record the PROT_EXEC mappings so that we can
>  	 * correlate userspace IPs to code. They have the following structure:
>  	 *
> @@ -488,7 +498,8 @@ enum perf_event_type {
>  	 *	u64				addr;
>  	 *	u64				len;
>  	 *	u64				pgoff;
> +	 * 	struct sample_id		sample_id;
>  	 * };
>  	 */
>  	PERF_RECORD_MMAP			= 1,
> @@ -498,6 +509,7 @@ enum perf_event_type {
>  	 *	struct perf_event_header	header;
>  	 *	u64				id;
>  	 *	u64				lost;
> +	 * 	struct sample_id		sample_id;
>  	 * };
>  	 */
>  	PERF_RECORD_LOST			= 2,
> @@ -508,6 +520,7 @@ enum perf_event_type {
>  	 *
>  	 *	u32				pid, tid;
>  	 *	char				comm[];
> +	 * 	struct sample_id		sample_id;
>  	 * };
>  	 */
>  	PERF_RECORD_COMM			= 3,
> @@ -518,6 +531,7 @@ enum perf_event_type {
>  	 *	u32				pid, ppid;
>  	 *	u32				tid, ptid;
>  	 *	u64				time;
> +	 * 	struct sample_id		sample_id;
>  	 * };
>  	 */
>  	PERF_RECORD_EXIT			= 4,
> @@ -528,6 +542,7 @@ enum perf_event_type {
>  	 *	u64				time;
>  	 *	u64				id;
>  	 *	u64				stream_id;
> +	 * 	struct sample_id		sample_id;
>  	 * };
>  	 */
>  	PERF_RECORD_THROTTLE			= 5,
> @@ -539,6 +554,7 @@ enum perf_event_type {
>  	 *	u32				pid, ppid;
>  	 *	u32				tid, ptid;
>  	 *	u64				time;
> +	 * 	struct sample_id		sample_id;
>  	 * };
>  	 */
>  	PERF_RECORD_FORK			= 7,
> @@ -549,6 +565,7 @@ enum perf_event_type {
>  	 *	u32				pid, tid;
>  	 *
>  	 *	struct read_format		values;
> +	 * 	struct sample_id		sample_id;
>  	 * };
>  	 */
>  	PERF_RECORD_READ			= 8,
> @@ -596,7 +613,7 @@ enum perf_event_type {
>  	 * 	  u64			dyn_size; } && PERF_SAMPLE_STACK_USER
>  	 *
>  	 *	{ u64			weight;   } && PERF_SAMPLE_WEIGHT
> -	 *	{ u64			data_src;     } && PERF_SAMPLE_DATA_SRC
> +	 *	{ u64			data_src; } && PERF_SAMPLE_DATA_SRC
>  	 * };
>  	 */
>  	PERF_RECORD_SAMPLE			= 9,
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 708ab70..c9ef899 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4462,20 +4462,6 @@ void perf_output_sample(struct perf_output_handle *handle,
>  		}
>  	}
>  
> -	if (!event->attr.watermark) {
> -		int wakeup_events = event->attr.wakeup_events;
> -
> -		if (wakeup_events) {
> -			struct ring_buffer *rb = handle->rb;
> -			int events = local_inc_return(&rb->events);
> -
> -			if (events >= wakeup_events) {
> -				local_sub(wakeup_events, &rb->events);
> -				local_inc(&rb->wakeup);
> -			}
> -		}
> -	}
> -
>  	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
>  		if (data->br_stack) {
>  			size_t size;
> @@ -4511,16 +4497,31 @@ void perf_output_sample(struct perf_output_handle *handle,
>  		}
>  	}
>  
> -	if (sample_type & PERF_SAMPLE_STACK_USER)
> +	if (sample_type & PERF_SAMPLE_STACK_USER) {
>  		perf_output_sample_ustack(handle,
>  					  data->stack_user_size,
>  					  data->regs_user.regs);
> +	}
>  
>  	if (sample_type & PERF_SAMPLE_WEIGHT)
>  		perf_output_put(handle, data->weight);
>  
>  	if (sample_type & PERF_SAMPLE_DATA_SRC)
>  		perf_output_put(handle, data->data_src.val);
> +
> +	if (!event->attr.watermark) {
> +		int wakeup_events = event->attr.wakeup_events;
> +
> +		if (wakeup_events) {
> +			struct ring_buffer *rb = handle->rb;
> +			int events = local_inc_return(&rb->events);
> +
> +			if (events >= wakeup_events) {
> +				local_sub(wakeup_events, &rb->events);
> +				local_inc(&rb->wakeup);
> +			}
> +		}
> +	}
>  }
>  
>  void perf_prepare_sample(struct perf_event_header *header,
> 
> 
>