[PATCH] perf/core: Fix creating kernel counters for PMUs that override event->cpu

[PATCH] perf/core: Fix creating kernel counters for PMUs that override event->cpu

[Leonard Crestez]

Some hardware PMU drivers will override perf_event.cpu inside their
event_init callback. This causes a lockdep splat when initialized through
the kernel API:

WARNING: CPU: 0 PID: 250 at kernel/events/core.c:2917 ctx_sched_out+0x78/0x208
CPU: 0 PID: 250 Comm: systemd-udevd Not tainted 5.3.0-rc1-next-20190723-00024-g94e04593c88a #80
Hardware name: FSL i.MX8MM EVK board (DT)
pstate: 40000085 (nZcv daIf -PAN -UAO)
pc : ctx_sched_out+0x78/0x208
lr : ctx_sched_out+0x78/0x208
sp : ffff0000127a3750
x29: ffff0000127a3750 x28: 0000000000000000
x27: ffff00001162bf20 x26: ffff000008cf3310
x25: ffff0000127a3de0 x24: ffff0000115ff808
x23: ffff7dffbff851b8 x22: 0000000000000004
x21: ffff7dffbff851b0 x20: 0000000000000000
x19: ffff7dffbffc51b0 x18: 0000000000000010
x17: 0000000000000001 x16: 0000000000000007
x15: 2e8ba2e8ba2e8ba3 x14: 0000000000005114
x13: ffff0000117d5e30 x12: ffff000011898378
x11: 0000000000000000 x10: ffff0000117d5000
x9 : 0000000000000045 x8 : 0000000000000000
x7 : ffff000010168194 x6 : ffff0000117d59d0
x5 : 0000000000000001 x4 : ffff80007db56128
x3 : ffff80007db56128 x2 : 0d9c118347a77600
x1 : 0000000000000000 x0 : 0000000000000024
Call trace:
 ctx_sched_out+0x78/0x208
 __perf_install_in_context+0x160/0x248
 remote_function+0x58/0x68
 generic_exec_single+0x100/0x180
 smp_call_function_single+0x174/0x1b8
 perf_install_in_context+0x178/0x188
 perf_event_create_kernel_counter+0x118/0x160

Fix by calling perf_install_in_context with event->cpu, just like
perf_event_open

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
I don't understand why PMUs outside the core are bound to a CPU anyway,
all this patch does is attempt to satisfy the assumptions made by
__perf_install_in_context and ctx_sched_out at init time so that lockdep
no longer complains.

ctx_sched_out asserts ctx->lock which seems to be taken by
__perf_install_in_context:

	struct perf_event_context *ctx = event->ctx;
	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
	...
	raw_spin_lock(&cpuctx->ctx.lock);

The lockdep warning happens when ctx != &cpuctx->ctx which can happen if
__perf_install_in_context is called on a cpu other than event->cpu.

Found while working on this patch:
https://patchwork.kernel.org/patch/11056785/

 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 026a14541a38..0463c1151bae 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11272,11 +11272,11 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
 	if (!exclusive_event_installable(event, ctx)) {
 		err = -EBUSY;
 		goto err_unlock;
 	}
 
-	perf_install_in_context(ctx, event, cpu);
+	perf_install_in_context(ctx, event, event->cpu);
 	perf_unpin_context(ctx);
 	mutex_unlock(&ctx->mutex);
 
 	return event;
 
-- 
2.17.1

Re: [PATCH] perf/core: Fix creating kernel counters for PMUs that override event->cpu

[Mark Rutland]

On Wed, Jul 24, 2019 at 03:53:24PM +0300, Leonard Crestez wrote:
> Some hardware PMU drivers will override perf_event.cpu inside their
> event_init callback. This causes a lockdep splat when initialized through
> the kernel API:
> 
> WARNING: CPU: 0 PID: 250 at kernel/events/core.c:2917 ctx_sched_out+0x78/0x208
> CPU: 0 PID: 250 Comm: systemd-udevd Not tainted 5.3.0-rc1-next-20190723-00024-g94e04593c88a #80
> Hardware name: FSL i.MX8MM EVK board (DT)
> pstate: 40000085 (nZcv daIf -PAN -UAO)
> pc : ctx_sched_out+0x78/0x208
> lr : ctx_sched_out+0x78/0x208
> sp : ffff0000127a3750
> x29: ffff0000127a3750 x28: 0000000000000000
> x27: ffff00001162bf20 x26: ffff000008cf3310
> x25: ffff0000127a3de0 x24: ffff0000115ff808
> x23: ffff7dffbff851b8 x22: 0000000000000004
> x21: ffff7dffbff851b0 x20: 0000000000000000
> x19: ffff7dffbffc51b0 x18: 0000000000000010
> x17: 0000000000000001 x16: 0000000000000007
> x15: 2e8ba2e8ba2e8ba3 x14: 0000000000005114
> x13: ffff0000117d5e30 x12: ffff000011898378
> x11: 0000000000000000 x10: ffff0000117d5000
> x9 : 0000000000000045 x8 : 0000000000000000
> x7 : ffff000010168194 x6 : ffff0000117d59d0
> x5 : 0000000000000001 x4 : ffff80007db56128
> x3 : ffff80007db56128 x2 : 0d9c118347a77600
> x1 : 0000000000000000 x0 : 0000000000000024
> Call trace:
>  ctx_sched_out+0x78/0x208
>  __perf_install_in_context+0x160/0x248
>  remote_function+0x58/0x68
>  generic_exec_single+0x100/0x180
>  smp_call_function_single+0x174/0x1b8
>  perf_install_in_context+0x178/0x188
>  perf_event_create_kernel_counter+0x118/0x160
> 
> Fix by calling perf_install_in_context with event->cpu, just like
> perf_event_open

