Re: [BUG]locking/rwsem: only clean RWSEM_FLAG_HANDOFF when already set

[Waiman Long]

On 11/11/21 16:55, Waiman Long wrote:
> On 11/11/21 16:53, Peter Zijlstra wrote:
> > On Thu, Nov 11, 2021 at 04:25:56PM -0500, Waiman Long wrote:
> > > On 11/11/21 16:01, Waiman Long wrote:
> > > > On 11/11/21 15:26, Peter Zijlstra wrote:
> > > > > On Thu, Nov 11, 2021 at 02:36:52PM -0500, Waiman Long wrote:
> > > > >
> > > > > > @@ -434,6 +430,7 @@ static void rwsem_mark_wake(struct
> > > > > > rw_semaphore *sem,
> > > > > >                if (!(oldcount & RWSEM_FLAG_HANDOFF) &&
> > > > > >                    time_after(jiffies, waiter->timeout)) {
> > > > > >                    adjustment -= RWSEM_FLAG_HANDOFF;
> > > > > > +                waiter->handoff_set = true;
> > > > > >                    lockevent_inc(rwsem_rlock_handoff);
> > > > > >                }
> > > > > Do we really need this flag? Wouldn't it be the same as 
> > > > > waiter-is-first
> > > > > AND sem-has-handoff ?
> > > > That is true. The only downside is that we have to read the count 
> > > > first
> > > > in rwsem_out_nolock_clear_flags(). Since this is not a fast path, it
> > > > should be OK to do that.
> > > I just realize that I may still need this flag for writer to 
> > > determine if it
> > > should spin after failing to acquire the lock. Or I will have to do 
> > > extra
> > > read of count value in the loop. I don't need to use it for writer now.
> > Maybe it's too late here, but afaict this is right after failing
> > try_write_lock(), which will have done at least that load you're
> > interested in, no?
> >
> > Simply have try_write_lock() update &count or something.
>
> You are right. I have actually decided to do an extra read after 
> second thought.

Oh, I would like to take it back. The condition to do spinning is when 
the handoff bit is set and the waiter is the first one in the queue. It 
be easier to do it with extra internal state variable.

Cheers,
Longman