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

[Peter Hurley <peter@hurleysoftware.com>]

On 05/12/2016 01:15 PM, Waiman Long wrote:
> On 05/11/2016 06:04 PM, Peter Hurley wrote:

[...]

>>
>>> @@ -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.

I look at that code and I think "what am I missing that it needs to reload at exit"
Extra cruft is just as misleading.

What matters while spinning on owner is that the reload is forced as part of the
loop, not at exit time. The whole point of rwsem_spin_on_owner() is to spin
until the owner _changes_; there's no need to get a more recent owner value
than the one that caused the loop to break.


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

??

Simpler.

	while (rwsem_spin_on_owner(sem)) {
		if (rwsem_try_write_lock_unqueued(sem)) {
			taken = true;
			break;
		}

		if (!sem->owner && (need_resched() || rt_task(current)))
			break;

		cpu_relax_lowlatency();
	}




bool rwsem_spin_on_owner(struct rw_semaphore *sem)
{
	struct task_struct *owner = READ_ONCE(sem->owner);

	if (!rwsem_is_writer_owned(owner))
		return false;

	rcu_read_lock();
	while (sem->owner == owner) {
		....
	}
	rcu_read_unlock();
	
	return !rwsem_is_reader_owned(sem->owner);
}


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

Well, why do it at all then?  Just before attempting the lock, rwsem_spin_on_owner()
determined that the owner was not a reader. If a reader became the new owner after
that, the loop will discover that the lock is no longer writer owned anyway.

Either way, the check here should be to sem->owner (ie., like I wrote above),
but there's no need to force reload here either.

Regards,
Peter Hurley