Ouch; good spot!

> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> I don't understand why PMUs outside the core are bound to a CPU anyway,
> all this patch does is attempt to satisfy the assumptions made by
> __perf_install_in_context and ctx_sched_out at init time so that lockdep
> no longer complains.

If you care about the background:

It's necessary because portions of the perf core code rely on
serialization that can only be ensured when all management of the PMU
occurs on the same CPU. e.g. for the per-cpu ringbuffers.

There are also some system/uncore PMUs that exist for groups of CPUs
(e.g. clusters or sockets), but are exposed as a single logical PMU,
assocaited with one CPU per group.

> 
> ctx_sched_out asserts ctx->lock which seems to be taken by
> __perf_install_in_context:
> 
> 	struct perf_event_context *ctx = event->ctx;
> 	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
> 	...
> 	raw_spin_lock(&cpuctx->ctx.lock);
> 
> The lockdep warning happens when ctx != &cpuctx->ctx which can happen if
> __perf_install_in_context is called on a cpu other than event->cpu.
> 
> Found while working on this patch:
> https://patchwork.kernel.org/patch/11056785/
> 
>  kernel/events/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 026a14541a38..0463c1151bae 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11272,11 +11272,11 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
>  	if (!exclusive_event_installable(event, ctx)) {
>  		err = -EBUSY;
>  		goto err_unlock;
>  	}
>  
> -	perf_install_in_context(ctx, event, cpu);
> +	perf_install_in_context(ctx, event, event->cpu);
>  	perf_unpin_context(ctx);
>  	mutex_unlock(&ctx->mutex);
>  
>  	return event;

This matches what we in a regular perf_event_open() syscall, and I
believe this is sane. I think we should also update the comment a few
lines above that refers to @cpu, since that's potentially misleading.
Could we change that from:

  Check if the @cpu we're creating an event for is online.

... to:

  Check if the new event's CPU is online.

With that:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[tip:perf/urgent] perf/core: Fix creating kernel counters for PMUs that override event->cpu

[tip-bot for Leonard Crestez]

Commit-ID:  4ce54af8b33d3e21ca935fc1b89b58cbba956051
Gitweb:     https://git.kernel.org/tip/4ce54af8b33d3e21ca935fc1b89b58cbba956051
Author:     Leonard Crestez <leonard.crestez@nxp.com>
AuthorDate: Wed, 24 Jul 2019 15:53:24 +0300
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Jul 2019 15:41:31 +0200

perf/core: Fix creating kernel counters for PMUs that override event->cpu

Some hardware PMU drivers will override perf_event.cpu inside their
event_init callback. This causes a lockdep splat when initialized through
the kernel API:

 WARNING: CPU: 0 PID: 250 at kernel/events/core.c:2917 ctx_sched_out+0x78/0x208
 pc : ctx_sched_out+0x78/0x208
 Call trace:
  ctx_sched_out+0x78/0x208
  __perf_install_in_context+0x160/0x248
  remote_function+0x58/0x68
  generic_exec_single+0x100/0x180
  smp_call_function_single+0x174/0x1b8
  perf_install_in_context+0x178/0x188
  perf_event_create_kernel_counter+0x118/0x160

Fix this by calling perf_install_in_context with event->cpu, just like
perf_event_open

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Frank Li <Frank.li@nxp.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/r/c4ebe0503623066896d7046def4d6b1e06e0eb2e.1563972056.git.leonard.crestez@nxp.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 026a14541a38..0463c1151bae 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11274,7 +11274,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
 		goto err_unlock;
 	}
 
-	perf_install_in_context(ctx, event, cpu);
+	perf_install_in_context(ctx, event, event->cpu);
 	perf_unpin_context(ctx);
 	mutex_unlock(&ctx->mutex);