Messages in this thread Patch in this message | | | Date | Wed, 6 Jul 2016 16:09:31 +0200 | From | Peter Zijlstra <> | Subject | [RFC][PATCH] perf: Remove superfluous perf_pmu_disable calls |
| |
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.
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/arch/alpha/kernel/perf_event.c +++ b/arch/alpha/kernel/perf_event.c @@ -435,18 +435,6 @@ static int alpha_pmu_add(struct perf_eve struct hw_perf_event *hwc = &event->hw; int n0; int ret; - unsigned long irq_flags; - - /* - * The Sparc code has the IRQ disable first followed by the perf - * disable, however this can lead to an overflowed counter with the - * PMI disabled on rare occasions. The alpha_perf_event_update() - * routine should detect this situation by noting a negative delta, - * nevertheless we disable the PMCs first to enable a potential - * final PMI to occur before we disable interrupts. - */ - perf_pmu_disable(event->pmu); - local_irq_save(irq_flags); /* Default to error to be returned */ ret = -EAGAIN; @@ -469,9 +457,6 @@ static int alpha_pmu_add(struct perf_eve if (!(flags & PERF_EF_START)) hwc->state |= PERF_HES_STOPPED; - local_irq_restore(irq_flags); - perf_pmu_enable(event->pmu); - return ret; } @@ -485,12 +470,8 @@ static void alpha_pmu_del(struct perf_ev { struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); struct hw_perf_event *hwc = &event->hw; - unsigned long irq_flags; int j; - perf_pmu_disable(event->pmu); - local_irq_save(irq_flags); - for (j = 0; j < cpuc->n_events; j++) { if (event == cpuc->event[j]) { int idx = cpuc->current_idx[j]; @@ -514,9 +495,6 @@ static void alpha_pmu_del(struct perf_ev break; } } - - local_irq_restore(irq_flags); - perf_pmu_enable(event->pmu); } --- a/arch/blackfin/kernel/perf_event.c +++ b/arch/blackfin/kernel/perf_event.c @@ -350,8 +350,6 @@ static int bfin_pmu_add(struct perf_even int idx = hwc->idx; int ret = -EAGAIN; - perf_pmu_disable(event->pmu); - if (__test_and_set_bit(idx, cpuc->used_mask)) { idx = find_first_zero_bit(cpuc->used_mask, MAX_HWEVENTS); if (idx == MAX_HWEVENTS) @@ -370,7 +368,6 @@ static int bfin_pmu_add(struct perf_even perf_event_update_userpage(event); ret = 0; out: - perf_pmu_enable(event->pmu); return ret; } --- a/arch/metag/kernel/perf/perf_event.c +++ b/arch/metag/kernel/perf/perf_event.c @@ -310,8 +310,6 @@ static int metag_pmu_add(struct perf_eve struct hw_perf_event *hwc = &event->hw; int idx = 0, ret = 0; - perf_pmu_disable(event->pmu); - /* check whether we're counting instructions */ if (hwc->config == 0x100) { if (__test_and_set_bit(METAG_INST_COUNTER, @@ -342,7 +340,6 @@ static int metag_pmu_add(struct perf_eve perf_event_update_userpage(event); out: - perf_pmu_enable(event->pmu); return ret; } --- a/arch/mips/kernel/perf_event_mipsxx.c +++ b/arch/mips/kernel/perf_event_mipsxx.c @@ -463,8 +463,6 @@ static int mipspmu_add(struct perf_event int idx; int err = 0; - perf_pmu_disable(event->pmu); - /* To look for a free counter for this event. */ idx = mipsxx_pmu_alloc_counter(cpuc, hwc); if (idx < 0) { @@ -488,7 +486,6 @@ static int mipspmu_add(struct perf_event perf_event_update_userpage(event); out: - perf_pmu_enable(event->pmu); return err; } --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c @@ -1406,13 +1406,9 @@ static int collect_events(struct perf_ev static int power_pmu_add(struct perf_event *event, int ef_flags) { struct cpu_hw_events *cpuhw; - unsigned long flags; int n0; int ret = -EAGAIN; - local_irq_save(flags); - perf_pmu_disable(event->pmu); - /* * Add the event to the list (if there is room) * and check whether the total set is still feasible. @@ -1464,8 +1460,6 @@ static int power_pmu_add(struct perf_eve event->attr.branch_sample_type); } - perf_pmu_enable(event->pmu); - local_irq_restore(flags); return ret; } @@ -1476,10 +1470,6 @@ static void power_pmu_del(struct perf_ev { struct cpu_hw_events *cpuhw; long i; - unsigned long flags; - - local_irq_save(flags); - perf_pmu_disable(event->pmu); power_pmu_read(event); @@ -1518,9 +1508,6 @@ static void power_pmu_del(struct perf_ev if (has_branch_stack(event)) power_pmu_bhrb_disable(event); - - perf_pmu_enable(event->pmu); - local_irq_restore(flags); } /* @@ -1543,6 +1530,9 @@ static void power_pmu_start(struct perf_ if (ef_flags & PERF_EF_RELOAD) WARN_ON_ONCE(!(event->hw.state & PERF_HES_UPTODATE)); + /* + * XXX do we really need these? If so, comment please. + */ local_irq_save(flags); perf_pmu_disable(event->pmu); --- a/arch/powerpc/perf/core-fsl-emb.c +++ b/arch/powerpc/perf/core-fsl-emb.c @@ -298,7 +298,6 @@ static int fsl_emb_pmu_add(struct perf_e u64 val; int i; - perf_pmu_disable(event->pmu); cpuhw = &get_cpu_var(cpu_hw_events); if (event->hw.config & FSL_EMB_EVENT_RESTRICTED) @@ -346,7 +345,6 @@ static int fsl_emb_pmu_add(struct perf_e ret = 0; out: put_cpu_var(cpu_hw_events); - perf_pmu_enable(event->pmu); return ret; } @@ -356,7 +354,6 @@ static void fsl_emb_pmu_del(struct perf_ struct cpu_hw_events *cpuhw; int i = event->hw.idx; - perf_pmu_disable(event->pmu); if (i < 0) goto out; @@ -384,7 +381,6 @@ static void fsl_emb_pmu_del(struct perf_ cpuhw->n_events--; out: - perf_pmu_enable(event->pmu); put_cpu_var(cpu_hw_events); } @@ -403,6 +399,9 @@ static void fsl_emb_pmu_start(struct per if (ef_flags & PERF_EF_RELOAD) WARN_ON_ONCE(!(event->hw.state & PERF_HES_UPTODATE)); + /* + * XXX do we really need these, if so comment please. + */ local_irq_save(flags); perf_pmu_disable(event->pmu); --- a/arch/s390/kernel/perf_cpum_sf.c +++ b/arch/s390/kernel/perf_cpum_sf.c @@ -1358,7 +1358,6 @@ static int cpumsf_pmu_add(struct perf_ev return -EINVAL; err = 0; - perf_pmu_disable(event->pmu); event->hw.state = PERF_HES_UPTODATE | PERF_HES_STOPPED; @@ -1392,7 +1391,6 @@ static int cpumsf_pmu_add(struct perf_ev cpumsf_pmu_start(event, PERF_EF_RELOAD); out: perf_event_update_userpage(event); - perf_pmu_enable(event->pmu); return err; } @@ -1400,7 +1398,6 @@ static void cpumsf_pmu_del(struct perf_e { struct cpu_hw_sf *cpuhw = this_cpu_ptr(&cpu_hw_sf); - perf_pmu_disable(event->pmu); cpumsf_pmu_stop(event, PERF_EF_UPDATE); cpuhw->lsctl.es = 0; @@ -1409,7 +1406,6 @@ static void cpumsf_pmu_del(struct perf_e cpuhw->event = NULL; perf_event_update_userpage(event); - perf_pmu_enable(event->pmu); } CPUMF_EVENT_ATTR(SF, SF_CYCLES_BASIC, PERF_EVENT_CPUM_SF); --- a/arch/sh/kernel/perf_event.c +++ b/arch/sh/kernel/perf_event.c @@ -269,8 +269,6 @@ static int sh_pmu_add(struct perf_event int idx = hwc->idx; int ret = -EAGAIN; - perf_pmu_disable(event->pmu); - if (__test_and_set_bit(idx, cpuc->used_mask)) { idx = find_first_zero_bit(cpuc->used_mask, sh_pmu->num_events); if (idx == sh_pmu->num_events) @@ -289,7 +287,6 @@ static int sh_pmu_add(struct perf_event perf_event_update_userpage(event); ret = 0; out: - perf_pmu_enable(event->pmu); return ret; } --- a/arch/sparc/kernel/perf_event.c +++ b/arch/sparc/kernel/perf_event.c @@ -1099,11 +1099,8 @@ static void sparc_pmu_stop(struct perf_e static void sparc_pmu_del(struct perf_event *event, int _flags) { struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); - unsigned long flags; int i; - local_irq_save(flags); - for (i = 0; i < cpuc->n_events; i++) { if (event == cpuc->event[i]) { /* Absorb the final count and turn off the @@ -1127,8 +1124,6 @@ static void sparc_pmu_del(struct perf_ev break; } } - - local_irq_restore(flags); } static void sparc_pmu_read(struct perf_event *event) @@ -1358,9 +1353,6 @@ static int sparc_pmu_add(struct perf_eve { struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); int n0, ret = -EAGAIN; - unsigned long flags; - - local_irq_save(flags); n0 = cpuc->n_events; if (n0 >= sparc_pmu->max_hw_events) @@ -1393,7 +1385,6 @@ static int sparc_pmu_add(struct perf_eve ret = 0; out: - local_irq_restore(flags); return ret; } --- 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; } --- 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;
| |