Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721703 > unrolled thread

Re: WARNING: possible circular locking dependency detected

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-28 17:00 +0200
    Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-28 17:10 +0200
      Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-28 18:40 +0200
    Re: WARNING: possible circular locking dependency detected Thomas Gleixner <tglx@linutronix.de> - 2017-08-29 19:50 +0200
      Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-29 21:50 +0200
        Re: WARNING: possible circular locking dependency detected Thomas Gleixner <tglx@linutronix.de> - 2017-08-29 22:20 +0200
          Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-30 07:50 +0200
            Re: WARNING: possible circular locking dependency detected Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:10 +0200
              Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-31 09:40 +0200
                Re: WARNING: possible circular locking dependency detected Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 10:00 +0200
                  Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-31 10:10 +0200
                    Re: WARNING: possible circular locking dependency detected Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 10:20 +0200
                      Re: WARNING: possible circular locking dependency detected Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 23:30 +0200
                        Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-09-01 22:40 +0200

#1721703 — Re: WARNING: possible circular locking dependency detected

On Fri, Aug 25, 2017 at 12:03:04PM +0200, Borislav Petkov wrote:
> Hey,
> 
> tglx says I have something for ya :-)
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.13.0-rc6+ #1 Not tainted
> ------------------------------------------------------
> watchdog/3/27 is trying to acquire lock:
>  (cpu_hotplug_lock.rw_sem){++++}, at: [<ffffffff8100c489>] release_ds_buffers+0x29/0xd0
> 
> but now in release context of a crosslock acquired at the following:
>  ((complete)&self->parked){+.+.}, at: [<ffffffff810895f6>] kthread_park+0x46/0x60


So I'm thinking this one is an actual deadlock.

So, as far as I can tell this ends up being:

CPU0                    CPU1

(smpboot_regiser_percpu_thread_cpumask)

get_online_cpus()
__smpboot_create_thread()
  kthread_park();
    wait_for_completion(&X)


                        (smpboot_thread_fn)

                        ->park() := watchdog_disable()
                          watchdog_nmi_disable()
                            perf_event_release_kernel();
                              put_event()
                                _free_event()
                                  ->destroy() := hw_perf_event_destroy()
                                    x86_release_hardware()
                                      release_ds_buffers()
                                        get_online_cpus()


                        kthread_parkme()
                          complete(&X)



So CPU0 holds cpus_hotplug_lock while wait_for_completion() and CPU1
needs to acquire before complete().

So if, in between, CPU2 does down_write(), things will get unstuck.

What's worse, there's also:

  cpus_write_lock()
    ...
      takedown_cpu()
        smpboot_park_threads()
	  smpboot_park_thread()
	    kthread_park()
	      ->park() := watchdog_disable()
		watchdog_nmi_disable()
		  perf_event_release_kernel();
		    put_event()
		      _free_event()
			->destroy() := hw_perf_event_destroy()
			  x86_release_hardware()
			    release_ds_buffers()
			      get_online_cpus()

which as far as I can tell, spells instant deadlock..

[toc] | [next] | [standalone]

#1721715

On Mon, Aug 28, 2017 at 04:58:08PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 25, 2017 at 12:03:04PM +0200, Borislav Petkov wrote:
> > Hey,
> > 
> > tglx says I have something for ya :-)
> > 
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 4.13.0-rc6+ #1 Not tainted
> > ------------------------------------------------------
> > watchdog/3/27 is trying to acquire lock:
> >  (cpu_hotplug_lock.rw_sem){++++}, at: [<ffffffff8100c489>] release_ds_buffers+0x29/0xd0
> > 
> > but now in release context of a crosslock acquired at the following:
> >  ((complete)&self->parked){+.+.}, at: [<ffffffff810895f6>] kthread_park+0x46/0x60
> 
> 
> So I'm thinking this one is an actual deadlock.
> 
> So, as far as I can tell this ends up being:
> 
> CPU0                    CPU1
> 
> (smpboot_regiser_percpu_thread_cpumask)
> 
> get_online_cpus()
> __smpboot_create_thread()
>   kthread_park();
>     wait_for_completion(&X)
> 
> 
>                         (smpboot_thread_fn)
> 
>                         ->park() := watchdog_disable()
>                           watchdog_nmi_disable()
>                             perf_event_release_kernel();
>                               put_event()
>                                 _free_event()
>                                   ->destroy() := hw_perf_event_destroy()
>                                     x86_release_hardware()
>                                       release_ds_buffers()
>                                         get_online_cpus()
> 
> 
>                         kthread_parkme()
>                           complete(&X)
> 
> 
> 
> So CPU0 holds cpus_hotplug_lock while wait_for_completion() and CPU1
> needs to acquire before complete().
> 
> So if, in between, CPU2 does down_write(), things will get unstuck.
> 
> What's worse, there's also:
> 
>   cpus_write_lock()
>     ...
>       takedown_cpu()
>         smpboot_park_threads()
> 	  smpboot_park_thread()
> 	    kthread_park()
> 	      ->park() := watchdog_disable()
> 		watchdog_nmi_disable()
> 		  perf_event_release_kernel();
> 		    put_event()
> 		      _free_event()
> 			->destroy() := hw_perf_event_destroy()
> 			  x86_release_hardware()
> 			    release_ds_buffers()
> 			      get_online_cpus()
> 
> which as far as I can tell, spells instant deadlock..

