Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support platforms without 64-bit atomics #10134

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

alexcrichton
Copy link
Member

This commit enables Wasmtime to build on platforms without 64-bit atomic instructions, such as Rust's riscv32imac-unknown-none-elf target. There are only two users of 64-bit atomics right now which are epochs and allocation of StoreId. This commit adds #[cfg] to epoch-related infrastructure in the runtime to turn that all of if 64-bit atomics aren't available. The thinking is that epochs more-or-less don't work without 64-bit atomics so it's easier to just remove them entirely.

Allocation of StoreId is trickier though because it's so core to Wasmtime and it basically can't be removed. I've opted to change the allocator to 32-bit indices instead of 64-bit indices. Note that StoreId requires unique IDs to be allocated for safety which means that while a 64-bit integer won't overflow for a few thousand years a 32-bit integer will overflow in a few hours from quickly creating stores. The rough hope though is that embeddings on platforms like this aren't churning through stores. Regardless if this condition is triggered it'll result in a panic rather than unsoundness, so we hopefully have at least that going for us.

Closes #8768

This commit enables Wasmtime to build on platforms without 64-bit atomic
instructions, such as Rust's `riscv32imac-unknown-none-elf` target.
There are only two users of 64-bit atomics right now which are epochs
and allocation of `StoreId`. This commit adds `#[cfg]` to epoch-related
infrastructure in the runtime to turn that all of if 64-bit atomics
aren't available. The thinking is that epochs more-or-less don't work
without 64-bit atomics so it's easier to just remove them entirely.

Allocation of `StoreId` is trickier though because it's so core to
Wasmtime and it basically can't be removed. I've opted to change the
allocator to 32-bit indices instead of 64-bit indices. Note that
`StoreId` requires unique IDs to be allocated for safety which means
that while a 64-bit integer won't overflow for a few thousand years a
32-bit integer will overflow in a few hours from quickly creating
stores. The rough hope though is that embeddings on platforms like this
aren't churning through stores. Regardless if this condition is
triggered it'll result in a panic rather than unsoundness, so we
hopefully have at least that going for us.

Closes bytecodealliance#8768
@alexcrichton alexcrichton requested review from a team as code owners January 28, 2025 15:23
@alexcrichton alexcrichton requested review from dicej and removed request for a team January 28, 2025 15:23
@alexcrichton
Copy link
Member Author

On a more abstract note this is part of my quest to "let's port everything everywhere" so I don't have anything specific in mind for the use case here. Instead it's more of "maybe others can do interesting things with this". The bit about StoreId I'm not particularly happy with because it's something where your tiny embedded device will crash after days online and you'll have no way to easily reproduce unless you can easily debug it. That seems like a pretty bad silent failure mode but at the same time I'm not sure what else to do...

Mostly saying that I'm not 100% sold this is a good idea, but I also don't know of a different way to run without 64-bit atomics right now.

@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:docs Issues related to Wasmtime's documentation labels Jan 28, 2025
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this tradeoff feels a little unsavory but I can't think of a better alternative. We don't want to refuse to support a platform at all because one of the guarantees wasmtime provides isn't well suited to a particularly intense corner case on that platform.

@alexcrichton alexcrichton added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
These aren't intended to be used in threaded contexts anyway and the use
of `AtomicXXX` is just to have interior mutability while still being
`Send` and `Sync`. Switch to using `AtomicU32` which is more portable.
@alexcrichton
Copy link
Member Author

Ah I forgot a usage in component model resources. The usage there is only intended to have interior mutability and still be Send/Sync so this updates the type to using two AtomicU32 values. No atomicity is required in this use case since bigger bugs are afoot if there are concurrent reads/writes to this value. The only thing we're trying to prevent is UB, which two AtomicU32 values work just fine for.

@hanna-kruppe
Copy link

hanna-kruppe commented Jan 28, 2025

Would a lock protecting a (non-atomic) u64 would be an option for StoreId? Of course in a general no_std context this would have to be a spinlock, which is bad for all sorts of reasons, but perhaps slightly better than the failure mode of running out of StoreIds after a few days of normal operation.

@alexcrichton
Copy link
Member Author

Yeah if std could be relied on I'd replace it with a Mutex<u64> on non-64-bit platforms. Personally I'd prefer to avoid spin locks or panic-on-contention for something so core as creating a new store. The failure mode here specifically is creating many Store<T> instances and destroying them, where eventually that'll panic. I'm not sure if low-end devices that don't have 64-bit atomics will be doing since since I'd otherwise suspect that they'd probably create a small handful of stores (if more than 1) and use those for their lifetime.

Overall I don't know what the best tradeoff is. As-is Store<T>::new will panic if called rapidly. With a spin-lock you run the risk of long pauses if you get preempted while the lock is held. With panic-on-contention you risk periodic panics if there are multiple threads/actors in the system. None seem like a great option :(

@alexcrichton alexcrichton added this pull request to the merge queue Jan 28, 2025
@cfallin
Copy link
Member

