Messages in this thread | | | Date | Wed, 6 Jul 2016 16:40:47 +0100 | From | Mark Rutland <> | Subject | Re: [RFC][PATCH] perf: Remove superfluous perf_pmu_disable calls |
| |
On Wed, Jul 06, 2016 at 04:09:31PM +0200, Peter Zijlstra wrote: > > Hai, > > So pmu::add() and pmu::del() are guaranteed to be called with ctx->lock > held, which implies that local IRQs are disabled. > > Furthermore, it is also guaranteed that perf_pmu_disable() is already > called when we call these methods.
It's probably worth noting that the latter is true since commit 443772776c69ac92 ("perf: Disable all pmus on unthrottling and rescheduling"), circa December 2013.
Much of this code was likely written before that, or cargo-culted from existing code which was.
> The following patch removes all perf_pmu_{dis,en}able() calls (and > local_irq_disable() where encountered) from pmu::{add,del}() > implementations. > > pmu::{start,stop}() are a little bit tricky, since we can call them from > the PMI handler, but we do guarantee local IRQs are disabled. PPC in > particular seems to need perf_pmu_{dis,en}able() there to actually > reprogram things, this is safe for them since they don't have actual > NMIs I suppose. > > --- > arch/alpha/kernel/perf_event.c | 22 ---------------------- > arch/mips/kernel/perf_event_mipsxx.c | 3 --- > arch/powerpc/perf/core-book3s.c | 16 +++------------- > arch/powerpc/perf/core-fsl-emb.c | 7 +++---- > arch/s390/kernel/perf_cpum_sf.c | 4 ---- > arch/sh/kernel/perf_event.c | 3 --- > b/arch/blackfin/kernel/perf_event.c | 3 --- > b/arch/metag/kernel/perf/perf_event.c | 3 --- > b/arch/sparc/kernel/perf_event.c | 9 --------- > drivers/bus/arm-cci.c | 3 --- > drivers/perf/arm_pmu.c | 3 --- > kernel/events/core.c | 6 ++++++ > 12 files changed, 12 insertions(+), 70 deletions(-)
[...]
> --- a/drivers/bus/arm-cci.c > +++ b/drivers/bus/arm-cci.c > @@ -1230,8 +1230,6 @@ static int cci_pmu_add(struct perf_event > int idx; > int err = 0; > > - perf_pmu_disable(event->pmu); > - > /* If we don't have a space for the counter then finish early. */ > idx = pmu_get_event_idx(hw_events, event); > if (idx < 0) { > @@ -1250,7 +1248,6 @@ static int cci_pmu_add(struct perf_event > perf_event_update_userpage(event); > > out: > - perf_pmu_enable(event->pmu); > return err; > } > > --- a/drivers/perf/arm_pmu.c > +++ b/drivers/perf/arm_pmu.c > @@ -240,8 +240,6 @@ armpmu_add(struct perf_event *event, int > if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus)) > return -ENOENT; > > - perf_pmu_disable(event->pmu); > - > /* If we don't have a space for the counter then finish early. */ > idx = armpmu->get_event_idx(hw_events, event); > if (idx < 0) { > @@ -265,7 +263,6 @@ armpmu_add(struct perf_event *event, int > perf_event_update_userpage(event); > > out: > - perf_pmu_enable(event->pmu); > return err; > }
These both look right to me. The only caller of either is event_sched_in(), which handles the disable/enable itself. That uses event->pmu, so the heterogeneous case doesn't throw a spanner in the works here.
> --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -8597,6 +8597,12 @@ int perf_pmu_register(struct pmu *pmu, c > } > } > > + /* > + * Software events cannot have pmu_{en,dis}able() calls because that > + * would make it 'hard' to put them in groups with hardware events. > + */ > + WARN_ON_ONCE(pmu->task_ctx_nr == perf_sw_event && pmu->pmu_enable); > + > if (!pmu->pmu_enable) { > pmu->pmu_enable = perf_pmu_nop_void; > pmu->pmu_disable = perf_pmu_nop_void; >
Looks sensible to me.
For the parts above:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Thanks, Mark.
| |