On Mon, Jun 23, 2014 at 06:12:00PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 17, 2014 at 04:03:29PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > + new = tail | (val & _Q_LOCKED_MASK);
> > > > +
> > > > + old = atomic_cmpxchg(&lock->val, val, new);
> > > > + if (old == val)
> > > > + break;
> > > > +
> > > > + val = old;
> > > > + }
> > > > +
> > > > + /*
> > > > + * we won the trylock; forget about queueing.
> > > > + */
> > > > + if (new == _Q_LOCKED_VAL)
> > > > + goto release;
> > > > +
> > > > + /*
> > > > + * if there was a previous node; link it and wait.
> > > > + */
> > > > + if (old & ~_Q_LOCKED_MASK) {
> > > > + prev = decode_tail(old);
> > > > + ACCESS_ONCE(prev->next) = node;
> > > > +
> > > > + arch_mcs_spin_lock_contended(&node->locked);
> >
> > Could you add a comment here:
> >
> > /* We are spinning forever until the previous node updates locked - which
> > it does once the it has updated lock->val with our tail number. */
>
> That's incorrect -- or at least, I understand that to be incorrect. The
> previous node will not have changed the tail to point to us. You always
> change to tail to point to yourself, seeing how you add yourself to the
> tail.
>
> Is the existing comment any better if I s/wait./wait for it to release
> us./ ?
Yes!
>
> > > > + /*
> > > > + * claim the lock:
> > > > + *
> > > > + * n,0 -> 0,1 : lock, uncontended
> > > > + * *,0 -> *,1 : lock, contended
> > > > + */
> > > > + for (;;) {
> > > > + new = _Q_LOCKED_VAL;
> > > > + if (val != tail)
> > > > + new |= val;
> > >
> > ..snip..
> > >
> > > Could you help a bit in explaining it in English please?
> >
> > After looking at the assembler code I finally figured out how
> > we can get here. And the 'contended' part threw me off. Somehow
> > I imagined there are two more more CPUs stampeding here and
> > trying to update the lock->val. But in reality the other CPUs
> > are stuck in the arch_mcs_spin_lock_contended spinning on their
> > local value.
>
> Well, the lock as a whole is contended (there's >1 waiters), and the
> point of MCS style locks it to make sure they're not actually pounding
> on the same cacheline. So the whole thing is consistent.
>
> > Perhaps you could add this comment.
> >
> > /* Once queue_spin_unlock is called (which _subtracts_ _Q_LOCKED_VAL from
> > the lock->val and still preserving the tail data), the winner gets to
> > claim the ticket.
>
> There's no tickets :/
s/ticket/be first in line/ ?
>
> > Since we still need the other CPUs to continue and
> > preserve the strict ordering in which they setup node->next, we:
> > 1) update lock->val to the tail value (so tail CPU and its index) with
> > _Q_LOCKED_VAL.
>
> We don't, we preserve the tail value, unless we're the tail, in which
> case we clear the tail.
>
> > 2). Once we are done, we poke the other CPU (the one that linked to
> > us) by writting to node->locked (below) so they can make progress and
> > loop on lock->val changing from _Q_LOCKED_MASK to zero).
>
> _If_ there was another cpu, ie. the tail didn't point to us.
<nods>
>
> ---
>
> I don't do well with natural language comments like that; they tend to
> confuse me more than anything.
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
