562687 - VMPI_lockAcquire should probably use SwitchToThread rather than Sleep

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Simon Wilkinson]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: 


Copied over from bug 555760:

--- Comment #6 from Stan Switzer <[e-mail]> 2010-04-29 10:02:12 PDT ---

I was looking at (possibly not the latest) version of windows spinlocks and a
couple of comments:

VMPI_lockAcquire should probably should use SwitchToThread rather than Sleep
(but maybe not on CE).

Also, typically a spin-lock would check if it's on an MP machine and if so
briefly spin in a loop before yielding to the OS. When spin locks are being
used correctly (for very brief spans of code) this will be much much faster.
You needn't worry overmuch about pipeline stalls from the tests; after all
wasting time is pretty much the whole point.


Reproducible: Always

Comment 1

[Stan Switzer]

I should add that the reason is that SwitctToThread avoids priority inversion problems where Sleep will not.

Comment 2

[Simon Wilkinson]

(In reply to comment #1)
> I should add that the reason is that SwitctToThread avoids priority inversion
> problems where Sleep will not.

Additionally, for x86, a pause instruction should be inserted in every iteration of a spin-wait loop. Otherwise some HT-enabled cpus really suffer from attempting clean-up memory ordering violations. It's a nop on cpus that do not need it.

For example, the inner spin-wait loop of the safepointing system looks something like this:

if (VMPI_processorQty() > 1) {
    while(mutator) {
        int spinCount = 0;
        while (mutator->isClear()) {
            if (++spinCount == SAFEPOINT_SPINS) {
                VMPI_threadYield();
                spinCount = 0;
            }
            #if defined(AVMPLUS_IA32)
            else {
                #if defined(__GNUC__)
                    __asm__("pause");
                #elif defined(_MSC_VER)
                    _asm {pause}
                #else
                    #error "Not implemented"
                #endif
            }
            #endif
        }
        mutator = mutator->m_listNext;
    }
} else {
    while(mutator) {
        while (mutator->isClear()) {
            VMPI_threadYield();
        }
        mutator = mutator->m_listNext;
    }
}

Note that it is well behaved with regards to uniprocessors (using VMPI_processorQty() from bug 611232), and uses the new non-priority-inverting VMPI_threadYield() from bug 609837.

Spin-locks should do the same.

Comment 3

[Andre Kruetzfeldt]

Krzys, should it be moved to Cyril?

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.