Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter

From: Vince Weaver <vincent.weaver@maine.edu>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Vince Weaver <vincent.weaver@maine.edu>,
	Andi Kleen <ak@linux.intel.com>,
	Stephane Eranian <eranian@google.com>,
	"Liang, Kan" <kan.liang@intel.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"alexander.shishkin@linux.intel.com" 
	<alexander.shishkin@linux.intel.com>,
	"acme@redhat.com" <acme@redhat.com>,
	"jolsa@redhat.com" <jolsa@redhat.com>,
	"torvalds@linux-foundation.org" <torvalds@linux-foundation.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>
Subject: Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter
Date: Wed, 31 May 2017 16:57:19 -0400 (EDT)	[thread overview]
Message-ID: <alpine.DEB.2.20.1705311654110.30556@macbook-air> (raw)
In-Reply-To: <20170530172555.5ya3ilfw3sowokjz@hirez.programming.kicks-ass.net>

On Tue, 30 May 2017, Peter Zijlstra wrote:

> On Wed, May 24, 2017 at 12:01:50PM -0400, Vince Weaver wrote:
> > I already have people really grumpy that you have to have one mmap() page 
> > per event, meaning you sacrifice one TLB entry for each event you are 
> > measuring.
> 
> So there is space in that page. We could maybe look at having an array
> of stuff (max 32 entries?) which would cover a whole event group.
> 
> Then you only need to mmap() the page for the leading event.

Yes, I think that was the interface they were hoping for.
I've been meaning to run some tests and see if there is a noticible hit 
with TLB misses but haven't had the chance.

> Something like the below for the uapi changes I suppose. I've not
> tried to actually implement it yet, but would something like that be
> usable?

it looks usable from a quick glance.  Often the problems only turn up once 
you try to implement it.

Vince

> 
> 
> ---
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index b1c0b187acfe..40ff77e52b9d 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -345,7 +345,8 @@ struct perf_event_attr {
>  				context_switch :  1, /* context switch data */
>  				write_backward :  1, /* Write ring buffer from end to beginning */
>  				namespaces     :  1, /* include namespaces data */
> -				__reserved_1   : 35;
> +				group_pmc      :  1, /* use group_pmc in perf_event_mmap_page */
> +				__reserved_1   : 34;
>  
>  	union {
>  		__u32		wakeup_events;	  /* wakeup every n events */
> @@ -469,7 +470,8 @@ struct perf_event_mmap_page {
>  				cap_user_rdpmc		: 1, /* The RDPMC instruction can be used to read counts */
>  				cap_user_time		: 1, /* The time_* fields are used */
>  				cap_user_time_zero	: 1, /* The time_zero field is used */
> -				cap_____res		: 59;
> +				cap_group_pmc		: 1, /* The group_pmc field is used, ignore @index and @offset */
> +				cap_____res		: 58;
>  		};
>  	};
>  
> @@ -530,11 +532,29 @@ struct perf_event_mmap_page {
>  	__u64	time_zero;
>  	__u32	size;			/* Header size up to __reserved[] fields. */
>  
> +	__u32	__reserved_4byte_hole;
> +
> +	/*
> +	 * If cap_group_pmc this array replaces @index and @offset. The array
> +	 * will contain an entry for each group member lead by the event belonging
> +	 * to this mmap().
> +	 *
> +	 * The @id field can be used to identify which sibling event the respective
> +	 * @index and @offset values belong to. Assuming an immutable group, the
> +	 * array index will stay constant for each event.
> +	 */
> +	struct {
> +		__u32	index;
> +		__u32	__reserved_hole;
> +		__s64	offset;
> +		__u64	id;		/* event id */
> +	} group_pmc[32];
> +
>  		/*
>  		 * Hole for extension of the self monitor capabilities
>  		 */
>  
> -	__u8	__reserved[118*8+4];	/* align to 1k. */
> +	__u8	__reserved[22*8];	/* align to 1k. */
>  
>  	/*
>  	 * Control data for the mmap() data buffer.
> 