Implement pthread spinlocks (easy)

[bonefish]

Implement the pthread_spin_*() functions. pthread_spinlock_t can e.g. be typedefed to { int32_t lock }.

[Barrett]

i'm working on it (since seems there's no any guy working on, and tomorrow i'll have some time free), what is the directive implenting the functions? looking at some code on internet every system implements it in various modes. Anyway at a first look on the pthread_mutex.c file the semaphores are used, is it the right way?

[lucian]

generic-spin-lock-implementation

[lucian]

Barrett, sorry for not posting this earlier. I've finished a generic implementation, and am working on making platform dependent implementations.

[bonefish]

Replying to lucian:

Barrett, sorry for not posting this earlier. I've finished a generic implementation, and am working on making platform dependent implementations.

• I don't see much value in platform specific implementations.
• pthread_spin_lock()/pthread_spin_trylock(): The slock->lock == SLOCKED check is superfluous. It just adds overhead in the likely case that the lock in not locked (spinlock are usually chosen when low lock contention is expected).
• The coding style encourages readable identifiers: I.e. either LOCKED/UNLOCKED or SPINLOCK_LOCKED/SPINLOCK_UNLOCKED.

[lucian]

a) There's no much value, just a lower footprint (size/speed): I can skim off a few call/ret instructions, and code it using better optimized asm.

b) The test is not really that superfluous. Hammering the processor with instructions with LOCK prefixes (as in atomic test and set) will block the memory lines for two cycles. In single processor mode this will delay access to DMA, while in multiprocessor setups this will delay all other processors too regardless of the addresses used.

Ok, the _trylock one is not really that important as it is done only once, but the one in the loop will lead to poorer performance.

See ​http://en.wikipedia.org/wiki/Test_and_Test-and-set

I'll run some tests to prove my claims.

c) I'll post a revised patch with LOCKED/UNLOCKED.

[lucian]

Seems I spoke too soon. Posted an updated patch addressing your comments. Will add some unit tests too.

[bonefish]

Thanks! Patch applied in hrev36333 with minor changes:

• atomic_test_and_set() takes an int32*, which is not the same as an int32_t* (due to BeOS compatibility). Added a cast to suppress a warning.
• Removed blank lines where there were more than two.

Regarding the architecture specific implementation: The performance and size gains will be minimal. I don't think they outweigh the additional source complexity (one trivial generic C implementation vs. several architecture specific assembly implementations).

Regarding unit tests: The Open POSIX Test Suite (src/tests/system/libroot/posix/posixtestsuite/) contains a few spinlock tests. They can be run via POSIX_TARGET=conformance/interfaces/pthread_spin* make run-tests.

[lucian]

Something's wrong, but I can't figure it out and I don't want to be chasing wild geese. The implementation you submitted fails one test: pthread_spin_trylock seems to return -EBUSY instead of the expected EBUSY.

In code I have return EBUSY;, but the value returned to the user is equal to the user's -EBUSY. I may have a different EBUSY definition than the library's user. Am I including the wrong headers, or forgot to define something?

Run POSIX_TARGET=conformance/interfaces/pthread_spin_tryunlock make run-tests to see what I mean.

[bonefish]

Replying to lucian:

Something's wrong, but I can't figure it out and I don't want to be chasing wild geese. The implementation you submitted fails one test: pthread_spin_trylock seems to return -EBUSY instead of the expected EBUSY.

In code I have return EBUSY;, but the value returned to the user is equal to the user's -EBUSY. I may have a different EBUSY definition than the library's user. Am I including the wrong headers, or forgot to define something?

Since (due to BeOS compatibility) Haiku's error codes are negative while POSIX demands positive values, the test suite is built with -lposix_error_mapper -DB_USE_POSITIVE_POSIX_ERRORS. The define makes all error code constants positive, the library provides wrappers for all functions that return or deal with error codes. I added the spinlock function wrappers in hrev36334. If you have built the test suite before that revision, +/- problems are to be expected.