Re: [PATCH] locking/rwsem: Add reader owned state to the owner field

[Jason Low <jason.low2@hp.com>]

On Wed, 2016-05-04 at 13:27 -0400, Waiman Long wrote:
> On 05/03/2016 08:21 PM, Davidlohr Bueso wrote:
> > On Wed, 27 Apr 2016, Waiman Long wrote:
> >> static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
> >> @@ -378,7 +367,8 @@ static bool rwsem_optimistic_spin(struct 
> >> rw_semaphore *sem)
> >>
> >>     while (true) {
> >>         owner = READ_ONCE(sem->owner);
> >> -        if (owner && !rwsem_spin_on_owner(sem, owner))
> >> +        if (rwsem_is_writer_owned(owner) &&
> >> +           !rwsem_spin_on_owner(sem, owner))
> >>             break;
> >>
> >>         /* wait_lock will be acquired if write_lock is obtained */
> >> @@ -391,9 +381,11 @@ static bool rwsem_optimistic_spin(struct 
> >> rw_semaphore *sem)
> >>          * 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.
> >> +         * the owner complete. We also quit if the lock is owned by
> >> +         * readers.
> >>          */
> >> -        if (!owner && (need_resched() || rt_task(current)))
> >> +        if ((owner == RWSEM_READER_OWNED) ||

It would be good to provide and use a rwsem_is_reader_owned() function
like we do with rwsem_is_writer_owned(), especially if we're going to
add in the additional cast.

> >> #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
> >> static inline void rwsem_set_owner(struct rw_semaphore *sem)
> >> {
> >> @@ -9,6 +26,16 @@ static inline void rwsem_clear_owner(struct 
> >> rw_semaphore *sem)
> >>     sem->owner = NULL;
> >> }
> >>
> >> +static inline void rwsem_reader_owned(struct rw_semaphore *sem)
> >
> > Nits: rwsem_set_reader_owner()?
> 
> How about rwsem_set_reder_owned()? reader_owner kind of looks weird to me.

I agree that rwsem_set_reader_owned() is a better name than
rwsem_set_reader_owner() since that matches the RWSEM_READER_OWNED
naming convention.

> >
> >> +{
> >> +    if (sem->owner != RWSEM_READER_OWNED)
> >> +        sem->owner = RWSEM_READER_OWNED;
> >
> > ... and just blindly setting it ough to be fine.
> 
> I was trying to minimize the number of writes to the rwsem cacheline so 
> as to reduce the amount of cacheline contention. I am not sure if the 
> CPU will be smart enough to discard the write if the cacheline has 
> already contained the value to be written.

It would be good to keep that check to avoid unnecessary cacheline
contention if sem->owner is already set to RWSEM_READER_OWNED.