add futex semaphore support derived from hutchinson port #50
Conversation
| #elif USE_FUTEX_SEM | ||
| #include <sys/syscall.h> | ||
| #include <linux/futex.h> | ||
| #endif |
There was a problem hiding this comment.
this block is duplicated (249-254)
| #define DISPATCH_FUTEX_NUM_SPINS 100 | ||
| #define DISPATCH_FUTEX_VALUE_MAX INT_MAX | ||
| #define DISPATCH_FUTEX_NWAITERS_SHIFT 32 | ||
| #define DISPATCH_FUTEX_VALUE_MASK ((1ull << DISPATCH_FUTEX_NWAITERS_SHIFT) - 1) |
There was a problem hiding this comment.
also these last 3 should be next to the structure definition
There was a problem hiding this comment.
please still move them next to the definition of dispatch_futex_t
|
ok I know this is a lot of comments, but despite that I love where this is going. I have not completely checked that the futex() based semaphore implementation is correct, and only made remarks for the barriers (which aren't important on intel but could be for other architectures). Note that dispatch assumes little endian in a LOT of places so I'm not entirely sure it's useful to test, but why not. Last, as far as atomic barriers go, for a semaphore, the semantics is that:
This is why there is no need for a barrier at all right before the call to futex() in the wait_slow because the sub2o done on the way out has one and that is enough (of course on Intel it has no impact anyway) I'm done with the first round of reviews |
| if (dispatch_atomic_cmpxchg2o(dfx, dfx_data, orig, orig - 1, acquire)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
this code should be:
uint64_t orig = dispatch_atomic_load2o(dfx, dfx_data, relaxed);
while (orig & DISPATCH_FUTEX_VALUE_MASK) {
if (dispatch_atomic_cmpxchgv2o(dfx, dfx_data, orig, orig - 1, &orig, acquire)) {
return true;
}
}
return false;
|
sorry I had missed the extra commit, wasn't notified, I start to believe either some notification go to my spam box or github has a problem. anyway made extra comments but that is more like what I had in mind indeed! |
| @@ -716,6 +785,10 @@ _dispatch_thread_semaphore_create(void) | |||
| int ret = sem_init(s4, 0, 0); | |||
| DISPATCH_SEMAPHORE_VERIFY_RET(ret); | |||
| return (_dispatch_thread_semaphore_t)s4; | |||
| #elif USE_FUTEX_SEM | |||
There was a problem hiding this comment.
not reall related to that very code but:
_dispatch_get_thread_semaphore and _dispatch_put_thread_semaphore are meant to amortize the cost of the syscall to create a kernel semaphore.
on linux where a futex is used, caching the malloc() is not useful, as we allocate a continuation which has its own cache.
_dispatch_put_thread_semaphore and _dispatch_get_thread_semaphore should be the same as dispose and create when USE_FUTEX_SEM is in use like this:
#if USE_FUTEX_SEM
#define _dispatch_get_thread_semaphore() _dispatch_thread_semaphore_create()
#define _dispatch_put_thread_semaphore() _dispatch_thread_semaphore_dispose()
#else
DISPATCH_ALWAYS_INLINE
static inline _dispatch_thread_semaphore_t
_dispatch_get_thread_semaphore(void)
{
...
}
#endif
|
OK .. in case the commit got lost .. |
|
oooh that's it, I don't get commit notifications. doh... I'm ok with this, can you please squash this in a single commit & force push and then I'll merge |
|
done .. can you see it ? |
|
nope I still see 3 commits. you have to update the pull request one way or the other, I assumed that a force push on your repository should do it naturally but maybe not. |
|
Damned .. 2023 git checkout master what's wrong with this ? |
|
|
|
Hmm none of that works .. all tell me its upto date. |
add futex semaphore support derived from hutchinson port
No description provided.