Re: Performance regression from switching lock to rw-sem foranon-vma tree

[Tim Chen]

On Mon, 2013-06-17 at 12:05 -0700, Davidlohr Bueso wrote:

> > 
> > Thanks.  Those are encouraging numbers.  On my exim workload I didn't
> > get a boost when I added in the preempt disable in optimistic spin and
> > put Alex's changes in. Can you send me your combined patch to see if
> > there may be something you did that I've missed.  I have a tweak to
> > Alex's patch below to simplify things a bit.  
> > 
> 
> I'm using:
> 
> int rwsem_optimistic_spin(struct rw_semaphore *sem)
> {
>         struct  task_struct     *owner;
> 
>         /* sem->wait_lock should not be held when attempting optimistic spinning */
>         if (!rwsem_can_spin_on_owner(sem))
>                 return 0;
> 
>         preempt_disable();
>         for (;;) {
>                 owner = ACCESS_ONCE(sem->owner);
>                 if (owner && !rwsem_spin_on_owner(sem, owner))
>                         break;
> 
>                 /* wait_lock will be acquired if write_lock is obtained */
>                 if (rwsem_try_write_lock(sem->count, true, sem)) {
>                         preempt_enable();
>                         return 1;
>                 }
> 
>                 /*                                                                                                                                                                   
>                  * When there's no owner, we might have preempted between the                                                                                                        
>                  * owner acquiring the lock and setting the owner field. If                                                                                                          
>                  * we're an RT task that will live-lock because we won't let                                                                                                         
>                  * the owner complete.                                                                                                                                               
>                  */
>                 if (!owner && (need_resched() || rt_task(current)))
>                         break;
> 
>                 /*                                                                                                                                                                   
>                  * The cpu_relax() call is a compiler barrier which forces                                                                                                           
>                  * everything in this loop to be re-loaded. We don't need                                                                                                            
>                  * memory barriers as we'll eventually observe the right                                                                                                             
>                  * values at the cost of a few extra spins.                                                                                                                          
>                  */
>                 arch_mutex_cpu_relax();
> 
>         }
> 
>         preempt_enable();
>         return 0;
> }

This is identical to the changes that I've tested.  Thanks for sharing.

Tim

> > > > @@ -85,15 +85,28 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type)
> > > >  	adjustment = 0;
> > > >  	if (wake_type != RWSEM_WAKE_READ_OWNED) {
> > > >  		adjustment = RWSEM_ACTIVE_READ_BIAS;
> > > > - try_reader_grant:
> > > > -		oldcount = rwsem_atomic_update(adjustment, sem) - adjustment;
> > > > -		if (unlikely(oldcount < RWSEM_WAITING_BIAS)) {
> > > > -			/* A writer stole the lock. Undo our reader grant. */
> > > > +		while (1) {
> > > > +			long oldcount;
> > > > +
> > > > +			/* A writer stole the lock. */
> > > > +			if (unlikely(sem->count & RWSEM_ACTIVE_MASK))
> > > > +				return sem;
> > > > +
> > > > +			if (unlikely(sem->count < RWSEM_WAITING_BIAS)) {
> > > > +				cpu_relax();
> > > > +				continue;
> > > > +			}
> > 
> > The above two if statements could be cleaned up as a single check:
> > 		
> > 			if (unlikely(sem->count < RWSEM_WAITING_BIAS))
> > 				return sem;
> > 	 
> > This one statement is sufficient to check that we don't have a writer
> > stolen the lock before we attempt to acquire the read lock by modifying
> > sem->count.  
> 
> We probably still want to keep the cpu relaxation if the statement
> doesn't comply.
> 
> Thanks,
> Davidlohr
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/