Re: [PATCH] locking/rwsem: Remove unnessary check in rwsem_down_read_slowpath()

[Haifeng Xu <haifeng.xu@shopee.com>]

On 2023/11/8 22:04, Waiman Long wrote:
> On 11/8/23 05:56, Haifeng Xu wrote:
>> When the owner of rw_semaphore is reader, the count can't be
>> RWSEM_WRITER_LOCKED, so there is no need to check it.
>>
>> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
>> ---
>>   kernel/locking/rwsem.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>> index 2340b6d90ec6..7a4d8a9ebd9c 100644
>> --- a/kernel/locking/rwsem.c
>> +++ b/kernel/locking/rwsem.c
>> @@ -1005,8 +1005,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
>>        * waiter, don't attempt optimistic lock stealing if the lock is
>>        * currently owned by readers.
>>        */
>> -    if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) &&
>> -        (rcnt > 1) && !(count & RWSEM_WRITER_LOCKED))
>> +    if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) && (rcnt > 1))
>>           goto queue;
>>         /*
> 
> Unlike RWSEM_WRITER_LOCKED bit in count, the RWSEM_READER_OWNED bit in owner is just a hint, not an authoritative state of the rwsem. So it is possible that both the RWSEM_READER_OWNED bit can be set in owner and RWSEM_WRITER_LOCKED bit set in count in a transition period right after RWSEM_WRITER_LOCKED bit is set. 

reader		writer					reader

acquire		
release		
		rwsem_write_trylock
			set RWSEM_WRITER_LOCKED
							rwsem_down_read_slowpath
			set owner

If prev lock holder is a reader, when it releases the lock, the owner isn't cleared(CONFIG_DEBUG_RWSEMS isn't enabled).
A writer comes and can set the RWSEM_WRITER_LOCKED bit succsessfully, then a new reader run into slow path, before
the writer set the owner, the new reader will see that both the RWSEM_READER_OWNED bit and RWSEM_WRITER_LOCKED bit are
set.

So the above sequence could be the case, right?



So the RWSEM_WRITER_LOCKED check can still provide some value. We should probably update the comment to reflect that.
> 
> Cheers,
> Longman
>