lkml.org 
[lkml]   [2019]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 08/16] locking/rwsem: Make rwsem_spin_on_owner() return owner state
    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;
    }

    \
     
     \ /
      Last update: 2019-04-17 15:01    [W:4.305 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site