Re: [Xen-devel] [PATCH v5 1/8] qspinlock: Introducing a 4-byte queue spinlock implementation

[Oleg Nesterov <oleg@xxxxxxxxxx>]

On 02/26, Waiman Long wrote:
>
> +void queue_spin_lock_slowpath(struct qspinlock *lock, int qsval)
> +{
> +     unsigned int cpu_nr, qn_idx;
> +     struct qnode *node, *next;
> +     u32 prev_qcode, my_qcode;
> +
> +     /*
> +      * Get the queue node
> +      */
> +     cpu_nr = smp_processor_id();
> +     node   = get_qnode(&qn_idx);
> +
> +     /*
> +      * It should never happen that all the queue nodes are being used.
> +      */
> +     BUG_ON(!node);
> +
> +     /*
> +      * Set up the new cpu code to be exchanged
> +      */
> +     my_qcode = queue_encode_qcode(cpu_nr, qn_idx);
> +
> +     /*
> +      * Initialize the queue node
> +      */
> +     node->wait = true;
> +     node->next = NULL;
> +
> +     /*
> +      * The lock may be available at this point, try again if no task was
> +      * waiting in the queue.
> +      */
> +     if (!(qsval >> _QCODE_OFFSET) && queue_spin_trylock(lock)) {
> +             put_qnode();
> +             return;
> +     }

Cosmetic, but probably "goto release_node" would be more consistent.

And I am wondering how much this "qsval >> _QCODE_OFFSET" check can help.
Note that this is the only usage of this arg, perhaps it would be better
to simply remove it and shrink the caller's code a bit? It is also used
in 3/8, but we can read the "fresh" value of ->qlcode (trylock does this
anyway), and perhaps it can actually help if it is already unlocked.

> +     prev_qcode = atomic_xchg(&lock->qlcode, my_qcode);
> +     /*
> +      * It is possible that we may accidentally steal the lock. If this is
> +      * the case, we need to either release it if not the head of the queue
> +      * or get the lock and be done with it.
> +      */
> +     if (unlikely(!(prev_qcode & _QSPINLOCK_LOCKED))) {
> +             if (prev_qcode == 0) {
> +                     /*
> +                      * Got the lock since it is at the head of the queue
> +                      * Now try to atomically clear the queue code.
> +                      */
> +                     if (atomic_cmpxchg(&lock->qlcode, my_qcode,
> +                                       _QSPINLOCK_LOCKED) == my_qcode)
> +                             goto release_node;
> +                     /*
> +                      * The cmpxchg fails only if one or more tasks
> +                      * are added to the queue. In this case, we need to
> +                      * notify the next one to be the head of the queue.
> +                      */
> +                     goto notify_next;
> +             }
> +             /*
> +              * Accidentally steal the lock, release the lock and
> +              * let the queue head get it.
> +              */
> +             queue_spin_unlock(lock);
> +     } else
> +             prev_qcode &= ~_QSPINLOCK_LOCKED;       /* Clear the lock bit */

You know, actually I started this email because I thought that "goto 
notify_next"
is wrong, I misread the patch as if this "goto" can happen even if prev_qcode 
!= 0.

So feel free to ignore, all my comments are cosmetic/subjective, but to me it
would be more clean/clear to rewrite the code above as

        if (prev_qcode == 0) {
                if (atomic_cmpxchg(..., _QSPINLOCK_LOCKED) == my_qcode)
                        goto release_node;
                goto notify_next;
        }

        if (prev_qcode & _QSPINLOCK_LOCKED)
                prev_qcode &= ~_QSPINLOCK_LOCKED;
        else
                queue_spin_unlock(lock);


> +     while (true) {
> +             u32 qcode;
> +             int retval;
> +
> +             retval = queue_get_lock_qcode(lock, &qcode, my_qcode);
> +             if (retval > 0)
> +                     ;       /* Lock not available yet */
> +             else if (retval < 0)
> +                     /* Lock taken, can release the node & return */
> +                     goto release_node;

I guess this is for 3/8which adds the optimized version of
queue_get_lock_qcode(), so perhaps this "retval < 0" block can go into 3/8
as well.

> +             else if (qcode != my_qcode) {
> +                     /*
> +                      * Just get the lock with other spinners waiting
> +                      * in the queue.
> +                      */
> +                     if (queue_spin_setlock(lock))
> +                             goto notify_next;

OTOH, at least the generic (non-optimized) version of queue_spin_setlock()
could probably accept "qcode" and avoid atomic_read() + _QSPINLOCK_LOCKED
check.

But once again, please feel free to ignore.

Oleg.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel