Re: [PATCH v4 08/16] locking/rwsem: Make rwsem_spin_on_owner() return owner state

[Waiman Long <longman@redhat.com>]

On 04/17/2019 09:00 AM, Peter Zijlstra wrote:
> On Wed, Apr 17, 2019 at 02:41:01PM +0200, Peter Zijlstra wrote:
>> On Sat, Apr 13, 2019 at 01:22:51PM -0400, Waiman Long wrote:
>>> In the special case that there is no active lock and the handoff bit
>>> is set, optimistic spinning has to be stopped.
>>> @@ -500,9 +521,19 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
>>>  
>>>  	/*
>>>  	 * If there is a new owner or the owner is not set, we continue
>>> -	 * spinning.
>>> +	 * spinning except when here is no active locks and the handoff bit
>>> +	 * is set. In this case, we have to stop spinning.
>>>  	 */
>>> -	return is_rwsem_owner_spinnable(READ_ONCE(sem->owner));
>>> +	owner = READ_ONCE(sem->owner);
>>> +	if (!is_rwsem_owner_spinnable(owner))
>>> +		return OWNER_NONSPINNABLE;
>>> +	if (owner && !is_rwsem_owner_reader(owner))
>>> +		return OWNER_WRITER;
>>> +
>>> +	count = atomic_long_read(&sem->count);
>>> +	if (RWSEM_COUNT_HANDOFF(count) && !RWSEM_COUNT_LOCKED(count))
>>> +		return OWNER_NONSPINNABLE;
>>> +	return !owner ? OWNER_NULL : OWNER_READER;
>>>  }
>> So this fixes a straight up bug in the last patch (and thus should be
>> done before so the bug never exists), and creates unreadable code while
>> at it.
>>
>> Also, I think only checking HANDOFF after the loop is wrong; the moment
>> HANDOFF happens you have to terminate the loop, irrespective of what
>> @owner does.
>>
>> Does something like so work?
>>
>> ---
> enum owner_state {
> 	OWNER_NULL		= 1 << 0,
> 	OWNER_WRITER		= 1 << 1,
> 	OWNER_READER		= 1 << 2,
> 	OWNER_NONSPINNABLE	= 1 << 3,
> };
> #define OWNER_SPINNABLE		(OWNER_NULL | OWNER_WRITER)
>
> static inline enum owner_state rwsem_owner_state(unsigned long owner)
> {
> 	if (!owner)
> 		return OWNER_NULL;
>
> 	if (owner & RWSEM_ANONYMOUSLY_OWNED)
> 		return OWNER_NONSPINNABLE;
>
> 	if (owner & RWSEM_READER_OWNER)
> 		return OWNER_READER;
>
> 	return OWNER_WRITER;
> }
>
> static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem)
> {
> 	struct task_struct *tmp, *owner = READ_ONCE(sem->owner);
> 	enum owner_state state = rwsem_owner_state((unsigned long)owner);
>
> 	if (state != OWNER_WRITER)
> 		return state;
>
> 	rcu_read_lock();
> 	for (;;) {
> 		if (atomic_long_read(&sem->count) & RWSEM_FLAG_HANDOFF) {
> 			state = OWNER_NONSPINNABLE;
> 			break;
> 		}
>
> 		tmp = READ_ONCE(sem->owner);
> 		if (tmp != owner) {
> 			state = rwsem_owner_state((unsigned long)tmp);
> 			break;
> 		}
>
> 		/*
> 		 * Ensure we emit the owner->on_cpu, dereference _after_
> 		 * checking sem->owner still matches owner, if that fails,
> 		 * owner might point to free()d memory, if it still matches,
> 		 * the rcu_read_lock() ensures the memory stays valid.
> 		 */
> 		barrier();
>
> 		if (need_resched() || !owner_on_cpu(owner)) {
> 			state = OWNER_NONSPINNABLE;
> 			break;
> 		}
>
> 		cpu_relax();
> 	}
> 	rcu_read_unlock();
>
> 	return state;
> }

That code looks good to me. Thanks for the rewrite.

Cheers,
Longman