Re: [PATCH] perf/core / arm_pmu: special-case hetereogeneous CPUs

[Mark Rutland]

Hi,

On Tue, Apr 26, 2016 at 11:33:46AM +0100, Mark Rutland wrote:
> On Mon, Apr 25, 2016 at 09:03:34PM +0200, Peter Zijlstra wrote:
> > On Mon, Apr 25, 2016 at 06:58:37PM +0100, Mark Rutland wrote:
> > > Hi,
> > > 
> > > When booting an arm64 defconfig linux-next (next-20160422) on an ARM
> > > Juno system, I hit a WARN_ON_ONCE in perf_pmu_register (see backtrace at
> > > the end of this email).
> > > 
> > > This was introduced by commit 26657848502b7847 ("perf/core: Verify we
> > > have a single perf_hw_context PMU") where we forcefully prevent multiple
> > > PMUs from sharing perf_hw_context (with a warning), and force additional
> > > PMUs to use perf_invalid_context.
> 
> > > Are you happy to revert 26657848502b787 for the timebeing? Or to somehow
> > > predicate the check such that it doesn't adversely affect those HW PMUs?
> > 
> > I'm happy with a chicken bit for now, its already found two real issues,
> > so I'd like to keep it.
> 
> Ok, how about the below? (based on next-20160422).

Peter, any thoughts?

This is still an issue for us in next-20160504 (to which the patch still
applies).

Will, I assume that you're ok with the change to drivers/perf/arm_pmu.c.

Thanks,
Mark.

> It looks like 26657848502b7847 was on a stable branch, so I guess we
> can't fold this in. It probably makes sense for this to go the same path
> as 26657848502b7847, assuming people are happy with that and there are
> no conflicts.
> 
> Mark.
> 
> ---->8----
> From 7b1007f86d30bfed1dde21218224f119b6ad547f Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@xxxxxxx>
> Date: Tue, 26 Apr 2016 11:17:51 +0100
> Subject: [PATCH] perf/core / arm_pmu: special-case hetereogeneous CPUs
> 
> Commit 26657848502b7847 ("perf/core: Verify we have a single
> perf_hw_context PMU") forcefully prevents multiple PMUs from sharing
> perf_hw_context, as this generally doesn't make sense. It is a common
> bug for uncore PMUs to use perf_hw_context rather than
> perf_invalid_context, which this detects.
> 
> However, systems exist with heterogeneous CPUs (and hence heterogeneous
> HW PMUs), for which sharing perf_hw_context is necessary, and possible
> in some limited cases. To make this work we have to perform some
> gymnastics, as we did in commit 66eb579e66ecfea5 ("perf: allow for
> PMU-specific event filtering") and c904e32a69b7c779 ("arm: perf: filter
> unschedulable events").
> 
> To allow those systems to work, we must allow PMUs for heterogeneous
> CPUs to share perf_hw_context, though we must still disallow sharing
> otherwise to detect the common misuse of perf_hw_context.
> 
> This patch adds a new PERF_PMU_CAP_HETEROGENEOUS_CPUS for this, updates
> the core logic to account for this, and makes use of it in the arm_pmu
> code that is used for systems with heterogeneous CPUs. Comments are
> added to make the rationale clear and hopefully avoid accidental abuse.
> 
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> CC: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  drivers/perf/arm_pmu.c     | 8 ++++++++
>  include/linux/perf_event.h | 1 +
>  kernel/events/core.c       | 8 +++++++-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 32346b5..bbf827a 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -836,6 +836,14 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>  	if (!platform_get_irq(cpu_pmu->plat_device, 0))
>  		cpu_pmu->pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
>  
> +	/*
> +	 * This is a CPU PMU potentially in a heterogeneous configuration (e.g.
> +	 * big.LITTLE). This is not an uncore PMU, and we have taken ctx
> +	 * sharing into account (e.g. with our pmu::filter_match callback and
> +	 * pmu::event_init group validation).
> +	 */
> +	cpu_pmu->pmu.capabilities |= PERF_PMU_CAP_HETEROGENEOUS_CPUS;
> +
>  	return 0;
>  
>  out_unregister:
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 342cb92..18d19d6 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -216,6 +216,7 @@ struct perf_event;
>  #define PERF_PMU_CAP_AUX_SW_DOUBLEBUF		0x08
>  #define PERF_PMU_CAP_EXCLUSIVE			0x10
>  #define PERF_PMU_CAP_ITRACE			0x20
> +#define PERF_PMU_CAP_HETEROGENEOUS_CPUS		0x40
>  
>  /**
>   * struct pmu - generic performance monitoring unit
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 1e0f117..796ee56 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7855,7 +7855,13 @@ skip_type:
>  	if (pmu->task_ctx_nr == perf_hw_context) {
>  		static int hw_context_taken = 0;
>  
> -		if (WARN_ON_ONCE(hw_context_taken))
> +		/*
> +		 * Other than systems with heterogeneous CPUs, it never makes
> +		 * sense for two PMUs to share perf_hw_context. PMUs which are
> +		 * uncore must use perf_invalid_context.
> +		 */
> +		if (WARN_ON_ONCE(hw_context_taken &&
> +		    !(pmu->capabilities & PERF_PMU_CAP_HETEROGENEOUS_CPUS)))
>  			pmu->task_ctx_nr = perf_invalid_context;
>  
>  		hw_context_taken = 1;
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>