Re: [PATCH v3 4/5] locking/rwsem: Enable direct rwsem lock handoff

[Mukesh Ojha <quic_mojha@quicinc.com>]

Hi,

On 10/18/2022 4:44 PM, Hillf Danton wrote:
> On 17 Oct 2022 17:13:55 -0400 Waiman Long <longman@redhat.com>
>> @@ -1067,13 +1119,33 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
>>   			return sem;
>>   		}
>>   		adjustment += RWSEM_FLAG_WAITERS;
>> +	} else if ((count & RWSEM_FLAG_HANDOFF) &&
>> +		  ((count & RWSEM_LOCK_MASK) == RWSEM_READER_BIAS)) {
> 
> Could a couple of CPUs go read slow path in parallel?
> 
>> +		/*
>> +		 * If the waiter to be handed off is a reader, this reader
>> +		 * can piggyback on top of top of that.
>> +		 */
>> +		if (rwsem_first_waiter(sem)->type == RWSEM_WAITING_FOR_READ)
>> +			adjustment = 0;
>> +		rwsem_handoff(sem, adjustment, &wake_q);
>> +
>> +		if (!adjustment) {
>> +			raw_spin_unlock_irq(&sem->wait_lock);
>> +			wake_up_q(&wake_q);
>> +			return sem;
>> +		}
>> +		adjustment = 0;
>>   	}
>>   	rwsem_add_waiter(sem, &waiter);
> 
> Why can this acquirer pigyback without becoming a waiter?
> 
>>   
>> -	/* we're now waiting on the lock, but no longer actively locking */
>> -	count = atomic_long_add_return(adjustment, &sem->count);
>> -
>> -	rwsem_cond_wake_waiter(sem, count, &wake_q);
>> +	if (adjustment) {
>> +		/*
>> +		 * We are now waiting on the lock with no handoff, but no
>> +		 * longer actively locking.
>> +		 */
>> +		count = atomic_long_add_return(adjustment, &sem->count);
>> +		rwsem_cond_wake_waiter(sem, count, &wake_q);
>> +	}
>>   	raw_spin_unlock_irq(&sem->wait_lock);
>>   
>>   	if (!wake_q_empty(&wake_q))
>> @@ -1120,7 +1192,6 @@ static struct rw_semaphore __sched *
>>   rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
>>   {
>>   	struct rwsem_waiter waiter;
>> -	int null_owner_retries;
> 
> This reverts 2/5 and feel free to drop it directly.

I think, he intents to tag the first two patches to go to stable branches.

-Mukesh
> 
> Hillf