Re: [Xen-devel] [PATCH RFC v6 09/11] pvqspinlock, x86: Add qspinlock para-virtualization support

[Paolo Bonzini <pbonzini@xxxxxxxxxx>]

Il 13/03/2014 12:21, David Vrabel ha scritto:
> On 12/03/14 18:54, Waiman Long wrote:
> > This patch adds para-virtualization support to the queue spinlock in
> > the same way as was done in the PV ticket lock code. In essence, the
> > lock waiters will spin for a specified number of times (QSPIN_THRESHOLD
> > = 2^14) and then halted itself. The queue head waiter will spins
> > 2*QSPIN_THRESHOLD times before halting itself. When it has spinned
> > QSPIN_THRESHOLD times, the queue head will assume that the lock
> > holder may be scheduled out and attempt to kick the lock holder CPU
> > if it has the CPU number on hand.
> I don't really understand the reasoning for kicking the lock holder.
I agree.  If the lock holder isn't running, there's probably a good 
reason for that and going to sleep will not necessarily convince the 
scheduler to give more CPU to the lock holder.  I think there are two 
choices:
1) use yield_to to donate part of the waiter's quantum to the lock 
holder?    For this we probably need a new, separate hypercall 
interface.  For KVM it would be the same as hlt in the guest but with an 
additional yield_to in the host.
2) do nothing, just go to sleep.

Could you get (or do you have) numbers for (2)?

More important, I think a barrier is missing:

        Lock holder ---------------------------------------

        // queue_spin_unlock
        barrier();
        ACCESS_ONCE(qlock->lock) = 0;
        barrier();

        // pv_kick_node:
        if (pv->cpustate != PV_CPU_HALTED)
                return;
        ACCESS_ONCE(pv->cpustate) = PV_CPU_KICKED;
        __queue_kick_cpu(pv->mycpu, PV_KICK_QUEUE_HEAD);

                Waiter -------------------------------------------

                // pv_head_spin_check
                ACCESS_ONCE(pv->cpustate) = PV_CPU_HALTED;
                lockval = cmpxchg(&qlock->lock,
                                  _QSPINLOCK_LOCKED,
                                  _QSPINLOCK_LOCKED_SLOWPATH);
                if (lockval == 0) {
                        /*
                         * Can exit now as the lock is free
                         */
                        ACCESS_ONCE(pv->cpustate) = PV_CPU_ACTIVE;
                        *count = 0;
                        return;
                }
                __queue_hibernate();

Nothing protects from writing qlock->lock before pv->cpustate is read, 
leading to this:
        Lock holder                     Waiter
        ---------------------------------------------------------------
        read pv->cpustate
                (it is PV_CPU_ACTIVE)
                                        pv->cpustate = PV_CPU_HALTED
                                        lockval = cmpxchg(...)
                                        hibernate()
        qlock->lock = 0
        if (pv->cpustate != PV_CPU_HALTED)
                return;

I think you need this:

-       if (pv->cpustate != PV_CPU_HALTED)
-               return;
-       ACCESS_ONCE(pv->cpustate) = PV_CPU_KICKED;
+       if (cmpxchg(pv->cpustate, PV_CPU_HALTED, PV_CPU_KICKED)
+                       != PV_CPU_HALTED)
+               return;

Paolo

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