Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5693 closed enhancement (fixed)

Implement pthread spinlocks (easy)

Reported by: bonefish Owned by: nobody
Priority: normal Milestone: R1
Component: System/POSIX Version: R1/Development
Keywords: Cc: lucian.grijincu@…
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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

Attachments (3)

spin-lock.patch (3.3 KB) - added by lucian 9 years ago.
generic-spin-lock-implementation
spin-lock-no-double-test.patch (3.0 KB) - added by lucian 9 years ago.
spin-lock-trylock-EBUSY.patch (468 bytes) - added by lucian 9 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by 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?

Changed 9 years ago by lucian

Attachment: spin-lock.patch added

generic-spin-lock-implementation

comment:2 Changed 9 years ago by lucian

Cc: lucian.grijincu@… added

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

comment:3 in reply to:  2 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by 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.

Changed 9 years ago by lucian

comment:5 Changed 9 years ago by lucian

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

comment:6 Changed 9 years ago by bonefish

Resolution: fixed
Status: newclosed

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.

comment:7 Changed 9 years ago by 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.

Changed 9 years ago by lucian

comment:8 in reply to:  7 Changed 9 years ago by 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.

Note: See TracTickets for help on using tickets.