Re: [PATCH 1/3] perf-events: Add support for supplementary event registers

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Thu, 2010-11-11 at 17:15 +0100, Andi Kleen wrote:

> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index ed63101..97133ec 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -93,6 +93,8 @@ struct amd_nb {
>  	struct event_constraint event_constraints[X86_PMC_IDX_MAX];
>  };
>  
> +struct intel_percore;
> +
>  #define MAX_LBR_ENTRIES		16
>  
>  struct cpu_hw_events {
> @@ -126,6 +128,8 @@ struct cpu_hw_events {
>  	void				*lbr_context;
>  	struct perf_branch_stack	lbr_stack;
>  	struct perf_branch_entry	lbr_entries[MAX_LBR_ENTRIES];

There wants to be a comment here, this is definitely not LBR stuff.

> +	int 				percore_used;
> +	struct intel_percore		*per_core;

Either per_core != NULL implies percore_used or it should be state
inside the struct.

>  
>  	/*
>  	 * AMD specific bits
> @@ -175,6 +179,24 @@ struct cpu_hw_events {
>  #define for_each_event_constraint(e, c)	\
>  	for ((e) = (c); (e)->weight; (e)++)
>  
> +/*
> + * Extra registers for specific events.
> + * Some events need large masks and require external MSRs.
> + * Define a mapping to these extra registers.
> + */
> +
> +struct extra_reg {
> +	unsigned event;
> +	unsigned msr;
> +	u64 config_mask;
> +	u64 valid_mask;
> +};

s/unsigned/unsigned int/ (also later in the patch) and then align the
variable names.

> +
> +#define EVENT_EXTRA_REG(event, msr, m, vm) { event, msr, m, vm }

C99 initializers please

> +#define INTEL_EVENT_EXTRA_REG(event, msr, vm) \
> +	EVENT_EXTRA_REG(event, msr, ARCH_PERFMON_EVENTSEL_EVENT, vm)
> +#define EVENT_EXTRA_END {}

Does that imply a zero filled struct?

> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index c8f5c08..bbe7fba 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1,5 +1,14 @@
>  #ifdef CONFIG_CPU_SUP_INTEL
>  
> +struct intel_percore {
> +	raw_spinlock_t lock;
> +	int ref;
> +	u64 config;
> +	unsigned extra_reg;
> +	u64 extra_config;
> +};
> +static DEFINE_PER_CPU(struct intel_percore, intel_percore);

Please dynamically allocate these when needed, just like the AMD
north-bridge structure.

>  /*
>   * Intel PerfMon, used on Core and later.
>   */

> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 057bf22..a353594 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -224,7 +224,10 @@ struct perf_event_attr {
>  	};
>  
>  	__u32			bp_type;
> -	__u64			bp_addr;
> +	union {
> +		__u64			bp_addr;
> +		__u64		        event_extra; /* Extra for some events */
> +	};
>  	__u64			bp_len;
>  };

I think I like Stephane's suggestion better, frob them into the existing
u64 word, since its model specific and we still have 33 empty bits in
the control register there's plenty space.