Re: [RFC] x86,perf: Implement minimal P4 PMU driver v14

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Wed, Mar 10, 2010 at 08:29:28PM +0100, Robert Richter wrote:
> On 10.03.10 21:31:02, Cyrill Gorcunov wrote:
> >  arch/x86/include/asm/perf_event.h      |    2 
> >  arch/x86/include/asm/perf_p4.h         |  707 +++++++++++++++++++++++++++++++++
> 
> If so, it should be perf_event_p4.h.
> 

Accepted, thanks!

> >  arch/x86/kernel/cpu/perf_event.c       |   46 +-
> >  arch/x86/kernel/cpu/perf_event_amd.c   |    2 
> >  arch/x86/kernel/cpu/perf_event_intel.c |   15 
> >  arch/x86/kernel/cpu/perf_event_p4.c    |  612 ++++++++++++++++++++++++++++
> >  arch/x86/kernel/cpu/perf_event_p6.c    |    2 
> >  7 files changed, 1363 insertions(+), 23 deletions(-)
> 
> > Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
> > =====================================================================
> > --- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event.c
> > +++ linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
> > @@ -190,6 +190,8 @@ struct x86_pmu {
> >  	void		(*enable_all)(void);
> >  	void		(*enable)(struct perf_event *);
> >  	void		(*disable)(struct perf_event *);
> > +	int		(*hw_config)(struct perf_event_attr *attr, struct hw_perf_event *hwc);
> > +	int		(*schedule_events)(struct cpu_hw_events *cpuc, int n, int *assign);
> 
> I don't like this extension since it widened the interface without
> additional use.
> 
> (*hw_config) could be instead implemented in (*event_map).

Well, I fear I don't see how exactly. event_map has the event number
without any-kind of attributes, or you mean to extend event_map up that
way to pass attribs there as well?

> (*schedule_events) could be implemented by a special p4 handler for
> (*enable) in struct pmu. Maybe there are other solutions for both
> cases, but it should be possible by adoption of existing functions.
> 

Assignment scheme is completely different from those which are in
use for architectural events.

> The current implementation of model specific functions is
> sufficient. We have already the following:
> 
> * event initialization: x86_pmu.raw_event(), x86_pmu.event_map()
> * event enable:   event->pmu->enable(), x86_pmu.enable()
> * event disable:  event->pmu->disable(), x86_pmu.disable()
> 
> Maybe I miss something in the list above. The introduction of more
> function pointers should be reduced to a minimum.
> 
> If the pmu differs heavily you even could return a different pmu for
> such an event.
> 

This would require much more code and will lead to a code duplication
as well.

> -Robert
> 

All in one, Robert, I would like to make this code less intrusive into
the former perf sources. But at moment I don't see an easy way for this.

Which means -- I would like to collect comments/complains and so on
to improve it.

> >  	unsigned	eventsel;
> >  	unsigned	perfctr;
> >  	u64		(*event_map)(int);
> > @@ -415,6 +417,25 @@ set_ext_hw_attr(struct hw_perf_event *hw
> >  	return 0;
> >  }
> 
> -- 
> Advanced Micro Devices, Inc.
> Operating System Research Center
> email: robert.richter@amd.com
> 
	-- Cyrill