cfallin commented Jan 28, 2025

Slightly wild idea inspired by number theory that might be close to working:

If we have two atomic u32s, and we increment them by relatively prime amounts (say, 3 and 5) that are also relatively prime to the modulus of the ring we're operating in (2^32), then the periodicity of the pair of them is the product of the periodicity of each. In other words, we walk through 2^64 possible tuples. They're not contiguous in ordinary numeric order but we don't need that -- only uniqueness.

The slight wrinkle is that if we do the atomic increments separately, the two halves of the tuple sequence can "slide" relative to each other -- and this reduces the periodicity / can lead to repeats. I think one can fix that by incrementing the first, the second, then the first again, and using the 3-tuple. My intuition is that that bounds the "slide" / recovers uniqueness, but I'm having some trouble thinking through it at the moment. Maybe it inspires someone else though?

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@cfallin
Copy link
Member

cfallin commented Jan 28, 2025

Ah, OK, here is something that should work. It's not constant-time (it can retry) but it is non-blocking (in the lockless programming sense): there are no spinloops.

struct PairCounter { x: AtomicU32, y: AtomicU32 }

impl PairCounter {
  fn next(&self) -> (u32, u32) {
    loop {
      let x = self.x.fetch_add(3, Ordering::SeqCst);
      let y = self.y.fetch_add(5, Ordering::SeqCst);
      let x_ = self.x.load(Ordering::SeqCst);
      if x.wrapping_add(3) == x_ { // Common case: no one else bumped self.x in the meantime.
        return (x, y);
      }
    }
  }

  fn next_u64(&self) -> u64 {
    let (x, y) = self.next();
    (u64::from(x) << 32) | u64::from(y)
  }
}

relaxing the SeqCst to acquire/release left as an exercise to the (smarter than me) reader...

@alexcrichton
Copy link
Member Author

@cfallin would you feel confident enough in that to have it be the default algorithm? From a speed perspective I'm not too worried about that being significantly slower than the current allocation (I'd expect Store<T> allocation in general to dwarf the atomic ops). What I would be worried about though is that if something as complex as that is not on-by-default then we won't catch any possible issues with it

@cfallin
Copy link
Member

cfallin commented Jan 28, 2025

Maybe? I'm by no means a niche expert in lockless algorithms -- I can try to sketch a correctness proof though. I don't want to block your work here so by all means land with 32-bit atomics and the (unfortunate but best option) "time bomb" panic and I can do this as followup.

@alexcrichton
Copy link
Member Author

Nah makes sense, but on reflection I've switched this to using crate::sync::RwLock<T> which is basically "put a lock around it". Our no_std locks currently all panic on contention under the assumption that you probably only have on thread in no_std contexts. In any case this is no worse than what Wasmtime had before this change and it's arguably more correct with respect to StoreId. This means that we'll still preserve 64-bit uniqueness guarantees and it'll just be a bit slower without 64-bit atomics.

If the panic-on-contention bit becomes a problem then it's already one we have to solve anyway, so this way if that's a problem we still only have one problem to solve as opposed to two.

@alexcrichton alexcrichton added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@alexcrichton alexcrichton added this pull request to the merge queue Jan 29, 2025
Merged via the queue into bytecodealliance:main with commit 10eda1c Jan 29, 2025
39 checks passed
@alexcrichton alexcrichton deleted the riscv32 branch January 29, 2025 16:14
MarinPostma pushed a commit to MarinPostma/wasmtime that referenced this pull request Feb 3, 2025
* Support platforms without 64-bit atomics

This commit enables Wasmtime to build on platforms without 64-bit atomic
instructions, such as Rust's `riscv32imac-unknown-none-elf` target.
There are only two users of 64-bit atomics right now which are epochs
and allocation of `StoreId`. This commit adds `#[cfg]` to epoch-related
infrastructure in the runtime to turn that all of if 64-bit atomics
aren't available. The thinking is that epochs more-or-less don't work
without 64-bit atomics so it's easier to just remove them entirely.

Allocation of `StoreId` is trickier though because it's so core to
Wasmtime and it basically can't be removed. I've opted to change the
allocator to 32-bit indices instead of 64-bit indices. Note that
`StoreId` requires unique IDs to be allocated for safety which means
that while a 64-bit integer won't overflow for a few thousand years a
32-bit integer will overflow in a few hours from quickly creating
stores. The rough hope though is that embeddings on platforms like this
aren't churning through stores. Regardless if this condition is
triggered it'll result in a panic rather than unsoundness, so we
hopefully have at least that going for us.

Closes bytecodealliance#8768

* Update component resources to not use `AtomicU64`

These aren't intended to be used in threaded contexts anyway and the use
of `AtomicXXX` is just to have interior mutability while still being
`Send` and `Sync`. Switch to using `AtomicU32` which is more portable.

* Use `RwLock<u64>` for `StoreId` instead.

* Fix compile

* Fix imports
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

riscv32imac no-std support for wasmtime
4 participants