[RFC][PATCH] perf: Remove superfluous perf_pmu_disable calls

[RFC][PATCH] perf: Remove superfluous perf_pmu_disable calls

[Peter Zijlstra]

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;

Re: [RFC][PATCH] perf: Remove superfluous perf_pmu_disable calls

[Mark Rutland]

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.