Aah, but that latter will never happen.. because each CPU will have a
&pmc_refcount and we can't unplug _all_ CPUs.

So the first one will only ever happen on boot, where we park() the very
first watchdog thread and is a potential deadlock, but won't happen
because nobody is around to do down_write() just yet.

argh!

[toc] | [prev] | [next] | [standalone]

#1721795

On Mon, Aug 28, 2017 at 05:06:17PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 28, 2017 at 04:58:08PM +0200, Peter Zijlstra wrote:
> > On Fri, Aug 25, 2017 at 12:03:04PM +0200, Borislav Petkov wrote:
> > > Hey,
> > > 
> > > tglx says I have something for ya :-)
> > > 
> > > ======================================================
> > > WARNING: possible circular locking dependency detected
> > > 4.13.0-rc6+ #1 Not tainted
> > > ------------------------------------------------------
> > > watchdog/3/27 is trying to acquire lock:
> > >  (cpu_hotplug_lock.rw_sem){++++}, at: [<ffffffff8100c489>] release_ds_buffers+0x29/0xd0
> > > 
> > > but now in release context of a crosslock acquired at the following:
> > >  ((complete)&self->parked){+.+.}, at: [<ffffffff810895f6>] kthread_park+0x46/0x60
> > 
> > 
> > So I'm thinking this one is an actual deadlock.
> > 
> > So, as far as I can tell this ends up being:
> > 
> > CPU0                    CPU1
> > 
> > (smpboot_regiser_percpu_thread_cpumask)
> > 
> > get_online_cpus()
> > __smpboot_create_thread()
> >   kthread_park();
> >     wait_for_completion(&X)
> > 
> > 
> >                         (smpboot_thread_fn)
> > 
> >                         ->park() := watchdog_disable()
> >                           watchdog_nmi_disable()
> >                             perf_event_release_kernel();
> >                               put_event()
> >                                 _free_event()
> >                                   ->destroy() := hw_perf_event_destroy()
> >                                     x86_release_hardware()
> >                                       release_ds_buffers()
> >                                         get_online_cpus()
> > 
> > 
> >                         kthread_parkme()
> >                           complete(&X)
> > 
> > 
> > 
> > So CPU0 holds cpus_hotplug_lock while wait_for_completion() and CPU1
> > needs to acquire before complete().
> > 
> > So if, in between, CPU2 does down_write(), things will get unstuck.
> > 
> > What's worse, there's also:
> > 
> >   cpus_write_lock()
> >     ...
> >       takedown_cpu()
> >         smpboot_park_threads()
> > 	  smpboot_park_thread()
> > 	    kthread_park()
> > 	      ->park() := watchdog_disable()
> > 		watchdog_nmi_disable()
> > 		  perf_event_release_kernel();
> > 		    put_event()
> > 		      _free_event()
> > 			->destroy() := hw_perf_event_destroy()
> > 			  x86_release_hardware()
> > 			    release_ds_buffers()
> > 			      get_online_cpus()
> > 
> > which as far as I can tell, spells instant deadlock..
> 
> Aah, but that latter will never happen.. because each CPU will have a
> &pmc_refcount and we can't unplug _all_ CPUs.
> 
> So the first one will only ever happen on boot, where we park() the very
> first watchdog thread and is a potential deadlock, but won't happen
> because nobody is around to do down_write() just yet.

