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

[Robert Richter <robert.richter@amd.com>]

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.

>  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).
(*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.

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.

-Robert

>  	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