Message-ID: <0911bd7f-2d8b-e925-2816-a0b5e01134b6@redhat.com>
Date:   Thu, 26 Jan 2023 07:59:17 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH v7 4/4] locking/rwsem: Enable direct rwsem lock handoff
Content-Language: en-US
To:     Hillf Danton <hdanton@sina.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        linux-kernel@vger.kernel.org, john.p.donnelly@oracle.com,
        Mukesh Ojha <quic_mojha@quicinc.com>,
        =?UTF-8?B?VGluZzExIFdhbmcg546L5am3?= <wangting11@xiaomi.com>
References: <20230126003628.365092-1-longman@redhat.com>
 <20230126100441.4823-1-hdanton@sina.com>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <20230126100441.4823-1-hdanton@sina.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/26/23 05:04, Hillf Danton wrote:
> On Wed, 25 Jan 2023 19:36:28 -0500 Waiman Long <longman@redhat.com>
>>   
>>   static struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
>>   {
>> -	unsigned long flags;
>>   	DEFINE_WAKE_Q(wake_q);
>> +	unsigned long flags;
>> +	unsigned long count;
>>   
>>   	raw_spin_lock_irqsave(&sem->wait_lock, flags);
>>   
>> -	if (!list_empty(&sem->wait_list))
>> -		rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
>> +	if (list_empty(&sem->wait_list))
>> +		goto unlock_out;
>> +
>> +	/*
>> +	 * If the rwsem is free and handoff flag is set with wait_lock held,
>> +	 * no other CPUs can take an active lock.
>> +	 */
>> +	count = atomic_long_read(&sem->count);
>> +	if (!(count & RWSEM_LOCK_MASK) && (count & RWSEM_FLAG_HANDOFF)) {
>> +		/*
>> +		 * Since rwsem_mark_wake() will handle the handoff to reader
>> +		 * properly, we don't need to do anything extra for reader.
>> +		 * Special handoff processing will only be needed for writer.
>> +		 */
>> +		struct rwsem_waiter *waiter = rwsem_first_waiter(sem);
>> +		long adj = RWSEM_WRITER_LOCKED - RWSEM_FLAG_HANDOFF;
>> +
>> +		if (waiter->type == RWSEM_WAITING_FOR_WRITE) {
>> +			atomic_long_set(&sem->owner, (long)waiter->task);
>> +			atomic_long_add(adj, &sem->count);
>> +			wake_q_add(&wake_q, waiter->task);
>> +			rwsem_del_waiter(sem, waiter);
>> +			waiter->task = NULL;	/* Signal the handoff */
> Nit, once waiter is signaled, the address of waiter on stack could be destructed,
> so use smp_store_release() instead.

The subsequent raw_spin_unlock_irqrestore() already has the release 
semantics. That is why I used a regular store. Note that this is in a 
lock critical section. I would have used smp_store_release() outside of 
that.

Cheers,
Longman

>> +			goto unlock_out;
>> +		}
>> +	}
>> +	rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
>>   
>> +unlock_out:
>>   	raw_spin_unlock_irqrestore(&sem->wait_lock, flags);
>>   	wake_up_q(&wake_q);
>>   
>> -- 
>> 2.31.1