Messages in this thread | | | Subject | Re: [PATCH 1/3] perf-events: Add support for supplementary event registers | From | Peter Zijlstra <> | Date | Thu, 11 Nov 2010 20:09:14 +0100 |
| |
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.
| |