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

[Mukesh Ojha]

Hi,

On 10/18/2022 4:44 PM, Hillf Danton wrote:
> On 17 Oct 2022 17:13:55 -0400 Waiman Long <longman@xxxxxxxxxx>
> > @@ -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