From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758415AbdELUOf (ORCPT ); Fri, 12 May 2017 16:14:35 -0400 Received: from mail.kernel.org ([198.145.29.99]:47488 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753791AbdELUOe (ORCPT ); Fri, 12 May 2017 16:14:34 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8A53823986 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=goodmis.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=rostedt@goodmis.org Date: Fri, 12 May 2017 16:14:30 -0400 From: Steven Rostedt To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, Thomas Gleixner , Ingo Molnar , Mathieu Desnoyers , "Paul E. McKenney" , Masami Hiramatsu Subject: Re: [RFC][PATCH 0/5] perf/tracing/cpuhotplug: Fix locking order Message-ID: <20170512161430.4a4fa278@gandalf.local.home> In-Reply-To: <20170512194956.GH4626@worktop.programming.kicks-ass.net> References: <20170512171544.100715273@goodmis.org> <20170512194956.GH4626@worktop.programming.kicks-ass.net> X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 12 May 2017 21:49:56 +0200 Peter Zijlstra wrote: > On Fri, May 12, 2017 at 01:15:44PM -0400, Steven Rostedt wrote: > > 2) Allow for get_online_cpus() to nest > > So Thomas and me have been avoiding doing this. > > In general we avoid nested locking in the kernel. Nested locking makes > an absolute mockery of locking rules and what all gets protected. > > Yes, its much easier.. But we managed to kill the BKL, so surely we can > fix the hotplug lock too, right ;-) Well, is it really a lock in that sense? Or more like a preempt_disable()? Which, one can argue is a BKL in its own right. get_online_cpus() is more like a preempt_disable() than a lock, as it is preventing something from happening and not really protecting data. preempt_disable() prevents a schedule from happening. get_online_cpus() prevents CPUs from going offline or coming online. Can you image the mess it would be if we prevent preempt_disable() from nesting? get_online_cpus() is similar, but maybe not so horrific. The problem I see with going the route of not letting get_online_cpus() from nesting, is that we are going to have to start encapsulating large areas where get_online_cpus() must be taken. Any time a low level function needs to take get_online_cpus() and there happens to be a higher level function that has a lock that also must have get_online_cpus() held, that calls that lower level function (take tracepoints_mutex for example), that means we need to remove the get_online_cpus() from the lower level function, and make it a requirement to be taken before calling that lower level function everywhere. It moves the get_online_cpus() away from what really needs to have protection, and makes it more into a global lock like the BKL. Look at all the places that needed get_online_cpus() in your patches where the function itself really didn't care about cpus going on or off line. -- Steve