linux-kernel - Re: [PATCH v7 4/4] locking/rwsem: Enable direct rwsem lock handoff

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <0911bd7f-2d8b-e925-2816-a0b5e01134b6@redhat.com>
Date:   Thu, 26 Jan 2023 07:59:17 -0500
From:   Waiman Long <longman@...hat.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        linux-kernel@...r.kernel.org, john.p.donnelly@...cle.com,
        Mukesh Ojha <quic_mojha@...cinc.com>,
        Ting11 Wang 王婷 <wangting11@...omi.com>
Subject: Re: [PATCH v7 4/4] locking/rwsem: Enable direct rwsem lock handoff

On 1/26/23 05:04, Hillf Danton wrote:
> On Wed, 25 Jan 2023 19:36:28 -0500 Waiman Long <longman@...hat.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