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.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
