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.
