diff mbox series

locking/rwsem: Remove unnessary check in rwsem_down_read_slowpath()

Message ID 20231108105639.70088-1-haifeng.xu@shopee.com
State New
Headers
Series locking/rwsem: Remove unnessary check in rwsem_down_read_slowpath() |

Commit Message

Haifeng Xu Nov. 8, 2023, 10:56 a.m. UTC 
  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(-)

Comments

Waiman Long Nov. 8, 2023, 2:04 p.m. UTC | #1 
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. So the RWSEM_WRITER_LOCKED check can 
still provide some value. We should probably update the comment to 
reflect that.

Cheers,
Longman
Haifeng Xu Nov. 9, 2023, 3:17 a.m. UTC | #2 
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
>
Tang Yizhou Nov. 10, 2023, 6:54 a.m. UTC | #3 
On Thu, Nov 9, 2023 at 11:17 AM Haifeng Xu <haifeng.xu@shopee.com> wrote:
>
> 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.
>

For the above example, it won't cause a problem. When the writer
successfully sets RWSEM_WRITER_LOCKED, the reader, when reading rcnt
through rwsem_down_read_slowpath(), will see that rcnt is 0 and will
jump to the queue label.

Thanks,
Tang
Haifeng Xu Nov. 10, 2023, 10:29 a.m. UTC | #4 
On 2023/11/10 14:54, Tang Yizhou wrote:
> On Thu, Nov 9, 2023 at 11:17 AM Haifeng Xu <haifeng.xu@shopee.com> wrote:
>>
>> 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.
>>
> 
> For the above example, it won't cause a problem. When the writer
> successfully sets RWSEM_WRITER_LOCKED, the reader, when reading rcnt
> through rwsem_down_read_slowpath(), will see that rcnt is 0 and will
> jump to the queue label.
> 
> Thanks,
> Tang

Yes, so if rcnt > 1, the RWSEM_WRITER_LOCKED bit couldn't be set?
Haifeng Xu Nov. 10, 2023, 11 a.m. UTC | #5 
On 2023/11/10 14:54, Tang Yizhou wrote:
> On Thu, Nov 9, 2023 at 11:17 AM Haifeng Xu <haifeng.xu@shopee.com> wrote:
>>
>> 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.
>>
> 
> For the above example, it won't cause a problem. When the writer
> successfully sets RWSEM_WRITER_LOCKED, the reader, when reading rcnt
> through rwsem_down_read_slowpath(), will see that rcnt is 0 and will
> jump to the queue label.
> 
> Thanks,
> Tang

In this case, rcnt is not 0, it's 1, because rwsem_read_trylock() has add RWSEM_READER_BIAS, so if more than one new reader comes,
it could be the case.

reader		writer					reader									reader

acquire
release
		rwsem_write_trylock
			set RWSEM_WRITER_LOCKED
							rwsem_down_read_slowpath						rwsem_down_read_slowpath
			
								...									check RWSEM_WRITER_LOCKED bit(rcnt=2)
								

								count = atomic_long_add_return(adjustment, &sem->count);

			set owner
Waiman Long Nov. 10, 2023, 1:38 p.m. UTC | #6 
On 11/10/23 05:29, Haifeng Xu wrote:
>
> On 2023/11/10 14:54, Tang Yizhou wrote:
>> On Thu, Nov 9, 2023 at 11:17 AM Haifeng Xu <haifeng.xu@shopee.com> wrote:
>>> 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.
>>>
>> For the above example, it won't cause a problem. When the writer
>> successfully sets RWSEM_WRITER_LOCKED, the reader, when reading rcnt
>> through rwsem_down_read_slowpath(), will see that rcnt is 0 and will
>> jump to the queue label.
>>
>> Thanks,
>> Tang
> Yes, so if rcnt > 1, the RWSEM_WRITER_LOCKED bit couldn't be set?

No. The way readers acquire the lock is via 
atomic_long_add_return_acquire() without looking at current state of the 
rwsem (write-locked or not). So rcnt can be greater than 0 and the rwsem 
is still writer-owned.

Because of that atomic_long_add_return_acquire() primitive, rcnt 
includes its reader count. The lock may be read-locked if only if there 
is at least one other reader present. So rcnt must be bigger than 1.

Cheers,
Longman
diff mbox series

Patch

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;
 
 	/*