[RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Lin Ming]

A new field "pmu_id" is added to struct perf_event_attr.

2 new functions: perf_event_register_pmu, perf_event_lookup_pmu
perf_event_register_pmu: the pmu registration facility
perf_event_lookup_pmu: lookup the pmu via the passed in event->attr.pmu_id.

A new api pmu->init_event to replace hw_perf_event_init

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 include/linux/perf_event.h |    9 +++++++++
 kernel/perf_event.c        |   39 +++++++++++++++++++++++++++++++--------
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 5856d3b..6246c99 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -225,6 +225,8 @@ struct perf_event_attr {
 	__u32			bp_type;
 	__u64			bp_addr;
 	__u64			bp_len;
+
+	int			pmu_id;
 };
 
 /*
@@ -553,6 +555,9 @@ struct perf_event;
  * struct pmu - generic performance monitoring unit
  */
 struct pmu {
+	int id;
+	struct list_head entry;
+
 	int (*enable)			(struct perf_event *event);
 	void (*disable)			(struct perf_event *event);
 	int (*start)			(struct perf_event *event);
@@ -569,6 +574,8 @@ struct pmu {
 	void (*start_txn)	(struct pmu *pmu);
 	void (*cancel_txn)	(struct pmu *pmu);
 	int  (*commit_txn)	(struct pmu *pmu);
+
+	int  (*init_event)	(struct perf_event *event);
 };
 
 /**
@@ -1014,6 +1021,8 @@ extern int perf_swevent_get_recursion_context(void);
 extern void perf_swevent_put_recursion_context(int rctx);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
+
+extern void perf_event_register_pmu(struct pmu *pmu);
 #else
 static inline void
 perf_event_task_sched_in(struct task_struct *task)			{ }
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 36baf85..f19d40e 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -40,6 +40,12 @@
  */
 static DEFINE_PER_CPU(struct perf_cpu_context, perf_cpu_context);
 
+/*
+ * The list of multiple hw pmus
+ */
+static struct list_head pmus;
+static int pmu_id_curr;
+
 int perf_max_events __read_mostly = 1;
 static int perf_reserved_percpu __read_mostly;
 static int perf_overcommit __read_mostly = 1;
@@ -75,11 +81,6 @@ static DEFINE_SPINLOCK(perf_resource_lock);
 /*
  * Architecture provided APIs - weak aliases:
  */
-extern __weak struct pmu *hw_perf_event_init(struct perf_event *event)
-{
-	return NULL;
-}
-
 void __weak hw_perf_disable(void)		{ barrier(); }
 void __weak hw_perf_enable(void)		{ barrier(); }
 
@@ -4670,6 +4671,19 @@ static struct pmu *sw_perf_event_init(struct perf_event *event)
 	return pmu;
 }
 
+static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
+{
+	struct pmu *pmu;
+	int pmu_id = event->attr.pmu_id;
+
+	list_for_each_entry(pmu, &pmus, entry) {
+		if (pmu->id == pmu_id)
+			return pmu;
+	}
+
+	return NULL;
+}
+
 /*
  * Allocate and initialize a event structure
  */
@@ -4685,7 +4699,7 @@ perf_event_alloc(struct perf_event_attr *attr,
 	struct pmu *pmu;
 	struct perf_event *event;
 	struct hw_perf_event *hwc;
-	long err;
+	long err = 0;
 
 	event = kzalloc(sizeof(*event), gfpflags);
 	if (!event)
@@ -4750,7 +4764,9 @@ perf_event_alloc(struct perf_event_attr *attr,
 	case PERF_TYPE_RAW:
 	case PERF_TYPE_HARDWARE:
 	case PERF_TYPE_HW_CACHE:
-		pmu = hw_perf_event_init(event);
+		pmu = perf_event_lookup_pmu(event);
+		if (pmu && pmu->init_event)
+			err = pmu->init_event(event);
 		break;
 
 	case PERF_TYPE_SOFTWARE:
@@ -4770,7 +4786,6 @@ perf_event_alloc(struct perf_event_attr *attr,
 		break;
 	}
 done:
-	err = 0;
 	if (!pmu)
 		err = -EINVAL;
 	else if (IS_ERR(pmu))
@@ -5627,6 +5642,8 @@ void __init perf_event_init(void)
 	perf_cpu_notify(&perf_cpu_nb, (unsigned long)CPU_ONLINE,
 			(void *)(long)smp_processor_id());
 	register_cpu_notifier(&perf_cpu_nb);
+
+	INIT_LIST_HEAD(&pmus);
 }
 
 static ssize_t perf_show_reserve_percpu(struct sysdev_class *class,
@@ -5726,3 +5743,9 @@ static int __init perf_event_sysfs_init(void)
 				  &perfclass_attr_group);
 }
 device_initcall(perf_event_sysfs_init);
