Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lockacquisition

[Tim Chen]

On Sat, 2013-06-22 at 03:57 -0400, Peter Hurley wrote:
> Will this spin for full scheduler value on a reader-owned lock?
> 
> > +		/* wait_lock will be acquired if write_lock is obtained */
> > +		if (rwsem_try_write_lock(sem->count, true, sem)) {
> > +			ret = 1;
> > +			goto out;
> > +		}
> > +
> > +		/*
> > +		 * When there's no owner, we might have preempted between the
>                                                          ^^^^^^^^
> 
> Isn't pre-emption disabled?
> 

Peter, on further review, this code is needed.  This code guard against 
the case of this thread preempting another thread in the middle
of setting the  owner field.  Disabling preemption does not prevent this
thread from preempting others, even though others cannot preempt 
this thread.


> 
> > +		 * 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();
> > +
> > +	}
> > +
> > +out:
> > +	preempt_enable();
> > +	return ret;
> > +}
> > +#endif
> > +
> >   /*
> >    * wait until we successfully acquire the write lock
> >    */
> > @@ -200,6 +326,9 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)
> >   	long count, adjustment = -RWSEM_ACTIVE_WRITE_BIAS;
> >   	struct rwsem_waiter waiter;
> >   	struct task_struct *tsk = current;
> > +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER
> > +	bool try_optimistic_spin = true;
> > +#endif
> >
> >   	/* set up my own style of waitqueue */
> >   	waiter.task = tsk;
> > @@ -223,20 +352,17 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)
> >   	/* wait until we successfully acquire the lock */
> >   	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> >   	while (true) {
> > -		if (!(count & RWSEM_ACTIVE_MASK)) {
> > -			/* Try acquiring the write lock. */
> > -			count = RWSEM_ACTIVE_WRITE_BIAS;
> > -			if (!list_is_singular(&sem->wait_list))
> > -				count += RWSEM_WAITING_BIAS;
> > -
> > -			if (sem->count == RWSEM_WAITING_BIAS &&
> > -			    cmpxchg(&sem->count, RWSEM_WAITING_BIAS, count) ==
> > -							RWSEM_WAITING_BIAS)
> > -				break;
> > -		}
> > +		if (rwsem_try_write_lock(count, false, sem))
> > +			break;
> >
> >   		raw_spin_unlock_irq(&sem->wait_lock);
> >
> > +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER
> > +		/* do optimistic spinning */
> > +		if (try_optimistic_spin && rwsem_optimistic_spin(sem))
> > +			break;
> > +		try_optimistic_spin = false;
> > +#endif
> >   		/* Block until there are no active lockers. */
> >   		do {
> >   			schedule();
> 

Thanks.

Tim

--
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/