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

[Paul E. McKenney]

On Wed, May 17, 2017 at 07:55:20AM -0700, Paul E. McKenney wrote:
> On Wed, May 17, 2017 at 12:40:10PM +0200, Peter Zijlstra wrote:
> > On Tue, May 16, 2017 at 07:27:42AM -0700, Paul E. McKenney wrote:
> > > On Tue, May 16, 2017 at 05:46:06AM -0700, Paul E. McKenney wrote:

[ . . . ]

> > 
> > With the above change any chance of a race is gone and we don't need to
> > worry about hotplug ordering too much.
> 
> Nice!!!
> 
> > Now I just need to do something about the swevent hash, because that's
> > got a hole in now.
> 
> On the styles, here is a more coherence list:
> 
> 1.	Use get_online_cpus(), but no CPU-hotplug notifiers.  You can
> 	use cpu_online() and cpu_is_offline() straightforwardly while
> 	get_online_cpus() is in effect, but these two may only be used
> 	for statistical/heuristic purposes otherwise.

And I forgot about the disable-preemption trick...

Of course you can also use cpu_online() and cpu_is_offline() more or less
straightforwardly when preemption is disabled, but if your code is making
use of subsystems that disable themselves early on outgoing CPUs or enable
themselves late on incoming CPUs, life can be difficult.  These subsystems
can make things easier, for example, smp_call_function_single() will
give a failure indication if it cannot help you because the CPU isn't
all there.  (RCU does OK, but could use a bit more improvement.)

Also, you can use the disable-preemption trick to prevent a CPU from
leaving (if you get there early enough in the offlining), but it won't
necessarily prevent a CPU from showing up.  Yes, there seem to be a
fair number of notifiers that wait for grace periods (perhaps too many),
but you cannot necessarily count on them being configured into the
kernel.

> 2a.	Use CPU-hotplug notifiers, but not get_online_cpus().
> 	The notifiers maintain some sort of bitmask tracking which CPUs
> 	are present from the viewpoint of this subsystem.  This bitmask
> 	provides exact CPU presence/absence indications, at least
> 	assuming the appropriate lock is held.	The cpu_online() and
> 	cpu_is_offline() macros should be avoided, except possibly for
> 	statistical/heuristic purposes.
> 
> 	For your perf patch, the bitmask is a cpumask_var_t.  For RCU,
> 	it is the combination of the ->qsmaskinitnext fields of the leaf
> 	rcu_node structures.
> 
> 2b.	Like 2a, except that instead of a bitmask, the CPU online/offline
> 	information is implicit in other data structures.  For example,
> 	per-CPU structures might be allocated only when the corresponding
> 	CPU is online, so that a given per-CPU pointer is non-NULL
> 	iff the corresponding CPU is online.

Here, disabling preemption will still prevent any CPU from responding
to the stop-CPUs mechanism, but it cannot prevent your own notifier from
executing.  So care is required using cpu_online() and cpu_is_offline()
even when preemption is disabled.

							Thanx, Paul

> So I was (confusingly) initially using "style" to distinguish #1 on the
> one hand from #2a and #2b on the other.  Later on, I was using "style"
> to distinguish #2a from #2b.
> 
> At this point, I am not sure that it makes all that much sense to
> distinguish 2a from 2b.  Or you could argue that use of a cpumask_var_t
> is its own substyle, with hand-crafted bitmasks (such as RCU's) are
> separate substyles.  Can't say that I care all that much about that
> fine-grained gradiations.  ;-)
> 
> 							Thanx, Paul