+
+void perf_event_register_pmu(struct pmu *pmu)
+{
+	pmu->id = pmu_id_curr++;
+	list_add_tail(&pmu->entry, &pmus);
+}

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Peter Zijlstra]

On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> +{
> +       struct pmu *pmu;
> +       int pmu_id = event->attr.pmu_id;
> +
> +       list_for_each_entry(pmu, &pmus, entry) {
> +               if (pmu->id == pmu_id)
> +                       return pmu;
> +       }
> +
> +       return NULL;
> +}

> +void perf_event_register_pmu(struct pmu *pmu)
> +{
> +       pmu->id = pmu_id_curr++;
> +       list_add_tail(&pmu->entry, &pmus);
> +} 

That will be wanting some sort of synchronization

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Lin Ming]

On Mon, 2010-05-10 at 17:40 +0800, Peter Zijlstra wrote:
> On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> > +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> > +{
> > +       struct pmu *pmu;
> > +       int pmu_id = event->attr.pmu_id;
> > +
> > +       list_for_each_entry(pmu, &pmus, entry) {
> > +               if (pmu->id == pmu_id)
> > +                       return pmu;
> > +       }
> > +
> > +       return NULL;
> > +}
> 
> > +void perf_event_register_pmu(struct pmu *pmu)
> > +{
> > +       pmu->id = pmu_id_curr++;
> > +       list_add_tail(&pmu->entry, &pmus);
> > +} 
> 
> That will be wanting some sort of synchronization

Will add a mutex to protect the list of pmus.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Peter Zijlstra]

On Mon, 2010-05-10 at 18:17 +0800, Lin Ming wrote:
> On Mon, 2010-05-10 at 17:40 +0800, Peter Zijlstra wrote:
> > On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> > > +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> > > +{
> > > +       struct pmu *pmu;
> > > +       int pmu_id = event->attr.pmu_id;
> > > +
> > > +       list_for_each_entry(pmu, &pmus, entry) {
> > > +               if (pmu->id == pmu_id)
> > > +                       return pmu;
> > > +       }
> > > +
> > > +       return NULL;
> > > +}
> > 
> > > +void perf_event_register_pmu(struct pmu *pmu)
> > > +{
> > > +       pmu->id = pmu_id_curr++;
> > > +       list_add_tail(&pmu->entry, &pmus);
> > > +} 
> > 
> > That will be wanting some sort of synchronization
> 
> Will add a mutex to protect the list of pmus.

I'm thinking RCU might be better suited, a mutex for lookup doesn't
sound ideal.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Lin Ming]

On Mon, 2010-05-10 at 18:19 +0800, Peter Zijlstra wrote:
> On Mon, 2010-05-10 at 18:17 +0800, Lin Ming wrote:
> > On Mon, 2010-05-10 at 17:40 +0800, Peter Zijlstra wrote:
> > > On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> > > > +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> > > > +{
> > > > +       struct pmu *pmu;
> > > > +       int pmu_id = event->attr.pmu_id;
> > > > +
> > > > +       list_for_each_entry(pmu, &pmus, entry) {
> > > > +               if (pmu->id == pmu_id)
> > > > +                       return pmu;
> > > > +       }
> > > > +
> > > > +       return NULL;
> > > > +}
> > > 
> > > > +void perf_event_register_pmu(struct pmu *pmu)
> > > > +{
> > > > +       pmu->id = pmu_id_curr++;
> > > > +       list_add_tail(&pmu->entry, &pmus);
> > > > +} 
> > > 
> > > That will be wanting some sort of synchronization
> > 
> > Will add a mutex to protect the list of pmus.
> 
> I'm thinking RCU might be better suited, a mutex for lookup doesn't
> sound ideal.

Thanks for the idea, I'll do that.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Frederic Weisbecker]

On Mon, May 10, 2010 at 12:19:29PM +0200, Peter Zijlstra wrote:
> On Mon, 2010-05-10 at 18:17 +0800, Lin Ming wrote:
> > On Mon, 2010-05-10 at 17:40 +0800, Peter Zijlstra wrote:
> > > On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> > > > +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> > > > +{
> > > > +       struct pmu *pmu;
> > > > +       int pmu_id = event->attr.pmu_id;
> > > > +
> > > > +       list_for_each_entry(pmu, &pmus, entry) {
> > > > +               if (pmu->id == pmu_id)
> > > > +                       return pmu;
> > > > +       }
> > > > +
> > > > +       return NULL;
> > > > +}
> > > 
> > > > +void perf_event_register_pmu(struct pmu *pmu)
> > > > +{
> > > > +       pmu->id = pmu_id_curr++;
> > > > +       list_add_tail(&pmu->entry, &pmus);
> > > > +} 
> > > 
> > > That will be wanting some sort of synchronization
> > 
> > Will add a mutex to protect the list of pmus.
> 
> I'm thinking RCU might be better suited, a mutex for lookup doesn't
> sound ideal.


Is it really needed? I expect this function to be called on boot
only.

In fact I would even suggest to tag it as __init.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Peter Zijlstra]

On Mon, 2010-05-10 at 14:27 +0200, Frederic Weisbecker wrote:
> On Mon, May 10, 2010 at 12:19:29PM +0200, Peter Zijlstra wrote:
> > On Mon, 2010-05-10 at 18:17 +0800, Lin Ming wrote:
> > > On Mon, 2010-05-10 at 17:40 +0800, Peter Zijlstra wrote:
> > > > On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> > > > > +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> > > > > +{
> > > > > +       struct pmu *pmu;
> > > > > +       int pmu_id = event->attr.pmu_id;
> > > > > +
> > > > > +       list_for_each_entry(pmu, &pmus, entry) {
> > > > > +               if (pmu->id == pmu_id)
> > > > > +                       return pmu;
> > > > > +       }
> > > > > +
> > > > > +       return NULL;
> > > > > +}
> > > > 
> > > > > +void perf_event_register_pmu(struct pmu *pmu)
> > > > > +{
> > > > > +       pmu->id = pmu_id_curr++;
> > > > > +       list_add_tail(&pmu->entry, &pmus);
> > > > > +} 
> > > > 
> > > > That will be wanting some sort of synchronization
> > > 
> > > Will add a mutex to protect the list of pmus.
> > 
> > I'm thinking RCU might be better suited, a mutex for lookup doesn't
> > sound ideal.
> 
> 
> Is it really needed? I expect this function to be called on boot
> only.
> 
> In fact I would even suggest to tag it as __init.

Loadable modules as well as PCI-Hotplug need supporting.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Frederic Weisbecker]

On Mon, May 10, 2010 at 02:54:14PM +0200, Peter Zijlstra wrote:
> On Mon, 2010-05-10 at 14:27 +0200, Frederic Weisbecker wrote:
> > On Mon, May 10, 2010 at 12:19:29PM +0200, Peter Zijlstra wrote:
> > > On Mon, 2010-05-10 at 18:17 +0800, Lin Ming wrote:
> > > > On Mon, 2010-05-10 at 17:40 +0800, Peter Zijlstra wrote:
> > > > > On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> > > > > > +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> > > > > > +{
> > > > > > +       struct pmu *pmu;
> > > > > > +       int pmu_id = event->attr.pmu_id;
> > > > > > +
> > > > > > +       list_for_each_entry(pmu, &pmus, entry) {
> > > > > > +               if (pmu->id == pmu_id)
> > > > > > +                       return pmu;
> > > > > > +       }
> > > > > > +
> > > > > > +       return NULL;
> > > > > > +}
> > > > > 
> > > > > > +void perf_event_register_pmu(struct pmu *pmu)
> > > > > > +{
> > > > > > +       pmu->id = pmu_id_curr++;
> > > > > > +       list_add_tail(&pmu->entry, &pmus);
> > > > > > +} 
> > > > > 
> > > > > That will be wanting some sort of synchronization
> > > > 
> > > > Will add a mutex to protect the list of pmus.
> > > 
> > > I'm thinking RCU might be better suited, a mutex for lookup doesn't
> > > sound ideal.
> > 
> > 
> > Is it really needed? I expect this function to be called on boot
> > only.
> > 
> > In fact I would even suggest to tag it as __init.
> 
> Loadable modules as well as PCI-Hotplug need supporting.


Which module do you have in mind that could register a pmu?
And I don't understand the problem with pci-hotplug.

Thanks.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Peter Zijlstra]

On Tue, 2010-05-11 at 01:09 +0200, Frederic Weisbecker wrote:

> Which module do you have in mind that could register a pmu?
> And I don't understand the problem with pci-hotplug.

Well, the DRM drivers for one, but basically the trend is for every
aspect of the machine to include PMUs of some form, this includes bus
bridges and fancy devices.

The point about PCI-hotplug is that GPUs live on the PCI bus and once
they start adding their PMU drivers we need to be able to unplug them.

Same for when the PCI bridge devices start featuring PMU
implementations.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Lin Ming]

On Tue, 2010-05-11 at 14:38 +0800, Peter Zijlstra wrote:
> On Tue, 2010-05-11 at 01:09 +0200, Frederic Weisbecker wrote:
> 
> > Which module do you have in mind that could register a pmu?
> > And I don't understand the problem with pci-hotplug.
> 
> Well, the DRM drivers for one, but basically the trend is for every
> aspect of the machine to include PMUs of some form, this includes bus
> bridges and fancy devices.

So the concept of "PMU" is really broadened here?
Not just only the performance monitoring hardware in CPU?

> 
> The point about PCI-hotplug is that GPUs live on the PCI bus and once
> they start adding their PMU drivers we need to be able to unplug them.
> 
> Same for when the PCI bridge devices start featuring PMU
> implementations.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Ingo Molnar]

* Lin Ming <ming.m.lin@intel.com> wrote:

> On Tue, 2010-05-11 at 14:38 +0800, Peter Zijlstra wrote:
> > On Tue, 2010-05-11 at 01:09 +0200, Frederic Weisbecker wrote:
> > 
> > > Which module do you have in mind that could register a pmu?
> > > And I don't understand the problem with pci-hotplug.
> > 
> > Well, the DRM drivers for one, but basically the trend is for every
> > aspect of the machine to include PMUs of some form, this includes bus
> > bridges and fancy devices.
> 
> So the concept of "PMU" is really broadened here?
> Not just only the performance monitoring hardware in CPU?

We _really_ dont want to call it a 'PMU' but 'events coming from an event 
source'.

The reason is that a PMU is an existing term that is quite attached to a CPU - 
while many hardware events come not from a PMU. Interrupts, error conditions, 
hotplug events, etc. etc.

Furthermore, the name 'PMU' is even less correct for software events.

So lets stick with 'events' and with some container that originates them. 
(event_source) Ok?

	Ingo

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Lin Ming]

On Tue, 2010-05-11 at 14:51 +0800, Ingo Molnar wrote:
> * Lin Ming <ming.m.lin@intel.com> wrote:
> 
> > On Tue, 2010-05-11 at 14:38 +0800, Peter Zijlstra wrote:
> > > On Tue, 2010-05-11 at 01:09 +0200, Frederic Weisbecker wrote:
> > > 
> > > > Which module do you have in mind that could register a pmu?
> > > > And I don't understand the problem with pci-hotplug.
> > > 
> > > Well, the DRM drivers for one, but basically the trend is for every
> > > aspect of the machine to include PMUs of some form, this includes bus
> > > bridges and fancy devices.
> > 
> > So the concept of "PMU" is really broadened here?
> > Not just only the performance monitoring hardware in CPU?
> 
> We _really_ dont want to call it a 'PMU' but 'events coming from an event 
> source'.
> 
> The reason is that a PMU is an existing term that is quite attached to a CPU - 
> while many hardware events come not from a PMU. Interrupts, error conditions, 
> hotplug events, etc. etc.
> 
> Furthermore, the name 'PMU' is even less correct for software events.
> 
> So lets stick with 'events' and with some container that originates them. 
> (event_source) Ok?

Yeah, understand.
"events coming from an event source" is a good clarification.

Thanks.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Peter Zijlstra]

On Tue, 2010-05-11 at 14:44 +0800, Lin Ming wrote:
> On Tue, 2010-05-11 at 14:38 +0800, Peter Zijlstra wrote:
> > On Tue, 2010-05-11 at 01:09 +0200, Frederic Weisbecker wrote:
> > 
> > > Which module do you have in mind that could register a pmu?
> > > And I don't understand the problem with pci-hotplug.
> > 
> > Well, the DRM drivers for one, but basically the trend is for every
> > aspect of the machine to include PMUs of some form, this includes bus
> > bridges and fancy devices.
> 
> So the concept of "PMU" is really broadened here?
> Not just only the performance monitoring hardware in CPU?

Arguably the uncore unit is already outside of what we traditionally
call a CPU ;-)

But yeah, PMUs are found in lots of things these days.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Frederic Weisbecker]

On Tue, May 11, 2010 at 08:38:55AM +0200, Peter Zijlstra wrote:
> On Tue, 2010-05-11 at 01:09 +0200, Frederic Weisbecker wrote:
> 
> > Which module do you have in mind that could register a pmu?
> > And I don't understand the problem with pci-hotplug.
> 
> Well, the DRM drivers for one, but basically the trend is for every
> aspect of the machine to include PMUs of some form, this includes bus
> bridges and fancy devices.
> 
> The point about PCI-hotplug is that GPUs live on the PCI bus and once
> they start adding their PMU drivers we need to be able to unplug them.
> 
> Same for when the PCI bridge devices start featuring PMU
> implementations.


Ah ok.

Thanks.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Peter Zijlstra]

On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> A new field "pmu_id" is added to struct perf_event_attr.
> 
> 2 new functions: perf_event_register_pmu, perf_event_lookup_pmu
> perf_event_register_pmu: the pmu registration facility
> perf_event_lookup_pmu: lookup the pmu via the passed in event->attr.pmu_id.
> 
> A new api pmu->init_event to replace hw_perf_event_init

Another problem, this patch breaks all of perf by removing the weak call
before there is a replacement.

Please rework such that there is a smooth transition.

Also, for compatibilities sake, the cpu pmu will be required to get
pmu_id==0, this doesn't provide that.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Lin Ming]

On Mon, 2010-05-10 at 17:42 +0800, Peter Zijlstra wrote:
> On Mon, 2010-05-10 at 17:26 +0800, Lin Ming wrote:
> > A new field "pmu_id" is added to struct perf_event_attr.
> > 
> > 2 new functions: perf_event_register_pmu, perf_event_lookup_pmu
> > perf_event_register_pmu: the pmu registration facility
> > perf_event_lookup_pmu: lookup the pmu via the passed in event->attr.pmu_id.
> > 
> > A new api pmu->init_event to replace hw_perf_event_init
> 
> Another problem, this patch breaks all of perf by removing the weak call
> before there is a replacement.
> 
> Please rework such that there is a smooth transition.

OK, I'll rework

> 
> Also, for compatibilities sake, the cpu pmu will be required to get
> pmu_id==0, this doesn't provide that.
> 

Will provide that.

Thanks,
Lin Ming

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Frederic Weisbecker]

