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

[Waiman Long]

On 10/18/22 10:13, Mukesh Ojha wrote:
> 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?
This is under wait_lock protection. So no parallel execution is possible.
> > > +        /*
> > > +         * 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?
The idea is to have as much reader parallelism as possible without 
writer starvation. In other word, a continuous stream of readers is not 
allowed to block out writer. However, there are places where allow one 
more reader to get the lock won't cause writer starvation.
> > >   -    /* 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.

This patch is too disruptive to go to the stable branches. Yes, I do 
intend the first 2 patches to go into stable branches.

Cheers,
Longman