Re: [PATCH 1/2] cpuhotplug: use rw_semaphore for cpu_hotplug

[Gautham R Shenoy <ego@in.ibm.com>]

On Fri, May 29, 2009 at 06:53:42PM -0700, Paul E. McKenney wrote:
> On Fri, May 29, 2009 at 04:29:30PM +0800, Lai Jiangshan wrote:
> > 
> > Current get_online_cpus()/put_online_cpus() re-implement
> > a rw_semaphore, so it is converted to a real rw_semaphore in this fix.
> > It simplifies codes, and is good for read.
> > 
> > And misc fix:
> > 1) Add comments for cpu_hotplug.active_writer.
> > 2) The theoretical disadvantage described in cpu_hotplug_begin()'s
> >    comments is no longer existed when we use rw_semaphore,
> >    so this part of comments was removed.
> > 
> > [Impact: improve get_online_cpus()/put_online_cpus() ]
> 
> Actually, it turns out that for my purposes it is only necessary to check:
> 
> 	cpu_hotplug.active_writer != NULL
> 
> The only time that it is unsafe to invoke get_online_cpus() is when
> in a notifier, and in that case the value of cpu_hotplug.active_writer
> is stable.  There could be false positives, but these are harmless, as
> the fallback is simply synchronize_sched().
> 
> Even this is only needed should the deadlock scenario you pointed out
> arise in practice.
> 
> As Oleg noted, there are some "interesting" constraints on
> get_online_cpus().  Adding Gautham Shenoy to CC for his views.

So, to put it in a sentence, get_online_cpus()/put_online_cpus() is a
read-write semaphore with read-preference while allowing writer to
downgrade to a reader when required.

Read-preference was one of the ways of allowing unsuspecting functions
which need the protection against cpu-hotplug to end up seeking help of
functions which also need protection against cpu-hotplug. IOW allow a
single context to call get_online_cpus() without giving away to circular
deadlock. A fair reader-write lock wouldn't allow that since in the
presence of a write, the recursive reads would block, thereby causing a
deadlock.

Also, around the time when this design was chosen, we had a whole bunch
of functions which did try to take the original "cpu_hotplug_mutex"
recursively. We could do well to use Lai's implementation if such
functions have mended their ways since this would make it a lot simpler
:-) . But I suspect it is easier said than done!

BTW, I second the idea of try_get_online_cpus(). I had myself proposed
this idea a year back. http://lkml.org/lkml/2008/4/29/222.



> 
> 							Thanx, Paul
> 
> > Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> > ---
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index 395b697..62198ec 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/kthread.h>
> >  #include <linux/stop_machine.h>
> >  #include <linux/mutex.h>
> > +#include <linux/rwsem.h>
> > 
> >  #ifdef CONFIG_SMP
> >  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> > @@ -27,20 +28,21 @@ static __cpuinitdata RAW_NOTIFIER_HEAD(cpu_chain);
> >  static int cpu_hotplug_disabled;
> > 
> >  static struct {
> > -	struct task_struct *active_writer;
> > -	struct mutex lock; /* Synchronizes accesses to refcount, */
> >  	/*
> > -	 * Also blocks the new readers during
> > -	 * an ongoing cpu hotplug operation.
> > +	 * active_writer makes get_online_cpus()/put_online_cpus() are allowd
> > +	 * to be nested in cpu_hotplug_begin()/cpu_hotplug_done().
> > +	 *
> > +	 * Thus, get_online_cpus()/put_online_cpus() can be called in
> > +	 * CPU notifiers.
> >  	 */
> > -	int refcount;
> > +	struct task_struct *active_writer;
> > +	struct rw_semaphore rwlock;
> >  } cpu_hotplug;
> > 
> >  void __init cpu_hotplug_init(void)
> >  {
> >  	cpu_hotplug.active_writer = NULL;
> > -	mutex_init(&cpu_hotplug.lock);
> > -	cpu_hotplug.refcount = 0;
> > +	init_rwsem(&cpu_hotplug.rwlock);
> >  }
> > 
> >  #ifdef CONFIG_HOTPLUG_CPU
> > @@ -50,9 +52,7 @@ void get_online_cpus(void)
> >  	might_sleep();
> >  	if (cpu_hotplug.active_writer == current)
> >  		return;
> > -	mutex_lock(&cpu_hotplug.lock);
> > -	cpu_hotplug.refcount++;
> > -	mutex_unlock(&cpu_hotplug.lock);
> > +	down_read(&cpu_hotplug.rwlock);
> > 
> >  }
> >  EXPORT_SYMBOL_GPL(get_online_cpus);
> > @@ -61,10 +61,7 @@ void put_online_cpus(void)
> >  {
> >  	if (cpu_hotplug.active_writer == current)
> >  		return;
> > -	mutex_lock(&cpu_hotplug.lock);
> > -	if (!--cpu_hotplug.refcount && unlikely(cpu_hotplug.active_writer))
> > -		wake_up_process(cpu_hotplug.active_writer);
> > -	mutex_unlock(&cpu_hotplug.lock);
> > +	up_read(&cpu_hotplug.rwlock);
> > 
> >  }
> >  EXPORT_SYMBOL_GPL(put_online_cpus);
> > @@ -86,45 +83,25 @@ void cpu_maps_update_done(void)
> >  }
> > 
> >  /*
> > - * This ensures that the hotplug operation can begin only when the
> > - * refcount goes to zero.
> > + * This ensures that the hotplug operation can begin only when
> > + * there is no reader.
> >   *
> >   * Note that during a cpu-hotplug operation, the new readers, if any,
> > - * will be blocked by the cpu_hotplug.lock
> > + * will be blocked by the cpu_hotplug.rwlock
> >   *
> >   * Since cpu_hotplug_begin() is always called after invoking
> >   * cpu_maps_update_begin(), we can be sure that only one writer is active.
> > - *
> > - * Note that theoretically, there is a possibility of a livelock:
> > - * - Refcount goes to zero, last reader wakes up the sleeping
> > - *   writer.
> > - * - Last reader unlocks the cpu_hotplug.lock.
> > - * - A new reader arrives at this moment, bumps up the refcount.
> > - * - The writer acquires the cpu_hotplug.lock finds the refcount
> > - *   non zero and goes to sleep again.
> > - *
> > - * However, this is very difficult to achieve in practice since
> > - * get_online_cpus() not an api which is called all that often.
> > - *
> >   */
> >  static void cpu_hotplug_begin(void)
> >  {
> > +	down_write(&cpu_hotplug.rwlock);
> >  	cpu_hotplug.active_writer = current;
> > -
> > -	for (;;) {
> > -		mutex_lock(&cpu_hotplug.lock);
> > -		if (likely(!cpu_hotplug.refcount))
> > -			break;
> > -		__set_current_state(TASK_UNINTERRUPTIBLE);
> > -		mutex_unlock(&cpu_hotplug.lock);
> > -		schedule();
> > -	}
> >  }
> > 
> >  static void cpu_hotplug_done(void)
> >  {
> >  	cpu_hotplug.active_writer = NULL;
> > -	mutex_unlock(&cpu_hotplug.lock);
> > +	up_write(&cpu_hotplug.rwlock);
> >  }
> >  /* Need to know about CPUs going up/down? */
> >  int __ref register_cpu_notifier(struct notifier_block *nb)
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Thanks and Regards
gautham