[Xen-devel] Re: Xen spinlock questions

["Jan Beulich" <jbeulich@xxxxxxxxxx>]

>>> Jeremy Fitzhardinge <jeremy@xxxxxxxx> 05.08.08 20:09 >>>
>Jan Beulich wrote:
>> Later yesterday I noticed another issue: The code setting lock_spinners
>> isn't interruption safe - you'll need to return the old value from
>> spinning_lock() and restore it in unspinning_lock().
>>   
>
>Good catch.

More on that: You'll really need two per-CPU variables afaics, one for the
current non-irq lock being spun upon, and one for the current irqs-disabled
one. The latter one might not need saving/restoring as long as you don't
re-enable interrupts, but the code might turn out cleaner when doing the
save/restore regardless, e.g. for me (doing ticket locking):

int xen_spin_wait(raw_spinlock_t *lock, unsigned int token)
{
        struct spinning *spinning;
        int rc;

        /* if kicker interrupt not initialized yet, just spin */
        if (spinlock_irq < 0)
                return 0;

        /* announce we're spinning */
        spinning = &__get_cpu_var(spinning);
        if (spinning->lock) {
                BUG_ON(!raw_irqs_disabled());
                spinning = &__get_cpu_var(spinning_irq);
                BUG_ON(spinning->lock);
        }
        spinning->ticket = token >> TICKET_SHIFT;
        smp_wmb();
        spinning->lock = lock;

        /* clear pending */
        xen_clear_irq_pending(spinlock_irq);

        /* check again make sure it didn't become free while
           we weren't looking  */
        rc = __raw_spin_trylock(lock);
        if (!rc) {
                /* block until irq becomes pending */
                xen_poll_irq(spinlock_irq);
                kstat_this_cpu.irqs[spinlock_irq]++;
        }

        /* announce we're done */
        spinning->lock = NULL;

        return rc;
}

>> Also I'm considering doing it ticket-based nevertheless, as "mix(ing) up
>> next cpu selection" won't really help fairness in xen_spin_unlock_slow().
>>   
>
>Why's that?  An alternative might be to just wake all cpus waiting for 
>the lock up, and then let them fight it out.  It should be unusual that 
>there's a significant number of waiters anyway, since contention is 
>(should be) rare.

On an 8-core system I'm seeing between 20,000 (x86-64) and 35,000
(i686) wakeup interrupts per CPU. I'm not certain this still counts as rare.
Though that number may go down a little once the hypervisor doesn't
needlessly wake all polling vCPU-s anymore.

>The main reason for ticket locks is to break the egregious unfairness 
>that (some) bus protocols implement.  That level of fairness shouldn't 
>be necessary here because once the cpus fall to blocking in the 
>hypervisor, it's up to Xen to tie-break.

Why? The hypervisor doing the tie-break makes it possibly even more
unfair, whereas with tickets and a way to kick the next owner (and only
it) almost the same level of fairness as on native can be achieved. The
only heuristic to determine by measurement is the number of spin loops
before going into poll mode (which your original patch's description and
implementation for some reason disagree about - the description says
2^16 loops, the implementation uses 2^10). Obviously, the optimal
numbers may turn out different for byte and ticket locks.

And doing a wake-them-all approach isn't good here, as was
(supposedly, I wasn't there) explained in a talk on the last summit.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel