From: Frederic Weisbecker on
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(a)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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Lin Ming on
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/