feat: implement memory.atomic.notify,wait32,wait64

[haraldh]

Added the parking_spot crate, which provides the needed registry for the operations.

Tests include:

• wait32 doesn't block if the values don't match
• wait32 eventually times out if the values match
• wait32 traps on unshared memories
• notify returns 0 on unshared memories
• notify returns 0 with 0 waiters
• notify returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)
• wait32 and notify require in-bounds and aligned addresses

[bjorn3]

The rustix crate wasmtime already uses has futex support. memory.atomic.{notify,wait32,wait64} are modeled after the futex model afaik, not the thread parking model that parking_lot_core exposes. In addition parking_lot_core is modelled on top of futex on Linux. As such using rustix::thread::futex will likely require less code and be more efficient.

[haraldh]

@bjorn3
I had a futex solution here, but we need a solution for other OSes and AtomicU64, too.

[haraldh]

@bjorn3 see also this comment

[bjorn3]

Didn't think of wait64. That is indeed an issue.

[alexcrichton]

Thanks for the PR here! Would it be possible to add tests with this as well? I'd ideally like to grow the threads test suite to be as comprehensive as we can reasonably get it as features land.

Internally I think there's a few things about the wasm semantics to touch up. You mention i64 vs u64, and in these intrinsics you'll want to cast timeout: u64 to timeout: i64 since the spec says that it's treated as a signed number. Additionally I think you'll want to handle timeout == 0 differently where it looks like now that equates to now deadline but with the spec that equates to a zero deadline.

For error handling I think it's ok to avoid TrapReason::user_with_backtrace since all of the error conditions here should be infallible, so I think they'd best be replaced with a .unwrap() where necessary.

Finally, for the low level implementation, I personally do not want to use parking-lot internally here. I reviewed it historically and I don't think it's suitable for Wasmtime where any mistake is a security issue. I think it would be better to build this with condvars/mutexes/etc within the SharedMemory structure for state management. That furthermore removes the need for global state management here and additionally enables a more flexible implementation that takes into account async one day, if necessary.

[haraldh]

Implemented parking_spot as a parking_lot_core replacement.

[haraldh]

Err... SIGKILL is hard in the CI :)
edit: maybe a timeout killer?

[alexcrichton]

Looks reasonable to me, thanks!

One thing that we'll want to add is embedder APIs to interact with these instructions as well. For eample I think we'll want SharedMemory::{notify,wait32,wait64} which are basically the same wasm instructions but callable by wasmtime embedders, allowing programmatic interactions with threads as well. That doesn't need to be added necessarily in this PR, however, but it might motivate movement of the implementation of these instructions into wasmtime_runtime::SharedMemory instead of keeping it in libcalls in the long run.

[alexcrichton]

Oh, and to reiterate, this will require tests to merge.

[haraldh]

Oh, and to reiterate, this will require tests to merge.

@alexcrichton wouldn't that need threading support first, to test it in full action?

[alexcrichton]

It would indeed, and everything necessary should be implemented in Wasmtime today. There are, for example, already tests with threads.

[haraldh]

Addressed some of the issues, working on the others. Now tested with my real pthread example.

[haraldh]

So, I setup the aarch64 cross compile locally and it passes... just takes some time:

wasmtime/crates/parking_spot on  feature/atomic_wait_notify via 🦀 v1.65.0 took 42s 
[debian-toolbox:latest] ❯ cargo test --target aarch64-unknown-linux-gnu  --locked -p wasmtime-parking_spot --lib
   Compiling wasmtime-parking_spot v4.0.0 (/home/harald/git/wasmtime/crates/parking_spot)
    Finished test [optimized + debuginfo] target(s) in 0.78s
     Running unittests src/lib.rs (/home/harald/git/wasmtime/target/aarch64-unknown-linux-gnu/debug/deps/wasmtime_parking_spot-91a65de835bf666b)

running 13 tests
test tests::atomic_wait_notify ... ok
test tests::parking_lot::hundred_unpark_all_one ... ok
test tests::parking_lot::unpark_one_fifty ... ok
test tests::parking_lot::unpark_all_one ... ok
test tests::parking_lot::unpark_one_one ... ok
test tests::parking_lot::unpark_one_fifty_then_fifty_all ... ok
test tests::parking_lot::unpark_one_hundred_fast ... ok
test tests::parking_lot::unpark_one_one_fast ... ok
test tests::parking_lot::unpark_one_fifty_then_fifty_all_fast ... ok
test tests::parking_lot::unpark_all_hundred ... ok
test tests::parking_lot::unpark_all_hundred_fast ... ok
test tests::parking_lot::hundred_unpark_all_one_fast has been running for over 60 seconds
test tests::parking_lot::unpark_all_one_fast has been running for over 60 seconds
test tests::parking_lot::hundred_unpark_all_one_fast ... ok
test tests::parking_lot::unpark_all_one_fast ... ok

test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 98.13s

[haraldh]

Oh, and to reiterate, this will require tests to merge.

@alexcrichton added one basic test... more to follow

[alexcrichton]

The test you added looks good, thanks! I think it might be also good to perhaps have a small mutex-like implementation with 2-4 threads incrementing a shared counter perhaps and ensure that the final count is as expected as well.

Otherwise others tests to add would be the basic single-threaded operational semantics of these instructions in the face of no threads. You may want to check the test suite but IIRC it's not very comprehensive (and we're almost certainly not running it at this time). Anything missing there can be added as a manual *.wast test or similar. I'm thinking things like:

• wait32 doesn't block if the values don't match
• wait32 eventually times out if the values match
• wait32 traps on unshared memories
• notify returns 0 on unshared memories
• notify returns 0 with 0 waiters
• notify returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)
• wait32 and notify require in-bounds and aligned addresses

Much of this can be expressed with a *.wast test I think. It all should theoretically be covered by the upstream test suite but you'd need to check to make sure it is. I can help out with this if you're having trouble.

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[haraldh]

Looks reasonable to me, thanks!

One thing that we'll want to add is embedder APIs to interact with these instructions as well. For eample I think we'll want SharedMemory::{notify,wait32,wait64} which are basically the same wasm instructions but callable by wasmtime embedders, allowing programmatic interactions with threads as well. That doesn't need to be added necessarily in this PR, however, but it might motivate movement of the implementation of these instructions into wasmtime_runtime::SharedMemory instead of keeping it in libcalls in the long run.

added Shared::Memory::atomic_* methods.

[github-actions]

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

• fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[alexcrichton]

Ok this all looks good to me so I'm going to mark this for merging. Thanks again @haraldh for your work here!