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

[Waiman Long]

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