lkml.org 
[lkml]   [2010]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] x86,perf: Implement minimal P4 PMU driver v14
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


\
 
 \ /
  Last update: 2010-03-10 20:45    [W:2.223 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site