On Tue, 2011-12-20 at 12:58 +0530, Srivatsa S. Bhat wrote:

On 12/20/2011 11:57 AM, Al Viro wrote:

quoted

On Tue, Dec 20, 2011 at 10:26:05AM +0530, Srivatsa S. Bhat wrote:

quoted

Oh, right, that has to be handled as well...

Hmmm... How about registering a CPU hotplug notifier callback during lock init
time, and then for every cpu that gets onlined (after we took a copy of the
cpu_online_mask to work with), we see if that cpu is different from the ones
we have already locked, and if it is, we lock it in the callback handler and
update the locked_cpu_mask appropriately (so that we release the locks properly
during the unlock operation).

Handling the newly introduced race between the callback handler and lock-unlock
code must not be difficult, I believe..

Any loopholes in this approach? Or is the additional complexity just not worth
it here?

To summarize the modified variant of that approach hashed out on IRC:

	* lglock grows three extra things: spinlock, cpu bitmap and cpu hotplug
notifier.
	* foo_global_lock_online starts with grabbing that spinlock and
loops over the cpus in that bitmap.
	* foo_global_unlock_online loops over the same bitmap and then drops
that spinlock
	* callback of the notifier is going to do all bitmap updates.  Under
that spinlock.  Events that need handling definitely include the things like
"was going up but failed", since we need the bitmap to contain all online CPUs
at all time, preferably without too much junk beyond that.  IOW, we need to add
it there _before_ low-level __cpu_up() calls set_cpu_online().  Which means
that we want to clean up on failed attempt to up it.  Taking a CPU down is
probably less PITA; just clear bit on the final "the sucker's dead" event.
	* bitmap is initialized once, at the same time we set the notifier
up.  Just grab the spinlock and do
	for_each_online_cpu(N)
		add N to bitmap
then release the spinlock and let the callbacks handle all updates.

I think that'll work with relatively little pain, but I'm not familiar enough
with the cpuhotplug notifiers, so I'd rather have the folks familiar with those
to supply the set of events to watch for...

We need not watch out for "up failed" events. It is enough if we handle
CPU_ONLINE and CPU_DEAD events. Because, these 2 events are triggered only
upon successful online or offline operation, and these notifications are
more than enough for our purpose (to update our bitmaps). Also, those cpus
which came online wont start running until these "success notifications"
are all done, which is where we do our stuff in the callback (ie., try
grabbing the spinlock..).

Of course, by doing this (only looking out for CPU_ONLINE and CPU_DEAD
events), our bitmap will probably be one step behind cpu_online_mask
(which means, we'll still have to take the snapshot of cpu_online_mask and
work with it instead of using for_each_online_cpu()).
But that doesn't matter, as long as:
  * we don't allow the newly onlined CPU to start executing code (this
    is achieved by taking the spinlock in the callback)

I think cpu notifier callback doesn't always run on the UPing cpu.
Actually, it rarely runs on the UPing cpu.
If I was wrong about the above thought, there is still a chance that lg-lock
operations are scheduled on the UPing cpu before calling the callback.

  * we stick to our bitmap while taking and releasing the spinlocks.

Both of these have been handled in the design proposed above. So we are good
to go I guess.

I am working on translating all these to working code.. Will post the patch
as soon as I'm done.

Regards,
Srivatsa S. Bhat