On 04/17/2019 09:00 AM, Peter Zijlstra wrote: > On Wed, Apr 17, 2019 at 02:41:01PM +0200, Peter Zijlstra wrote: >> On Sat, Apr 13, 2019 at 01:22:51PM -0400, Waiman Long wrote: >>> In the special case that there is no active lock and the handoff bit >>> is set, optimistic spinning has to be stopped. >>> @@ -500,9 +521,19 @@ static noinline bool rwsem_spin_on_owner(struct >>> rw_semaphore *sem) >>> >>> /* >>> * If there is a new owner or the owner is not set, we continue >>> - * spinning. >>> + * spinning except when here is no active locks and the handoff bit >>> + * is set. In this case, we have to stop spinning. >>> */ >>> - return is_rwsem_owner_spinnable(READ_ONCE(sem->owner)); >>> + owner = READ_ONCE(sem->owner); >>> + if (!is_rwsem_owner_spinnable(owner)) >>> + return OWNER_NONSPINNABLE; >>> + if (owner && !is_rwsem_owner_reader(owner)) >>> + return OWNER_WRITER; >>> + >>> + count = atomic_long_read(&sem->count); >>> + if (RWSEM_COUNT_HANDOFF(count) && !RWSEM_COUNT_LOCKED(count)) >>> + return OWNER_NONSPINNABLE; >>> + return !owner ? OWNER_NULL : OWNER_READER; >>> } >> So this fixes a straight up bug in the last patch (and thus should be >> done before so the bug never exists), and creates unreadable code while >> at it. >> >> Also, I think only checking HANDOFF after the loop is wrong; the moment >> HANDOFF happens you have to terminate the loop, irrespective of what >> @owner does. >> >> Does something like so work? >> >> --- > enum owner_state { > OWNER_NULL = 1 << 0, > OWNER_WRITER = 1 << 1, > OWNER_READER = 1 << 2, > OWNER_NONSPINNABLE = 1 << 3, > }; > #define OWNER_SPINNABLE (OWNER_NULL | OWNER_WRITER) > > static inline enum owner_state rwsem_owner_state(unsigned long owner) > { > if (!owner) > return OWNER_NULL; > > if (owner & RWSEM_ANONYMOUSLY_OWNED) > return OWNER_NONSPINNABLE; > > if (owner & RWSEM_READER_OWNER) > return OWNER_READER; > > return OWNER_WRITER; > } > > static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem) > { > struct task_struct *tmp, *owner = READ_ONCE(sem->owner); > enum owner_state state = rwsem_owner_state((unsigned long)owner); > > if (state != OWNER_WRITER) > return state; > > rcu_read_lock(); > for (;;) { > if (atomic_long_read(&sem->count) & RWSEM_FLAG_HANDOFF) { > state = OWNER_NONSPINNABLE; > break; > } > > tmp = READ_ONCE(sem->owner); > if (tmp != owner) { > state = rwsem_owner_state((unsigned long)tmp); > break; > } > > /* > * Ensure we emit the owner->on_cpu, dereference _after_ > * checking sem->owner still matches owner, if that fails, > * owner might point to free()d memory, if it still matches, > * the rcu_read_lock() ensures the memory stays valid. > */ > barrier(); > > if (need_resched() || !owner_on_cpu(owner)) { > state = OWNER_NONSPINNABLE; > break; > } > > cpu_relax(); > } > rcu_read_unlock(); > > return state; > }
That code looks good to me. Thanks for the rewrite. Cheers, Longman