Re: [RFC][PATCH 0/5] perf/tracing/cpuhotplug: Fix locking order

[Steven Rostedt]

On Fri, 12 May 2017 21:49:56 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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