I suspect however it is possible to interleave:

	sysctl.kernel.nmi_watchdog = 0
	  proc_watchdog_common()

with:

	hot un-plug
	 watchdog_disable()

to tickle that exact problem. Just needs a bit of luck.

[toc] | [prev] | [next] | [standalone]

#1722646

On Mon, 28 Aug 2017, Peter Zijlstra wrote:
> What's worse, there's also:
> 
>   cpus_write_lock()
>     ...
>       takedown_cpu()
>         smpboot_park_threads()
> 	  smpboot_park_thread()
> 	    kthread_park()
> 	      ->park() := watchdog_disable()
> 		watchdog_nmi_disable()
> 		  perf_event_release_kernel();
> 		    put_event()
> 		      _free_event()
> 			->destroy() := hw_perf_event_destroy()
> 			  x86_release_hardware()
> 			    release_ds_buffers()
> 			      get_online_cpus()
> 
> which as far as I can tell, spells instant deadlock..

Yes, it does if the destroyed event has the last reference on pmc_refcount.

All it needs for that is to shutdown the watchdog on CPU0 via the sysctl
cpumask and then offline all other CPUs. Works^Wdeadlocks like a charm.

That's not a new deadlock, it's been there forever. Just now lockdep tells
us that it's a potential deadlock _before_ we actually hit it.

The user space interface one has been there as well before the cpu lock
rework. That one was not covered by lockdep either.

None of this is easy to fixup. I started to tackle the unholy mess in the
watchdog code, but now I'm stuck in yet another circular dependency hell.

One solution I'm looking into right now is to reverse the lock order and
actually make the hotplug code do:

	 watchdog_lock();
	 cpu_write_lock();

	 ....
	 cpu_write_unlock();
	 watchdog_unlock();
	 
and get rid of cpu_read_(un)lock() in the sysctl interface completely. I
know it's ugly, but we have other locks we take in the hotplug path as
well.

That solves that part of the issue, but it does not solve the
release_ds_buffers() problem. Though with the watchdog_lock() mechanism, it
allows me to do:

       ->park() := watchdog_disable()
          perf_event_disable(percpuevt);
	  cleanup_event = percpuevt;
	  percpuevt = NULL;
and then

       watchdog_unlock()
          if (cleanup_event) {
	  	perf_event_release_ebent(cleanup_event);
		cleanup_event = NULL;
	  }
	  mutex_unlock(&watchdog_mutex);

That should do the trick nicely for both user space functions and the cpu
hotplug machinery.

Though it's quite a rewrite of that mess, which is particularly non trivial
because that extra non perf implementation in arch/powerpc which has its
own NMI watchdog thingy wants its calls preserved. But AFAICT so far it
should just work. Famous last words....

Thoughts?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1722774

On Tue, Aug 29, 2017 at 07:40:44PM +0200, Thomas Gleixner wrote:

> One solution I'm looking into right now is to reverse the lock order and
> actually make the hotplug code do:
> 
> 	 watchdog_lock();
> 	 cpu_write_lock();
> 
> 	 ....
> 	 cpu_write_unlock();
> 	 watchdog_unlock();
> 	 
> and get rid of cpu_read_(un)lock() in the sysctl interface completely. I
> know it's ugly, but we have other locks we take in the hotplug path as
> well.

This is to serialize the sysctl against hotplug? I'm not immediately
seeing why watchdog_lock needs to be the outer most lock, is that
because of vfs locks or something?

> That solves that part of the issue, but it does not solve the
> release_ds_buffers() problem. Though with the watchdog_lock() mechanism, it
> allows me to do:
> 
>        ->park() := watchdog_disable()
>           perf_event_disable(percpuevt);
> 	  cleanup_event = percpuevt;
> 	  percpuevt = NULL;
> and then
> 
>        watchdog_unlock()
>           if (cleanup_event) {
> 	  	perf_event_release_ebent(cleanup_event);
> 		cleanup_event = NULL;
> 	  }
> 	  mutex_unlock(&watchdog_mutex);
> 
> That should do the trick nicely for both user space functions and the cpu
> hotplug machinery.
> 
> Though it's quite a rewrite of that mess, which is particularly non trivial
> because that extra non perf implementation in arch/powerpc which has its
> own NMI watchdog thingy wants its calls preserved. But AFAICT so far it
> should just work. Famous last words....
> 
> Thoughts?

