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

[Waiman Long]

On 11/11/21 15:50, Peter Zijlstra wrote:
> So I suspect that if..
>
> On Thu, Nov 11, 2021 at 02:36:52PM -0500, Waiman Long wrote:
> >   static inline bool rwsem_try_write_lock(struct rw_semaphore *sem,
> > -					enum writer_wait_state wstate)
> > +					struct rwsem_waiter *waiter)
> >   {
> >   	long count, new;
> > +	bool first = rwsem_first_waiter(sem) == waiter;
> >   
> >   	lockdep_assert_held(&sem->wait_lock);
> >   
> > @@ -546,13 +541,14 @@ static inline bool rwsem_try_write_lock(struct rw_semaphore *sem,
> >   	do {
> >   		bool has_handoff = !!(count & RWSEM_FLAG_HANDOFF);
> >   
> > -		if (has_handoff && wstate == WRITER_NOT_FIRST)
> > +		if (has_handoff && !first)
> >   			return false;
> >   
> >   		new = count;
> >   
> >   		if (count & RWSEM_LOCK_MASK) {
> > -			if (has_handoff || (wstate != WRITER_HANDOFF))
> > +			if (has_handoff || (!waiter->rt_task &&
> > +					    !time_after(jiffies, waiter->timeout)))
> >   				return false;
> we delete this whole condition, and..
I don't think we can take out this if test.
> >   
> >   			new |= RWSEM_FLAG_HANDOFF;
> > @@ -889,6 +888,24 @@ rwsem_spin_on_owner(struct rw_semaphore *sem)
> >   }
> >   #endif
> >   
> > +/*
> > + * Common code to handle rwsem flags in out_nolock path with wait_lock held.
> > + */
> > +static inline void rwsem_out_nolock_clear_flags(struct rw_semaphore *sem,
> > +						struct rwsem_waiter *waiter)
> > +{
> > +	long flags = 0;
> > +
> > +	list_del(&waiter->list);
> > +	if (list_empty(&sem->wait_list))
> > +		flags = RWSEM_FLAG_HANDOFF | RWSEM_FLAG_WAITERS;
> > +	else if (waiter->handoff_set)
> > +		flags = RWSEM_FLAG_HANDOFF;
> take out this else,
>
> > +
> > +	if (flags)
> > +		atomic_long_andnot(flags,  &sem->count);
> > +}
> We get the inherit thing for free, no?
>
> Once HANDOFF is set, new readers are blocked. And then allow any first
> waiter to acquire the lock, who cares if it was the one responsible for
> having set the HANDOFF bit.

Yes, we can have the policy of inheriting the HANDOFF bit as long as it 
is consistent which will be the case here with a common out_nolock 
function. I can go with that. I just have to document this fact in the 
comment.

Cheers,
Longman