Re: [PATCH RFC tip/core/rcu 11/12] rcu: fix race condition in synchronize_sched_expedited()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Wed, Nov 10, 2010 at 04:56:32PM +0800, Lai Jiangshan wrote:
> On 11/09/2010 09:26 PM, Tejun Heo wrote:
> > Hello, Paul.
> > 
> > 
> > How about something like the following?  It's slightly bigger but I
> > think it's a bit easier to understand.  Thanks.
> 
> Hello, Paul, Tejun,
> 
> I think this approach is good and much better when several tasks
> call synchronize_sched_expedited() at the same time.

I am becoming more comfortable with it as well.  Tejun, what kind of
testing did you do?  Lai, could you please run it on your systems?

							Thanx, Paul

> Acked-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> 
> > 
> > diff --git a/kernel/sched.c b/kernel/sched.c
> > index aa14a56..0069be5 100644
> > --- a/kernel/sched.c
> > +++ b/kernel/sched.c
> > @@ -9342,7 +9342,8 @@ EXPORT_SYMBOL_GPL(synchronize_sched_expedited);
> > 
> >  #else /* #ifndef CONFIG_SMP */
> > 
> > -static atomic_t synchronize_sched_expedited_count = ATOMIC_INIT(0);
> > +static atomic_t sync_sched_expedited_token = ATOMIC_INIT(0);
> > +static atomic_t sync_sched_expedited_done = ATOMIC_INIT(0);
> > 
> >  static int synchronize_sched_expedited_cpu_stop(void *data)
> >  {
> > @@ -9373,11 +9374,18 @@ static int synchronize_sched_expedited_cpu_stop(void *data)
> >   */
> >  void synchronize_sched_expedited(void)
> >  {
> > -	int snap, trycount = 0;
> > +	int my_tok, tok, t, trycount = 0;
> > +
> > +	smp_mb();  /* ensure prior mod happens before getting token. */
> > +
> > +	/*
> > +	 * Get a token.  This is used to coordinate with other
> > +	 * concurrent syncers and consolidate multiple syncs.
> > +	 */
> > +	my_tok = tok = atomic_inc_return(&sync_sched_expedited_token);
> > 
> > -	smp_mb();  /* ensure prior mod happens before capturing snap. */
> > -	snap = atomic_read(&synchronize_sched_expedited_count) + 1;
> >  	get_online_cpus();
> > +
> >  	while (try_stop_cpus(cpu_online_mask,
> >  			     synchronize_sched_expedited_cpu_stop,
> >  			     NULL) == -EAGAIN) {
> > @@ -9388,13 +9396,34 @@ void synchronize_sched_expedited(void)
> >  			synchronize_sched();
> >  			return;
> >  		}
> > -		if (atomic_read(&synchronize_sched_expedited_count) - snap > 0) {
> > +
> > +		/*
> > +		 * If the done count reached @my_tok, we know at least
> > +		 * one synchronization happened since we entered this
> > +		 * function.
> > +		 */
> > +		if (atomic_read(&sync_sched_expedited_done) - my_tok >= 0) {
> >  			smp_mb(); /* ensure test happens before caller kfree */
> >  			return;
> >  		}
> > +
> >  		get_online_cpus();
> > +
> > +		/* about to retry, get the latest token value */
> > +		tok = atomic_read(&sync_sched_expedited_token);
> >  	}
> > -	atomic_inc(&synchronize_sched_expedited_count);
> > +
> > +	/*
> > +	 * We now know that everything upto @tok is synchronized.
> > +	 * Update done counter which should always monotonically
> > +	 * increase (with wrapping considered).
> > +	 */
> > +	do {
> > +		t = atomic_read(&sync_sched_expedited_done);
> > +		if (t - tok >= 0)
> > +			break;
> > +	} while (atomic_cmpxchg(&sync_sched_expedited_done, t, tok) != t);
> > +
> >  	smp_mb__after_atomic_inc(); /* ensure post-GP actions seen after GP. */
> >  	put_online_cpus();
> >  }
> > 
>