So I have a patch _somewhere_ that preserves the event<->cpu relation
across hotplug and disable/enable would be sufficient. If you want I can
try and dig that out and make it work again.

That would avoid having to do the destroy/create cycle of the watchdog
events.

[toc] | [prev] | [next] | [standalone]

#1722814

On Tue, 29 Aug 2017, Peter Zijlstra wrote:
> On Tue, Aug 29, 2017 at 07:40:44PM +0200, Thomas Gleixner wrote:
> 
> > One solution I'm looking into right now is to reverse the lock order and
> > actually make the hotplug code do:
> > 
> > 	 watchdog_lock();
> > 	 cpu_write_lock();
> > 
> > 	 ....
> > 	 cpu_write_unlock();
> > 	 watchdog_unlock();
> > 	 
> > and get rid of cpu_read_(un)lock() in the sysctl interface completely. I
> > know it's ugly, but we have other locks we take in the hotplug path as
> > well.
> 
> This is to serialize the sysctl against hotplug? I'm not immediately
> seeing why watchdog_lock needs to be the outer most lock, is that
> because of vfs locks or something?

Well, the watchdog sysctls serialization today is:

      cpus_read_lock();
      mutex_lock(&watchdog_mutex);
      do_stuff()
	access -> online_cpu_mask

      do_stuff()
        ...
	  ...
	    cpus_read_lock();

So we need

    watchdog_mutex -> cpuhotplug_rwsem

lock order all over the place.

> > Though it's quite a rewrite of that mess, which is particularly non trivial
> > because that extra non perf implementation in arch/powerpc which has its
> > own NMI watchdog thingy wants its calls preserved. But AFAICT so far it
> > should just work. Famous last words....
> > 
> > Thoughts?
> 
> So I have a patch _somewhere_ that preserves the event<->cpu relation
> across hotplug and disable/enable would be sufficient. If you want I can
> try and dig that out and make it work again.
> 
> That would avoid having to do the destroy/create cycle of the watchdog
> events.

Yes, that would solve the x86_release_hw() issue, but still lots of the
other rework is required in one way or the other.

I'm currently trying to avoid that extra lock mess in the cpu hotplug code,
which would just open the door for everybody to add his extra locks there,
so we end up taking a gazillion locks before we can hotplug :)

I think I have an idea how to solve that cleanly, but certainly your offer
of preserving the event - cpu relation accross hotplug would help
tremendously.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1723037

On Tue, Aug 29, 2017 at 10:10:37PM +0200, Thomas Gleixner wrote:
> On Tue, 29 Aug 2017, Peter Zijlstra wrote:
> > So I have a patch _somewhere_ that preserves the event<->cpu relation
> > across hotplug and disable/enable would be sufficient. If you want I can
> > try and dig that out and make it work again.
> > 
> > That would avoid having to do the destroy/create cycle of the watchdog
> > events.
> 
> Yes, that would solve the x86_release_hw() issue, but still lots of the
> other rework is required in one way or the other.
> 
> I'm currently trying to avoid that extra lock mess in the cpu hotplug code,
> which would just open the door for everybody to add his extra locks there,
> so we end up taking a gazillion locks before we can hotplug :)
> 
> I think I have an idea how to solve that cleanly, but certainly your offer
> of preserving the event - cpu relation accross hotplug would help
> tremendously.

I think something like the below ought to work. Compile tested only.

On offline it basically does perf_event_disable() for all CPU context
events, and then adds HOTPLUG_OFFSET (-32) to arrive at: OFF +
HOTPLUG_OFFSET = -33.

That's smaller than ERROR and thus perf_event_enable() no-ops on events
for offline CPUs (maybe we should try and plumb an error return for
IOC_ENABLE).

On online we subtract the HOTPLUG_OFFSET again and the event becomes a
regular OFF, after which perf_event_enable() should work again.