On Mon, May 10, 2010 at 05:26:35PM +0800, Lin Ming wrote:
> A new field "pmu_id" is added to struct perf_event_attr.
> 
> 2 new functions: perf_event_register_pmu, perf_event_lookup_pmu
> perf_event_register_pmu: the pmu registration facility
> perf_event_lookup_pmu: lookup the pmu via the passed in event->attr.pmu_id.
> 
> A new api pmu->init_event to replace hw_perf_event_init
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  include/linux/perf_event.h |    9 +++++++++
>  kernel/perf_event.c        |   39 +++++++++++++++++++++++++++++++--------
>  2 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 5856d3b..6246c99 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -225,6 +225,8 @@ struct perf_event_attr {
>  	__u32			bp_type;
>  	__u64			bp_addr;
>  	__u64			bp_len;
> +
> +	int			pmu_id;
>  };
>  
>  /*
> @@ -553,6 +555,9 @@ struct perf_event;
>   * struct pmu - generic performance monitoring unit
>   */
>  struct pmu {
> +	int id;
> +	struct list_head entry;
> +
>  	int (*enable)			(struct perf_event *event);
>  	void (*disable)			(struct perf_event *event);
>  	int (*start)			(struct perf_event *event);
> @@ -569,6 +574,8 @@ struct pmu {
>  	void (*start_txn)	(struct pmu *pmu);
>  	void (*cancel_txn)	(struct pmu *pmu);
>  	int  (*commit_txn)	(struct pmu *pmu);
> +
> +	int  (*init_event)	(struct perf_event *event);
>  };
>  
>  /**
> @@ -1014,6 +1021,8 @@ extern int perf_swevent_get_recursion_context(void);
>  extern void perf_swevent_put_recursion_context(int rctx);
>  extern void perf_event_enable(struct perf_event *event);
>  extern void perf_event_disable(struct perf_event *event);
> +
> +extern void perf_event_register_pmu(struct pmu *pmu);
>  #else
>  static inline void
>  perf_event_task_sched_in(struct task_struct *task)			{ }
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 36baf85..f19d40e 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -40,6 +40,12 @@
>   */
>  static DEFINE_PER_CPU(struct perf_cpu_context, perf_cpu_context);
>  
> +/*
> + * The list of multiple hw pmus
> + */
> +static struct list_head pmus;
> +static int pmu_id_curr;
> +
>  int perf_max_events __read_mostly = 1;
>  static int perf_reserved_percpu __read_mostly;
>  static int perf_overcommit __read_mostly = 1;
> @@ -75,11 +81,6 @@ static DEFINE_SPINLOCK(perf_resource_lock);
>  /*
>   * Architecture provided APIs - weak aliases:
>   */
> -extern __weak struct pmu *hw_perf_event_init(struct perf_event *event)
> -{
> -	return NULL;
> -}
> -
>  void __weak hw_perf_disable(void)		{ barrier(); }
>  void __weak hw_perf_enable(void)		{ barrier(); }
>  
> @@ -4670,6 +4671,19 @@ static struct pmu *sw_perf_event_init(struct perf_event *event)
>  	return pmu;
>  }
>  
> +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> +{
> +	struct pmu *pmu;
> +	int pmu_id = event->attr.pmu_id;
> +
> +	list_for_each_entry(pmu, &pmus, entry) {
> +		if (pmu->id == pmu_id)
> +			return pmu;
> +	}
> +
> +	return NULL;
> +}
> +
>  /*
>   * Allocate and initialize a event structure
>   */
> @@ -4685,7 +4699,7 @@ perf_event_alloc(struct perf_event_attr *attr,
>  	struct pmu *pmu;
>  	struct perf_event *event;
>  	struct hw_perf_event *hwc;
> -	long err;
> +	long err = 0;
>  
>  	event = kzalloc(sizeof(*event), gfpflags);
>  	if (!event)
> @@ -4750,7 +4764,9 @@ perf_event_alloc(struct perf_event_attr *attr,
>  	case PERF_TYPE_RAW:
>  	case PERF_TYPE_HARDWARE:
>  	case PERF_TYPE_HW_CACHE:
> -		pmu = hw_perf_event_init(event);
> +		pmu = perf_event_lookup_pmu(event);
> +		if (pmu && pmu->init_event)
> +			err = pmu->init_event(event);



Having the same for software, tracepoints and breakpoints events would
be nice, so that we have a single simple path in perf_event_alloc
to initialize the event.

Thanks.

Re: [RFC][PATCH 2/9] perf: core, remove hw_perf_event_init

[Lin Ming]

On Mon, 2010-05-10 at 20:24 +0800, Frederic Weisbecker wrote:
> On Mon, May 10, 2010 at 05:26:35PM +0800, Lin Ming wrote:
> > A new field "pmu_id" is added to struct perf_event_attr.
> > 
> > 2 new functions: perf_event_register_pmu, perf_event_lookup_pmu
> > perf_event_register_pmu: the pmu registration facility
> > perf_event_lookup_pmu: lookup the pmu via the passed in event->attr.pmu_id.
> > 
> > A new api pmu->init_event to replace hw_perf_event_init
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >  include/linux/perf_event.h |    9 +++++++++
> >  kernel/perf_event.c        |   39 +++++++++++++++++++++++++++++++--------
> >  2 files changed, 40 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index 5856d3b..6246c99 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -225,6 +225,8 @@ struct perf_event_attr {
> >  	__u32			bp_type;
> >  	__u64			bp_addr;
> >  	__u64			bp_len;
> > +
> > +	int			pmu_id;
> >  };
> >  
> >  /*
> > @@ -553,6 +555,9 @@ struct perf_event;
> >   * struct pmu - generic performance monitoring unit
> >   */
> >  struct pmu {
> > +	int id;
> > +	struct list_head entry;
> > +
> >  	int (*enable)			(struct perf_event *event);
> >  	void (*disable)			(struct perf_event *event);
> >  	int (*start)			(struct perf_event *event);
> > @@ -569,6 +574,8 @@ struct pmu {
> >  	void (*start_txn)	(struct pmu *pmu);
> >  	void (*cancel_txn)	(struct pmu *pmu);
> >  	int  (*commit_txn)	(struct pmu *pmu);
> > +
> > +	int  (*init_event)	(struct perf_event *event);
> >  };
> >  
> >  /**
> > @@ -1014,6 +1021,8 @@ extern int perf_swevent_get_recursion_context(void);
> >  extern void perf_swevent_put_recursion_context(int rctx);
> >  extern void perf_event_enable(struct perf_event *event);
> >  extern void perf_event_disable(struct perf_event *event);
> > +
> > +extern void perf_event_register_pmu(struct pmu *pmu);
> >  #else
> >  static inline void
> >  perf_event_task_sched_in(struct task_struct *task)			{ }
> > diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> > index 36baf85..f19d40e 100644
> > --- a/kernel/perf_event.c
> > +++ b/kernel/perf_event.c
> > @@ -40,6 +40,12 @@
> >   */
> >  static DEFINE_PER_CPU(struct perf_cpu_context, perf_cpu_context);
> >  
> > +/*
> > + * The list of multiple hw pmus
> > + */
> > +static struct list_head pmus;
> > +static int pmu_id_curr;
> > +
> >  int perf_max_events __read_mostly = 1;
> >  static int perf_reserved_percpu __read_mostly;
> >  static int perf_overcommit __read_mostly = 1;
> > @@ -75,11 +81,6 @@ static DEFINE_SPINLOCK(perf_resource_lock);
> >  /*
> >   * Architecture provided APIs - weak aliases:
> >   */
> > -extern __weak struct pmu *hw_perf_event_init(struct perf_event *event)
> > -{
> > -	return NULL;
> > -}
> > -
> >  void __weak hw_perf_disable(void)		{ barrier(); }
> >  void __weak hw_perf_enable(void)		{ barrier(); }
> >  
> > @@ -4670,6 +4671,19 @@ static struct pmu *sw_perf_event_init(struct perf_event *event)
> >  	return pmu;
> >  }
> >  
> > +static struct pmu *perf_event_lookup_pmu(struct perf_event *event)
> > +{
> > +	struct pmu *pmu;
> > +	int pmu_id = event->attr.pmu_id;
> > +
> > +	list_for_each_entry(pmu, &pmus, entry) {
> > +		if (pmu->id == pmu_id)
> > +			return pmu;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  /*
> >   * Allocate and initialize a event structure
> >   */
> > @@ -4685,7 +4699,7 @@ perf_event_alloc(struct perf_event_attr *attr,
> >  	struct pmu *pmu;
> >  	struct perf_event *event;
> >  	struct hw_perf_event *hwc;
> > -	long err;
> > +	long err = 0;
> >  
> >  	event = kzalloc(sizeof(*event), gfpflags);
> >  	if (!event)
> > @@ -4750,7 +4764,9 @@ perf_event_alloc(struct perf_event_attr *attr,
> >  	case PERF_TYPE_RAW:
> >  	case PERF_TYPE_HARDWARE:
> >  	case PERF_TYPE_HW_CACHE:
> > -		pmu = hw_perf_event_init(event);
> > +		pmu = perf_event_lookup_pmu(event);
> > +		if (pmu && pmu->init_event)
> > +			err = pmu->init_event(event);
> 
> 
> 
> Having the same for software, tracepoints and breakpoints events would
> be nice, so that we have a single simple path in perf_event_alloc
> to initialize the event.

Will add this later.
For now, let's first work on the hw pmus.

> 
> Thanks.
>