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)
Hmm, we should not spin on OWNER_NULL. Or at least not mixed in with the
patch that changes the shape of all this. That should go in the RT
thingy patch, which comes after this.
> 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;
>
> rcu_read_lock();
> for (;;) {
> state = rwsem_owner_state((unsigned long)owner);
> if (!(state & OWNER_SPINNABLE))
> break;
>
> 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;
> }
