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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>