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

[Waiman Long]

On 05/11/2016 06:04 PM, Peter Hurley wrote:
>   	/* Grant an infinite number of read locks to the readers at the front
> @@ -306,16 +312,11 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>
>   	rcu_read_lock();
>   	owner = READ_ONCE(sem->owner);
> -	if (!owner) {
> -		long count = READ_ONCE(sem->count);
> +	if (!rwsem_is_writer_owned(owner)) {
>   		/*
> -		 * If sem->owner is not set, yet we have just recently entered the
> -		 * slowpath with the lock being active, then there is a possibility
> -		 * reader(s) may have the lock. To be safe, bail spinning in these
> -		 * situations.
> +		 * Don't spin if the rwsem is readers owned.
>   		 */
> -		if (count&  RWSEM_ACTIVE_MASK)
> -			ret = false;
> +		ret = !rwsem_is_reader_owned(owner);
>   		goto done;
>   	}
> I'm not a big fan of all the helpers; istm like they're obfuscating the more
> important requirements of rwsem. For example, this reduces to
>
> 	rcu_read_lock();
> 	owner = READ_ONCE(sem->owner);
> 	ret = !owner || (rwsem_is_writer_owned(owner)&&  owner->on_cpu);
> 	rcu_read_unlock();
> 	return ret;

Using helper functions usually make the code easier to read. This is 
helpful for the rwsem code which can be hard to understand especially 
how the different count values interact.


> > @@ -328,8 +329,6 @@ done:
> >   static noinline
> >   bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner)
> >   {
> > -	long count;
> > -
> >   	rcu_read_lock();
> >   	while (sem->owner == owner) {
> >   		/*
> > @@ -350,16 +349,11 @@ bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner)
> >   	}
> >   	rcu_read_unlock();
> >
> > -	if (READ_ONCE(sem->owner))
> > -		return true; /* new owner, continue spinning */
> > -
> >   	/*
> > -	 * When the owner is not set, the lock could be free or
> > -	 * held by readers. Check the counter to verify the
> > -	 * state.
> > +	 * If there is a new owner or the owner is not set, we continue
> > +	 * spinning.
> >   	 */
> > -	count = READ_ONCE(sem->count);
> > -	return (count == 0 || count == RWSEM_WAITING_BIAS);
> > +	return !rwsem_is_reader_owned(READ_ONCE(sem->owner));
> It doesn't make sense to force reload sem->owner here; if sem->owner
> is not being reloaded then the loop above will execute forever.

I agree that we don't actually need to use READ_ONCE() here for 
sem->owner as the barrier() call will force the reloading. It is more 
like a habit to use it for public variable or we will have to think a 
bit harder to make sure that we are doing the right thing.

> Arguably, this check should be bumped out to the optimistic spin and
> reload/check the owner there?
>
> Or better yet; don't pass the owner in as a parameter at all, but
> instead snapshot the owner and check its ownership on entry.

That will make the main optimistic spinning loop more complex.

> Because see below...
>
> >   }
> >
> >   static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
> > @@ -378,7 +372,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 +386,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 (rwsem_is_reader_owned(owner) ||
> > +		   (!owner&&  (need_resched() || rt_task(current))))
> This is using the stale owner value that was cached before spinning on the owner;
> That can't be right.

The code is going to loop back and reload the new owner value anyway. It 
is just a bit of additional latency. I will move the is_reader check up 
after loading sem->owner to clear any confusion.

Cheers,
Longman