---
 include/linux/perf_event.h |  2 ++
 kernel/events/core.c       | 51 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 9bac4bfa5e1a..7b39ceeb206b 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -497,6 +497,8 @@ enum perf_event_active_state {
 	PERF_EVENT_STATE_OFF		= -1,
 	PERF_EVENT_STATE_INACTIVE	=  0,
 	PERF_EVENT_STATE_ACTIVE		=  1,
+
+	PERF_EVENT_STATE_HOTPLUG_OFFSET	= -32,
 };
 
 struct file;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f77c97477e08..b277c27fd81e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11025,19 +11025,21 @@ void perf_swevent_init_cpu(unsigned int cpu)
 }
 
 #if defined CONFIG_HOTPLUG_CPU || defined CONFIG_KEXEC_CORE
-static void __perf_event_exit_context(void *__info)
+static void __perf_event_exit_cpu(void *__info)
 {
-	struct perf_event_context *ctx = __info;
-	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
+	struct perf_cpu_context *cpuctx = __info;
+	struct perf_event_context *ctx = &cpuctx->ctx;
 	struct perf_event *event;
 
 	raw_spin_lock(&ctx->lock);
-	list_for_each_entry(event, &ctx->event_list, event_entry)
-		__perf_remove_from_context(event, cpuctx, ctx, (void *)DETACH_GROUP);
+	list_for_each_entry(event, &ctx->event_list, event_entry) {
+		__perf_event_disable(event, cpuctx, ctx, NULL);
+		event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+	}
 	raw_spin_unlock(&ctx->lock);
 }
 
-static void perf_event_exit_cpu_context(int cpu)
+int perf_event_exit_cpu(unsigned int cpu)
 {
 	struct perf_cpu_context *cpuctx;
 	struct perf_event_context *ctx;
@@ -11049,17 +11051,43 @@ static void perf_event_exit_cpu_context(int cpu)
 		ctx = &cpuctx->ctx;
 
 		mutex_lock(&ctx->mutex);
-		smp_call_function_single(cpu, __perf_event_exit_context, ctx, 1);
+		smp_call_function_single(cpu, __perf_event_exit_cpu, cpuctx, 1);
 		cpuctx->online = 0;
 		mutex_unlock(&ctx->mutex);
 	}
 	cpumask_clear_cpu(cpu, perf_online_mask);
 	mutex_unlock(&pmus_lock);
+
+	return 0;
+}
+
+static void __perf_event_init_cpu(void *__info)
+{
+	struct perf_cpu_context *cpuctx = __info;
+	struct perf_event_context *ctx = &cpuctx->ctx;
+	struct perf_event *event;
+
+	raw_spin_lock(&ctx->lock);
+	list_for_each_entry(event, &ctx->event_list, event_entry)
+		event->state -= PERF_EVENT_STATE_HOTPLUG_OFFSET;
+	raw_spin_unlock(&ctx->lock);
+}
+
+static void _perf_event_init_cpu(int cpu, struct perf_cpu_context *cpuctx)
+{
+	smp_call_function_single(cpu, __perf_event_init_cpu, cpuctx, 1);
 }
+
 #else
 
-static void perf_event_exit_cpu_context(int cpu) { }
+int perf_event_exit_cpu(unsigned int cpu)
+{
+	return 0;
+}
 
+static void _perf_event_init_cpu(int cpu, struct perf_cpu_context *cpuctx)
+{
+}
 #endif
 
 int perf_event_init_cpu(unsigned int cpu)
@@ -11078,6 +11106,7 @@ int perf_event_init_cpu(unsigned int cpu)
 
 		mutex_lock(&ctx->mutex);
 		cpuctx->online = 1;
+		_perf_event_init_cpu(cpu, cpuctx);
 		mutex_unlock(&ctx->mutex);
 	}
 	mutex_unlock(&pmus_lock);
@@ -11085,12 +11114,6 @@ int perf_event_init_cpu(unsigned int cpu)
 	return 0;
 }
 
