Re: [PATCH v4 11/16] locking/rwsem: Enable readers spinning on writer

[Waiman Long]

On 04/18/2019 04:57 AM, Peter Zijlstra wrote:
> On Wed, Apr 17, 2019 at 01:34:01PM -0400, Waiman Long wrote:
>> On 04/17/2019 09:56 AM, Peter Zijlstra wrote:
>>> On Sat, Apr 13, 2019 at 01:22:54PM -0400, Waiman Long wrote:
>>>> @@ -549,7 +582,7 @@ static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem)
>>>>  	return !owner ? OWNER_NULL : OWNER_READER;
>>>>  }
>>>>  
>>>> -static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
>>>> +static bool rwsem_optimistic_spin(struct rw_semaphore *sem, bool wlock)
>>>>  {
>>>>  	bool taken = false;
>>>>  	bool is_rt_task = rt_task(current);
>>>> @@ -558,9 +591,6 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
>>>>  	preempt_disable();
>>>>  
>>>>  	/* sem->wait_lock should not be held when doing optimistic spinning */
>>>> -	if (!rwsem_can_spin_on_owner(sem))
>>>> -		goto done;
>>>> -
>>>>  	if (!osq_lock(&sem->osq))
>>>>  		goto done;
>>>>  
>>>> @@ -580,10 +610,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
>>>>  		/*
>>>>  		 * Try to acquire the lock
>>>>  		 */
>>>> -		if (rwsem_try_write_lock_unqueued(sem)) {
>>>> -			taken = true;
>>>> +		taken = wlock ? rwsem_try_write_lock_unqueued(sem)
>>>> +			      : rwsem_try_read_lock_unqueued(sem);
>>>> +
>>>> +		if (taken)
>>>>  			break;
>>>> -		}
>>>>  
>>>>  		/*
>>>>  		 * An RT task cannot do optimistic spinning if it cannot
>>> Alternatively you pass the trylock function as an argument:
>>>
>>> static bool rwsem_optimistic_spin(struct rw_semaphore *sem,
>>> 				  bool (*trylock)(struct rw_semaphore *sem))
>>> {
>>> 	...
>>> 		if (trylock(sem)) {
>>> 			taken = true;
>>> 			goto unlock;
>>> 		}
>>> 	...
>>> }
>>>
>> With retpoline, an indirect function call will be slower.
> With compiler optimization we can avoid that. Just mark the function as
> __always_inline, there's only two call-sites, each with a different
> trylock.
>
> It might have already done that anyway, and used constant propagation
> on your bool, but the function pointer one is far easier to read.

The bool was extended to an "unsigned long" and trylock functions will
have different argument list in the last patch.

-Longman