Re: WARNING: possible circular locking dependency detected

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Aug 31, 2017 at 11:24:13PM +0200, Thomas Gleixner wrote:
> On Thu, 31 Aug 2017, Thomas Gleixner wrote:
> > On Thu, 31 Aug 2017, Peter Zijlstra wrote:
> > 
> > > On Thu, Aug 31, 2017 at 09:55:57AM +0200, Thomas Gleixner wrote:
> > > > > Arghh!!!
> > > > > 
> > > > > And allowing us to create events for offline CPUs (possible I think, but
> > > > > maybe slightly tricky) won't solve that, because we're already holding
> > > > > the hotplug_lock during PREPARE.
> > > > 
> > > > There are two ways to cure that:
> > > > 
> > > > 1) Have a pre cpus_write_lock() stage which is serialized via
> > > >    cpus_add_remove_lock, which is the outer lock for hotplug.
> > > > 
> > > >    There we can sanely create stuff and fail with all consequences.
> > > 
> > > True, if you're willing to add more state to that hotplug thing I'll try
> > > and make that perf patch that allows attaching to offline CPUs.
> > 
> > Now that I think more about it. That's going to be an interesting exercise
> > vs. the hotplug state registration which relies on cpus_read_lock()
> > serialization.....
> 
> We could have that for built-in stuff which is guaranteed to be never
> unregistered. Pretty restricted, but for cases like that it could
> work. Famous last work ...

I think something like this (on top of the previous patch that preserves
the event<->cpu relation over hotplug) should allow
perf_event_create_kernel_counter() to create events on offline CPUs.

It will create them as if perf_event_attr::disabled=1 and requires
perf_event_enable() after the CPU comes online (mirroring how offline
does an implicit perf_event_disable() as per the previous patch).


--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2427,6 +2427,24 @@ perf_install_in_context(struct perf_even
 	smp_store_release(&event->ctx, ctx);
 
 	if (!task) {
+		/*
+		 * Check if the @cpu we're creating an event for is offline.
+		 *
+		 * We use the perf_cpu_context::ctx::mutex to serialize against
+		 * the hotplug notifiers. See perf_event_{init,exit}_cpu().
+		 */
+		struct perf_cpu_context *cpuctx =
+			container_of(ctx, struct perf_cpu_context, ctx);
+
+		if (!cpuctx->online) {
+			raw_spin_lock_irq(&ctx->lock);
+			add_event_to_context(event, ctx);
+			event->state = PERF_EVENT_STATE_OFF +
+				       PERF_EVENT_STATE_HOTPLUG_OFFSET;
+			raw_spin_unlock_irq(&ctx->lock);
+			return;
+		}
+
 		cpu_function_call(cpu, __perf_install_in_context, event);
 		return;
 	}
@@ -10181,6 +10199,8 @@ SYSCALL_DEFINE5(perf_event_open,
 		 *
 		 * We use the perf_cpu_context::ctx::mutex to serialize against
 		 * the hotplug notifiers. See perf_event_{init,exit}_cpu().
+		 *
+		 * XXX not strictly required, preserves existing behaviour.
 		 */
 		struct perf_cpu_context *cpuctx =
 			container_of(ctx, struct perf_cpu_context, ctx);
@@ -10368,21 +10388,6 @@ perf_event_create_kernel_counter(struct
 		goto err_unlock;
 	}
 
-	if (!task) {
-		/*
-		 * Check if the @cpu we're creating an event for is online.
-		 *
-		 * We use the perf_cpu_context::ctx::mutex to serialize against
-		 * the hotplug notifiers. See perf_event_{init,exit}_cpu().
-		 */
-		struct perf_cpu_context *cpuctx =
-			container_of(ctx, struct perf_cpu_context, ctx);
-		if (!cpuctx->online) {
-			err = -ENODEV;
-			goto err_unlock;
-		}
-	}
-
 	if (!exclusive_event_installable(event, ctx)) {
 		err = -EBUSY;
 		goto err_unlock;