-int perf_event_exit_cpu(unsigned int cpu)
-{
-	perf_event_exit_cpu_context(cpu);
-	return 0;
-}
-
 static int
 perf_reboot(struct notifier_block *notifier, unsigned long val, void *v)
 {

[toc] | [prev] | [next] | [standalone]

#1723888

On Wed, 30 Aug 2017, Peter Zijlstra wrote:
> On offline it basically does perf_event_disable() for all CPU context
> events, and then adds HOTPLUG_OFFSET (-32) to arrive at: OFF +
> HOTPLUG_OFFSET = -33.
> 
> That's smaller than ERROR and thus perf_event_enable() no-ops on events
> for offline CPUs (maybe we should try and plumb an error return for
> IOC_ENABLE).
> 
> On online we subtract the HOTPLUG_OFFSET again and the event becomes a
> regular OFF, after which perf_event_enable() should work again.

I haven't come around to test that as I was busy cleaning up the unholy
mess in the watchdog code.

One other thing I stumbled over is:

    perf_event_create()
      ....
      x86_hw_reserve(event)

      if (__x86_pmu_event_init(event) < 0)
      	    event->destroy(event);
	    	x86_hw_release()
		    ....
		    cpus_read_lock();

If that happens from a hotplug function, we are doomed.

I mean, that particular watchdog event won't fail if the watchdog code
would verify that already at init time (which it does soon), but in general
event creation during hotplug is dangerous.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1723917

On Thu, Aug 31, 2017 at 09:08:05AM +0200, Thomas Gleixner wrote:
> On Wed, 30 Aug 2017, Peter Zijlstra wrote:
> > On offline it basically does perf_event_disable() for all CPU context
> > events, and then adds HOTPLUG_OFFSET (-32) to arrive at: OFF +
> > HOTPLUG_OFFSET = -33.
> > 
> > That's smaller than ERROR and thus perf_event_enable() no-ops on events
> > for offline CPUs (maybe we should try and plumb an error return for
> > IOC_ENABLE).
> > 
> > On online we subtract the HOTPLUG_OFFSET again and the event becomes a
> > regular OFF, after which perf_event_enable() should work again.
> 
> I haven't come around to test that as I was busy cleaning up the unholy
> mess in the watchdog code.
> 
> One other thing I stumbled over is:
> 
>     perf_event_create()
>       ....
>       x86_hw_reserve(event)
> 
>       if (__x86_pmu_event_init(event) < 0)
>       	    event->destroy(event);
> 	    	x86_hw_release()
> 		    ....
> 		    cpus_read_lock();
> 
> If that happens from a hotplug function, we are doomed.
> 
> I mean, that particular watchdog event won't fail if the watchdog code
> would verify that already at init time (which it does soon), but in general
> event creation during hotplug is dangerous.

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.

I'll try and think...

[toc] | [prev] | [next] | [standalone]

#1723946

On Thu, 31 Aug 2017, Peter Zijlstra wrote:
> On Thu, Aug 31, 2017 at 09:08:05AM +0200, Thomas Gleixner wrote:
> > On Wed, 30 Aug 2017, Peter Zijlstra wrote:
> > > On offline it basically does perf_event_disable() for all CPU context
> > > events, and then adds HOTPLUG_OFFSET (-32) to arrive at: OFF +
> > > HOTPLUG_OFFSET = -33.
> > > 
> > > That's smaller than ERROR and thus perf_event_enable() no-ops on events
> > > for offline CPUs (maybe we should try and plumb an error return for
> > > IOC_ENABLE).
> > > 
> > > On online we subtract the HOTPLUG_OFFSET again and the event becomes a
> > > regular OFF, after which perf_event_enable() should work again.
> > 
> > I haven't come around to test that as I was busy cleaning up the unholy
> > mess in the watchdog code.
> > 
> > One other thing I stumbled over is:
> > 
> >     perf_event_create()
> >       ....
> >       x86_hw_reserve(event)
> > 
> >       if (__x86_pmu_event_init(event) < 0)
> >       	    event->destroy(event);
> > 	    	x86_hw_release()
> > 		    ....
> > 		    cpus_read_lock();
> > 
> > If that happens from a hotplug function, we are doomed.
> > 
> > I mean, that particular watchdog event won't fail if the watchdog code
> > would verify that already at init time (which it does soon), but in general
> > event creation during hotplug is dangerous.
> 
> 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.

2) Have some deferred mechanism, which is destroying the event after
   failure, but that might be tricky as RCU and workqueues might end up
   being flushed during hotplug, which creates the same mess again.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1723963

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.

[toc] | [prev] | [next] | [standalone]

#1723978

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.....

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1724603

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 ...

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1725335

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;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web