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

[Waiman Long <waiman.long@hpe.com>]

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