Re: [PATCH 2/3 RFC paul/rcu/srcu] srcu: only increase the upper bit for srcu_read_lock()

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

On Wed, Feb 22, 2012 at 01:20:56PM -0800, Paul E. McKenney wrote:
> On Wed, Feb 22, 2012 at 05:29:32PM +0800, Lai Jiangshan wrote:
> > >From de49bb517e6367776e2226b931346ab6c798b122 Mon Sep 17 00:00:00 2001
> > From: Lai Jiangshan <laijs@cn.fujitsu.com>
> > Date: Wed, 22 Feb 2012 10:41:59 +0800
> > Subject: [PATCH 2/3 RFC paul/rcu/srcu] srcu: only increase the upper bit for srcu_read_lock()
> > 
> > The algorithm/smp_mb()s ensure 'there is only one srcu_read_lock()
> > between flip and recheck for a cpu'.
> > Increment of the upper bit for srcu_read_lock() only can
> > ensure a single pair of lock/unlock change the cpu counter.
> 
> Very nice!  Also makes is more clear in that no combination of operations
> including exactly one increment can possibly wrap back to the same value,
> because the upper bit would be different.

Make that without underflow -- one increment and 2^31+1 decrements would
in fact return the counter to its original value, but that would require
cramming more than two billion tasks into a 32-bit address space, which
I believe to be sufficiently unlikely.  (Famous last words...)

							Thanx, Paul

> In deference to Peter Zijlstra's sensibilities, I changed the:
> 
> 	ACCESS_ONCE(this_cpu_ptr(sp->per_cpu_ref)->c[idx]) += -1;
> 
> to:
> 
> 	ACCESS_ONCE(this_cpu_ptr(sp->per_cpu_ref)->c[idx]) -= 1;
> 
> I did manage to resist the temptation to instead say:
> 
> 	ACCESS_ONCE(this_cpu_ptr(sp->per_cpu_ref)->c[idx]) -= +1;
> 
> ;-)
> 
> Queued, thank you!
> 
> 							Thanx, Paul
> 
> > Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> > ---
> >  include/linux/srcu.h |    2 +-
> >  kernel/srcu.c        |   11 +++++------
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index a478c8e..5b49d41 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -35,7 +35,7 @@ struct srcu_struct_array {
> >  };
> > 
> >  /* Bit definitions for field ->c above and ->snap below. */
> > -#define SRCU_USAGE_BITS		2
> > +#define SRCU_USAGE_BITS		1
> >  #define SRCU_REF_MASK		(ULONG_MAX >> SRCU_USAGE_BITS)
> >  #define SRCU_USAGE_COUNT	(SRCU_REF_MASK + 1)
> > 
> > diff --git a/kernel/srcu.c b/kernel/srcu.c
> > index 17e95bc..a51ac48 100644
> > --- a/kernel/srcu.c
> > +++ b/kernel/srcu.c
> > @@ -138,10 +138,10 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *sp, int idx)
> > 
> >  	/*
> >  	 * Now, we check the ->snap array that srcu_readers_active_idx()
> > -	 * filled in from the per-CPU counter values.  Since both
> > -	 * __srcu_read_lock() and __srcu_read_unlock() increment the
> > -	 * upper bits of the per-CPU counter, an increment/decrement
> > -	 * pair will change the value of the counter.  Since there is
> > +	 * filled in from the per-CPU counter values. Since
> > +	 * __srcu_read_lock() increments the upper bits of
> > +	 * the per-CPU counter, an increment/decrement pair will
> > +	 * change the value of the counter.  Since there is
> >  	 * only one possible increment, the only way to wrap the counter
> >  	 * is to have a huge number of counter decrements, which requires
> >  	 * a huge number of tasks and huge SRCU read-side critical-section
> > @@ -234,8 +234,7 @@ void __srcu_read_unlock(struct srcu_struct *sp, int idx)
> >  {
> >  	preempt_disable();
> >  	smp_mb(); /* C */  /* Avoid leaking the critical section. */
> > -	ACCESS_ONCE(this_cpu_ptr(sp->per_cpu_ref)->c[idx]) +=
> > -		SRCU_USAGE_COUNT - 1;
> > +	ACCESS_ONCE(this_cpu_ptr(sp->per_cpu_ref)->c[idx]) += -1;
> >  	preempt_enable();
> >  }
> >  EXPORT_SYMBOL_GPL(__srcu_read_unlock);
> > -- 
> > 1.7.4.4
> >