linux-kernel - Re: [RFC][PATCH] perf: Remove superfluous perf_pmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160706154031.GA5693@leverpostej>
Date:	Wed, 6 Jul 2016 16:40:47 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Richard Henderson <rth@...ddle.net>,
	Ivan Kokshaysky <ink@...assic.park.msu.ru>,
	Matt Turner <mattst88@...il.com>,
	Steven Miao <realmz6@...il.com>,
	James Hogan <james.hogan@...tec.com>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>,
	Michael Ellerman <mpe@...erman.id.au>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Yoshinori Sato <ysato@...rs.sourceforge.jp>,
	Rich Felker <dalias@...c.org>,
	Will Deacon <will.deacon@....com>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org
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@....com>

Thanks,
Mark.