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

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 12 May 2017 21:49:56 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> In general we avoid nested locking in the kernel. Nested locking makes
> an absolute mockery of locking rules and what all gets protected.

I'm not against the goal of having get_online_cpus() not be nested,
but I don't agree with the above comment.

If we look at locking in a more abstract view, lock(A) is just
something to protect critical section A. Lock(B) is just something to
protect critical section B. To prevent deadlocks, if one enters
critical section A and then enters critical section B before leaving
critical section A, then the system must never enter critical section B
and then enter critical section A.

But for nested locks, the nested lock is not a new critical section. If
we have lock(A); ...; lock(A); ...; unlock(A); ...; unlock(A); that
just shows a single entry into critical section A. Even if we have:

lock(A);...; lock(B); ...; lock(A); ...; unlock(A); ...;
unlock(B); ...; unlock(A);

As long as there never exits a lock(B); ...; lock(A); without first
taking lock A before taking lock(B).

Because the rules only matter when entering a critical section, and the
taking of the second lock(A) is basically just a nop.

We do this all the time in the kernel, but we just don't do it with
nesting locks. We usually do it with __func()s.


void func(void) {
	lock(A);
	__func();
	unlock(A);
}

and later we could even have:

void foo(void) {
	lock(A);
	...;
	lock(B);
	...;
	__func();
	...;
	unlock(B);
	...;
	unlock(A);
}

If lock(A) was able to nest, then __func() wouldn't be needed. We could
always just call func() which would take lock(A); lockdep will still
work just fine, as it would only care when the lock is first taken, not
its nesting.

One thing we need to do with the current approach is

void __func(void) {

	lockdep_assert_held(A);

	...;
}

to make sure that A is held when calling __func().

Again, I'm not against the lofty goal of having no nesting of
get_online_cpus(), but I just don't agree that nesting locks make a
mockery out of the locking rules.

-- Steve