perf: register pmu implementations
in [Kernel]
|
Prev: [PATCH] procfs: Do not release pid_ns->proc_mnt too early
Next: perf/perf_events: misleading number of samples due to mmap()
From: Robert Richter on 16 Jun 2010 12:50 On 16.06.10 12:00:30, Peter Zijlstra wrote: > Simple registration interface for struct pmu, this provides the > infrastructure for removing all the weak functions. > > Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > --- > arch/arm/kernel/perf_event.c | 36 + > arch/powerpc/kernel/perf_event.c | 41 +- > arch/powerpc/kernel/perf_event_fsl_emb.c | 31 - > arch/sh/kernel/perf_event.c | 19 > arch/sparc/kernel/perf_event.c | 13 > arch/x86/kernel/cpu/perf_event.c | 45 +- > include/linux/perf_event.h | 10 > kernel/hw_breakpoint.c | 35 + > kernel/perf_event.c | 597 +++++++++++++++---------------- > 9 files changed, 432 insertions(+), 395 deletions(-) Yes, this makes the code much more easier! Now it would be possible to easy register multiple hw pmus. Thanks Peter, -Robert -- Advanced Micro Devices, Inc. Operating System Research Center email: robert.richter(a)amd.com -- 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 16 Jun 2010 13:10 On Wed, Jun 16, 2010 at 06:00:30PM +0200, Peter Zijlstra wrote: > +static void bp_perf_event_destroy(struct perf_event *event) > +{ > + release_bp_slot(event); > +} > + > +static struct pmu *bp_perf_event_init(struct perf_event *bp) > +{ > + int err; > + > + if (event->attr.type != PERF_TYPE_BREAKPOINT) > + return -ENOENT; > + > + err = register_perf_hw_breakpoint(bp); > + if (err) > + return err; > + > + bp->destroy = bp_perf_event_destroy; > + > + return 0; > +} > + > +static struct pmu perf_breakpoint = { > + .event_init = hw_breakpoint_event_init, Should be bp_perf_event_init? > + .enable = arch_install_hw_breakpoint, > + .disable = arch_uninstall_hw_breakpoint, > + .read = hw_breakpoint_pmu_read, > +}; <snip> > +static int perf_swevent_int(struct perf_event *event) > +{ > + if (event->attr.type != PERF_TYPE_SOFTWARE) > + return -ENOENT perf_swevent_init() ? > +int perf_pmu_register(struct pmu *pmu) > +{ > + spin_lock(&pmus_lock); > + list_add_rcu(&pmu->entry, &pmus); > + spin_unlock(&pmus_lock); > + > + return 0; > +} > + > +void perf_pmu_unregister(struct pmu *pmu) > +{ > + spin_lock(&pmus_lock); > + list_del_rcu(&pmu->entry); > + spin_unlock(&pmus_lock); > + > + synchronize_srcu(&pmus_srcu); > +} Who needs this? > + > +struct pmu *perf_init_event(struct perf_event *event) > +{ > + struct pmu *pmu; > + int idx; > + > + idx = srcu_read_lock(&pmus_srcu); > + list_for_each_entry_rcu(pmu, &pmus, entry) { > + int ret = pmu->event_init(event); > + if (!ret) > + break; > + if (ret != -ENOENT) { > + pmu = ERR_PTR(ret); > + break; > } > - pmu = &perf_ops_generic; > - break; > } > + srcu_read_unlock(&pmus_srcu, idx); This could use a simple mutex instead of a spinlock + srcu_sync on writer and srcu on reader. That doesn't matter much that said. What I don't understand is why we need to synchronize the writers. Walking the list with list_*_rcu() looks justified once we support boot events, but until then... For the rest of the patch, the whole idea is nice. -- 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 16 Jun 2010 13:50 On Wed, 2010-06-16 at 19:03 +0200, Frederic Weisbecker wrote: > > +static struct pmu perf_breakpoint = { > > + .event_init = hw_breakpoint_event_init, > > > > Should be bp_perf_event_init? Ah, yes, like said, the compiler didn't get near yet.. > > > + .enable = arch_install_hw_breakpoint, > > + .disable = arch_uninstall_hw_breakpoint, > > + .read = hw_breakpoint_pmu_read, > > +}; > <snip> > > +static int perf_swevent_int(struct perf_event *event) > > +{ > > + if (event->attr.type != PERF_TYPE_SOFTWARE) > > + return -ENOENT > > > perf_swevent_init() ? copy/paste gone wild.. > > +void perf_pmu_unregister(struct pmu *pmu) > > +{ > > + spin_lock(&pmus_lock); > > + list_del_rcu(&pmu->entry); > > + spin_unlock(&pmus_lock); > > + > > + synchronize_srcu(&pmus_srcu); > > +} > Who needs this? Nobody yet.. > > + > > +struct pmu *perf_init_event(struct perf_event *event) > > +{ > > + struct pmu *pmu; > > + int idx; > > + > > + idx = srcu_read_lock(&pmus_srcu); > > + list_for_each_entry_rcu(pmu, &pmus, entry) { > > + int ret = pmu->event_init(event); > > + if (!ret) > > + break; > > + if (ret != -ENOENT) { > > + pmu = ERR_PTR(ret); > > + break; > > } > > - pmu = &perf_ops_generic; > > - break; > > } > > + srcu_read_unlock(&pmus_srcu, idx); > > > > This could use a simple mutex instead of a spinlock + srcu_sync on > writer and srcu on reader. Right, that spinlock needs to be a mutex for sure, a later patch adds an allocation under it. But even with a mutex we need srcu_sync in there to sync against the readers. > That doesn't matter much that said. What I don't understand is > why we need to synchronize the writers. Walking the list with > list_*_rcu() looks justified once we support boot events, but > until then... Well, the typical unregister user would be a module, if you unregister and then dealloc the struct pmu by unloading the module a reader might still see a reference to it if you don't srcu_sync it. -- 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 16 Jun 2010 14:20 On Wed, 2010-06-16 at 19:48 +0200, Peter Zijlstra wrote: > > > +static int perf_swevent_int(struct perf_event *event) > > > +{ > > > + if (event->attr.type != PERF_TYPE_SOFTWARE) > > > + return -ENOENT > > > > > > perf_swevent_init() ? > > copy/paste gone wild.. /me learns how to read again,.. D'0h! -- 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: Paul Mackerras on 17 Jun 2010 21:50
On Wed, Jun 16, 2010 at 06:00:30PM +0200, Peter Zijlstra wrote: > Simple registration interface for struct pmu, this provides the > infrastructure for removing all the weak functions. Nice idea... > @@ -1011,7 +1001,7 @@ static int hw_perf_cache_event(u64 confi > return 0; > } > > -struct pmu *hw_perf_event_init(struct perf_event *event) > +static int power_pmu_event_init(struct perf_event *event) How does power_pmu_event_init ever get called now? I don't see any other references to it in the patch. Should struct pmu have a reference to it? Paul. -- 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/ |