Date: Tue, 29 Aug 2017 19:40:44 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Peter Zijlstra <peterz@infradead.org>
cc: Borislav Petkov <bp@alien8.de>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: WARNING: possible circular locking dependency detected
In-Reply-To: <20170828145808.btuqpe2bvxymljyg@hirez.programming.kicks-ass.net>
Message-ID: <alpine.DEB.2.20.1708291921350.2408@nanos>
References: <20170825100304.5cwrlrfwi7f3zcld@pd.tnic> <20170828145808.btuqpe2bvxymljyg@hirez.programming.kicks-ass.net>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2384
Lines: 79

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