Add futex relative sleeps and wakeup counts - #846
Conversation
This commit is a refactor of some futex-related functionality in addition to adding some new functionality. Specifically: * `__wasilibc_futex_wait` now specifies a `__WASILIBC_FUTEX_TIMESPEC_ABSOLUTE` value for the `flags` argument which indicates whether the timespec specified is absolute or relative. * The `__wasilibc_futex_wake` API now returns the number of threads woken on success.qj * The `__wait.c` functions shared amongst targets is now in the `common` directory and is implemented in terms of `__wasilibc_futex_*`. Per-implementation functions previously implemented in terms of these functions are now migrated to internal implementations. This removed the unused `int op` in the wasi-threads implementation, for example. This is originally motivated by looking at Rust and seeking how the `wake` functionality wants to know if something was woken up, and additionally how the timeout there is specified as a relative duration. Additionally the wasi-threads backend only supports relative timeouts, and the coop-threads backend supports either relative or absolute, so it wasn't too hard to integrate.
This fixes a failing test with the previous futex refactoring which exposed two preexisting issues: * The coop `pthread_barrier_wait` implementation didn't handle reuse of the barrier and threads could accidentally get stuck. * Waking multiple threads blocked on a futex address would re-inspect the list one each iteration as opposed to dequeueing all threads first. The first bug was exposed when the previous refactoring changed a "wake all" loop from the wake_all function to the wake_one function, and the second bug is that multiple invocations of wake_one weren't correct. This commit fixes both of these issues by first fixing the underlying futex issue itself and then moving the musl-standard pthread barrier implementation to the shared implementation across all backends.
|
FWIW the barrier bits here are one of the reasons that I want to try to reuse the upstream musl primitives as-is b/c I find it quite tricky to get them all right, especially with "POSIX semantics aren't generally fully known by anyone". In that sense leaning on the battle-tested implementations is something I'd personally be the most comfortable with. If you agree with this PR @TartanLlama my hope is that's basically trivial to migrate all the other primitives into |
| if ((flags & ~__WASILIBC_FUTEX_TIMESPEC_ABSOLUTE) != 0) { | ||
| return -EINVAL; | ||
| } | ||
| if ((((intptr_t)addr) & 3) != 0) { |
There was a problem hiding this comment.
Should we document this alignment restriction?
There was a problem hiding this comment.
Oh this is a copy/paste from the wasi-threads implementation that I forgot to remove, but yeah might as well keep it everywhere and document it.
d368a4a to
7cdde10
Compare
2 participants
This commit is a refactor of some futex-related functionality in addition to adding some new functionality. Specifically:
__wasilibc_futex_waitnow specifies a__WASILIBC_FUTEX_TIMESPEC_ABSOLUTEvalue for theflagsargument which indicates whether the timespec specified is absolute or relative.__wasilibc_futex_wakeAPI now returns the number of threads woken on success.qj__wait.cfunctions shared amongst targets is now in thecommondirectory and is implemented in terms of__wasilibc_futex_*. Per-implementation functions previously implemented in terms of these functions are now migrated to internal implementations. This removed the unusedint opin the wasi-threads implementation, for example.This is originally motivated by looking at Rust and seeking how the
wakefunctionality wants to know if something was woken up, and additionally how the timeout there is specified as a relative duration. Additionally the wasi-threads backend only supports relative timeouts, and the coop-threads backend supports either relative or absolute, so it wasn't too hard to integrate.