Date: Tue, 29 Aug 2017 22:10:37 +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: <20170829194948.GD32112@worktop.programming.kicks-ass.net>
Message-ID: <alpine.DEB.2.20.1708292155360.2696@nanos>
References: <20170825100304.5cwrlrfwi7f3zcld@pd.tnic> <20170828145808.btuqpe2bvxymljyg@hirez.programming.kicks-ass.net> <alpine.DEB.2.20.1708291921350.2408@nanos> <20170829194948.GD32112@worktop.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: 2087
Lines: 68

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