Re: [PATCH v4 08/22] perf: Add AUX record

[Peter Zijlstra]

On Wed, Aug 20, 2014 at 03:36:05PM +0300, Alexander Shishkin wrote:
> When there's new data in the AUX space, output a record indicating its
> offset and size and weather it was truncated to fix in the ring buffer.

This patch is too late; it should have been before the patch adding
perf_aux_output_*().

I also added acme to cc, he might have wants/needs for the data format I
suppose.

> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> ---
>  include/uapi/linux/perf_event.h | 16 ++++++++++++++++
>  kernel/events/core.c            | 39 +++++++++++++++++++++++++++++++++++++++
>  kernel/events/internal.h        |  3 +++
>  kernel/events/ring_buffer.c     |  1 +
>  4 files changed, 59 insertions(+)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 7e0967c0f5..c022c3d756 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -733,6 +733,22 @@ enum perf_event_type {
>  	 */
>  	PERF_RECORD_MMAP2			= 10,
>  
> +	/*
> +	 * Records that new data landed in the AUX buffer part.
> +	 *
> +	 * struct {
> +	 * 	struct perf_event_header	header;
> +	 *
> +	 * 	u64				aux_offset;
> +	 * 	u64				aux_size;
> +	 *	u8				truncated;
> +	 *	u8				reserved[7];

Creative.. do we want a u64 flags instead? Is there any chance at all
we're going to fill out these other bits?

> +	 *	u64				id;
> +	 *	u64				stream_id;

You probably should have included a struct sample_id there instead.

> +	 * };
> +	 */
> +	PERF_RECORD_AUX				= 11,
> +
>  	PERF_RECORD_MAX,			/* non-ABI */
>  };
>  
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9fc9a7583b..0251983018 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5542,6 +5542,45 @@ void perf_event_mmap(struct vm_area_struct *vma)
>  	perf_event_mmap_event(&mmap_event);
>  }
>  
> +void perf_event_aux_event(struct perf_event *event, unsigned long head,
> +			  unsigned long size, bool truncated)
> +{
> +	struct perf_output_handle handle;
> +	struct perf_sample_data sample;
> +	struct perf_aux_event {
> +		struct perf_event_header	header;
> +		u64				offset;
> +		u64				size;
> +		u8				truncated;
> +		u8				reserved[7];
> +		u64				id;
> +		u64				stream_id;
> +	} rec = {
> +		.header = {
> +			.type = PERF_RECORD_AUX,
> +			.misc = 0,
> +			.size = sizeof(rec),
> +		},
> +		.offset		= head,
> +		.size		= size,
> +		.truncated	= truncated,
> +		.id		= primary_event_id(event),
> +		.stream_id	= event->id,
> +	};
> +	int ret;
> +
> +	perf_event_header__init_id(&rec.header, &sample, event);

Oh hey, you do actually do the struct sample_id here, so why then also
include the id/stream_id again?

> +	ret = perf_output_begin(&handle, event, rec.header.size);
> +
> +	if (ret)
> +		return;
> +
> +	perf_output_put(&handle, rec);
> +	perf_event__output_id_sample(event, &handle, &sample);
> +
> +	perf_output_end(&handle);
> +}

Attachment: pgpfTaHT405RX.pgp
Description: PGP signature