MMU-based epoch interruption#12990
Conversation
github-actions
Bot
added
cranelift
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
DetailsTo modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
071bbba to
21863e6
Compare
928d774 to
4a42689
Compare
Compared to emitting raw load instructions, this gives us more convenient hooks from which to track the locations and lengths of dead-load instructions. It also lets us idiomatically reserve the specific registers we need. * Add `mem_flags_aligned_read_only` helper so we can construct aligned-and-read-only `MemFlags`es in ISLE. It currently expresses alignedness but not read-only-ness. I believe it correct like this though obviously not as constrained as it could be. * Add a spot in `MachBuffer` for tracking dead-load instruction locations. This will let a signal handler (in a later commit) distinguish between signals thrown by these instructions and ordinary crashes.
… the interrupt.
Add `mmu_interrupt_page_ptr` field to `VMStoreContext` to point to that Store's interrupt page. Because the only instantiation of `VMStoreContext` is in the course of instantiating a `StoreOpaque`, a decent place to dispose of it is in `impl Drop for StoreOpaque`.
Allocate the page from `Store::new` when the config flag is on.
Add `MmuInterrupter`: a `Send + Sync` handle that lets an outside thread flip the interrupt page's protection. This is the MMU-based analogue to `Engine::increment_epoch`.
Finally, update `disas` test results.
These are all just 8-byte offset increases due to adding the interrupt page ptr field. This script strips out all the obviously okay parts of the diff, making the review easier (and the script is hopefully fairly easy to review, too):
```python
"""Compare runs of - and + blocks of a diff, and assert that the only
differences between them are differences in hex and decimal numbers therein.
Further, assert that those differences are a rise of 8, representing the size of
the field I added.
Output the diff with the proven-correct regions resolved in favor of the +
lines. Any remaining diff lines are suspicious and should be manually examined.
"""
import re
from sys import argv
def is_diff_line(s, plus_or_minus):
return bool(re.match(r"^ +" + "\\" + plus_or_minus, s))
def is_minus_line(s):
return is_diff_line(s, "-")
def is_plus_line(s):
return is_diff_line(s, "+")
def check_line_pairs(file_path):
with open(file_path, 'r') as file:
lines = file.readlines()
i = 0
while i < len(lines):
if is_minus_line(lines[i]):
minus_block = []
while i < len(lines) and is_minus_line(lines[i]):
minus_block.append(lines[i])
i += 1
plus_block = []
while i < len(lines) and is_plus_line(lines[i]):
plus_block.append(lines[i])
i += 1
if len(minus_block) != len(plus_block):
print(" + BLOCK LENGTHS DIFFERED.")
print("".join(minus_block))
print("".join(plus_block))
continue
# Compare the two blocks line by line
for line1, line2 in zip(minus_block, plus_block):
# Extract numbers (both decimal and hexadecimal) from both lines
numbers1 = [int(num, 16) if num.startswith("0x") else int(num)
for num in re.findall(r'0x[0-9a-fA-F]+|\d+', line1)]
numbers2 = [int(num, 16) if num.startswith("0x") else int(num)
for num in re.findall(r'0x[0-9a-fA-F]+|\d+', line2)]
# Check if the numbers differ by 0 or 8
if len(numbers1) == len(numbers2) and all(n2 - n1 in (0, 8) for n1, n2 in zip(numbers1, numbers2)):
# It's just an increment (or nothing), so keep the new line:
print(re.sub(r"^( +)\+", r"\1 ", line2), end="")
else:
print(line1, end="")
print(line2, end="")
else:
print(lines[i], end="")
i += 1
check_line_pairs(argv[1])
```
These prod the interrupt page, causing an actual interruption if the page is protected. Cache the interrupt page ptr in a local for speed, as we did with the epoch deadline. Here is how I interpret the generated code in mmu-interruption.wat: ``` ;; Skip over magic number (4b) and alignment (another 4b): ;; @001B v2 = load.i64 notrap aligned readonly can_move v0+8 ;; Get interrupt page ptr: ;; @001B v3 = load.i64 notrap aligned v2+16 ;; Read from page ptr: ;; @001B v4 = load.i32 aligned readonly v3 ```
Add a `.wasmtime.mmu_interrupt_checks` section to the emitted ELF. This keeps track of the the locations of `dead_load_with_context` instructions in the binary so the eventual signal handler will know (1) which segfaults indicate purposeful interruption points and (2) how far afterward to resume later (based on instruction length).
Add a block to the signal handler to recognize segfaults caused by interrupt checks. So far, this bounces to an asm trampoline that just jumps back to where the interrupted Wasm left off, without any yielding. The interrupt page doesn't get unprotected yet either.
* Unprotect the interrupt page once we've switched tasks. * Add a test to show stack unwinding works when a fiber is cancelled. Asm trampoline cribs generously from https://github.com/cfallin/wasmtime/blob/f6476d3174e0ffbe59f807385b5518691eeacffd/crates/wasmtime/src/runtime/vm/traphandlers/inject_call/x86_64.rs#L11.
We still default to deadline-based epochs, but, if `epoch-interruption-via-mmu` is explicitly turned on, use it instead. Same for `--profile guest`.
This commit performs validation of the invariants that need to be met in order to use mmu-based interruption in Wasmtime. Specifically, mmu based interruption requires: * Signals based traps + native signals * Async support * A Linux+x86_64 host
These were a lot of undefined-symbol errors, unused-import errors, and an unread-field error. Factor up the rather large collection of features and target attributes that MMU interruption requires into a `has_mmu_interruption` cfg flag. Use it in some new spots as well.
That requires its invocation in the cli-flags crate to be conditional, and there's not a terrifically clean way of expressing the `mmu-interruptions` custom_cfg flags there.
…t compiled in. Now that MMU-interrupt machinery just isn't around when its feature prerequisites are absent, avoid calling it.
|
@erikrose do you feel this is landable as-is modulo review? I see the review request now and wanted to confirm. If so, before going too deep into this, have you done performance testing in the contexts of where this is expected to provide a benefit? |
|
Yes. I'm about to commit a more in-depth comment on The perf testing is interesting: it should bench out the same as when we did it before on the no-interrupt path (14.4% overhead for epochs, 2.8% for this). The yes-interrupt path is probably going to depend on how fully loaded the machine is: things like TLB shoot-downs interacting with multiple cores, all furiously running Wasm guest code. My thinking was to get it landed upstream, update the version of wasmtime we're using, and then try the new flag in some canary contexts to get real-world numbers. Of course, I'm happy to do Sightglass benchmarks of the yes-interrupt path at various intervals (every .1ms, 1ms, 10ms) and see how those compare, though it'll require some coding. Sightglass is better than nothing, and I was probably going to do that anyway just in case huge surprises come out of it. If you want to gate landing on that, it's fine with me. But I don't think it needs to hold up review. I probably should have led with this, but we're having Saúl help us out for a bit, and he's going to take a look over the next day or two before he becomes unavailable for a span. @saulecabrera Maybe you want to claim review? I'll let you guys fight it out. Thanks! |
|
I'm happy to look at the Cranelift side of this. (And Alex should definitely look at the runtime side...) On first skim, I think the overall shape of the "dead load" seems fine, but a few style and design comments:
|
Also remove commas making a few other sentences masquerade as compound.
| if self.tunables().mmu_interruption { | ||
| use target_lexicon::{Architecture, OperatingSystem}; | ||
|
|
||
| if !matches!( |
There was a problem hiding this comment.
I just realized that you had introduced has_mmu_interruption, I believe this can be replaced with
if !cfg!(has_mmu_interruption) { ... }This totally my fault for not digging deeper here.
With this change, I think we can get rid of the host.architecture check.
There was a problem hiding this comment.
I think this approach has the added advantage that it checks for async support, which my previous comment states, but does not check at the engine level.
There was a problem hiding this comment.
Nope, I introduced that after your commit! But thanks for noticing; I'll have a look.
@cfallin Oh, sorry; I misinterpreted your "we can add to the module metadata, produced alongside e.g. trap codes" to mean a new piece of metadata was needed. However, it may yet be so: it turned out I needed not only to flag the interruption-check instruction offsets but also their lengths so I could compute where to resume after the interruption. The x64 So I see a few possibilities:
What's your favorite? Numbers 2 and 3 have the advantage of glomming onto the LEB and delta-compression fanciness of trap codes, which I wasn't aware of until just now. My leaning is toward 2. |
|
I don't like any of those options actually -- and it gets at my general concern with the way that this change conflates abstractions from a number of logically-separate places in a way that makes things really brittle. The proposed solutions are (i) x64-specific, and (ii) combine distributed knowledge from instruction emission and regalloc into an invariant encoded in the trap table and then used by the signal handler (!!). The alternative is to have a very specific and weird "MMU interruption" concept in Cranelift. A compiler should not know what MMU interruption is; it understands loads, stores, and instruction metadata. Let me ask more deeply: why do we need to know about the instruction encoding at all, to advance past the instruction? Why can't we resume into the guest and let it redo the load, with the page mapped back in? Unless we burn a virtual address on every interruption and never reuse it, it will have to be mapped back in eventually, so I don't think that's an issue? If we do need to resume past the load, then we should not bake assumptions about possible lengths into the compiler. So "one trap code for 3 bytes, one for 4 bytes" is a bad idea: what happens when we get a new architecture that requires 5 bytes? Or add an optimization to the x64 backend to encode it in 2 bytes? Or ... Instead what you're really trying to build is the concept of resumable traps. So in the trap table, we have a kind of entry that indicates "resumable" and gives the PC-offset to correct the captured PC by. One could encode that with a sparse array alongside trap codes. That might end up looking something like your current custom section, but named in a much less confusing and specific way, and reusable for any other purpose that requires resumable traps in the future (e.g. hardware debug-break opcodes, or missing-instruction emulation, or ...). So: I prefer that we don't build a distributed, brittle invariant that is ISA-specific like this; but if we have to, let's reify the concept as a more fundamental thing. |
|
Personally I think it would be best to prioritize performance numbers for this. I agree with everything @cfallin is saying review-wise, but I think it would be best to get on the same page about the overall shape of this feature first as well since that will inform the implementation which will have knock-on effects on what will be reviewed. Of the two possibilities here -- flipping virtual memory permissions vs having an indirect load that is dynamically switched to null -- there's still quite a lot of shared implementation/mechanisms between the two, but I also feel they're different enough we should try to settle on one before landing. I personally have a strong hunch that what's implemented here, flipping virtual memory permissions, will be a significant performance regression over today's implementation of epochs, specifically because there are more TLB shootdowns (repeatedly unmapping previously-mapped pages). I understand that getting performance numbers on this isn't easy, and I also understand that it would be easiest to land everything here, gated, and then get performance numbers. This is a significant change, however, and I believe we're going to want more confidence before landing it, even gated. The performance numbers that I'm specifically interested in is the performance of a multi-threaded program with wasm execution in a lot of threads. I'd like to see the performance effect of flipping pages being accessible/inaccessible for the running wasms. This would end-to-end exercise the Cranelift bits here, how the page flipping is implemented, how the list of pages to flip are managed, etc. In-repo the closest equivalent to this is benchmarking One thing I'll also explicitly say is that I'm specifically not too interested in the single-threaded overhead of epochs nor the impact of virtual mapping changes in a single-threaded program. I understand some benchmarks were done, but Sightglass does not exercise anything related to TLB shootdowns so it's the wrong benchmark corpus for what I'm interested in. While it's good to provie that this has lower overhead than epochs that's also pretty naturally expected given the reduction in the size of the generated code. The specific concerns I have lie in the TLB shootdown behavior, which surface only in a multithreaded environment. |
|
@cfallin First, thank you for bringing your wide and lengthy wasmtime perspective to bear on this!
We could, if we don't mind making resumption 1 instruction more expensive. It's the cold path, but I'd be lying to claim knowledge of how significant this would be. Actually, it should be pretty easy to get performance numbers—decent ones, if I make There remains, however, a pathological case which troubles me:
This loop could happen indefinitely, hopefully unlikely but conceivably not, depending on how many fibers are competing, their scheduling algorithm, and how quickly the interrupter thread (in which I envision some adjustable sleep interval) spins. The epoch-deadline approach doesn't have this problem; it always makes forward progress.
That's only if we want to represent MMU interrupt locations as trap codes, of course. Otherwise, we have complete freedom to express instruction length in any kind of forward-compatible, ISA-agnostic way. Your suggestion to get aarch64 going before merging has the advantage of ferreting out these sorts of ISA-specific assumptions. So chalk up one in its column.
I appreciate your reimagining of this as a future-looking resumable-traps mechanism, and I'm intrigued. To clarify, are you proposing to add the sparse array of lengths to the trap section or to bail out into a separate section after all to avoid further complicating traps? How do you and @alexcrichton feel about this course of action?
(In this plan, I'm leaving a few of your earlier comments on the table until we have an idea of multithreaded numbers.) |
|
That sounds good to me. Personally I wouldn't be too concerned about live-locking of sorts with resuming and always faulting at the load-that-traps. I also don't think that the performance of executing a load twice will be noticable at all. Which is to say, I would agree with @cfallin that the initial state here is to run the load again. In the absolute worst case if live-locking is a problem in practice I'd prefer to go the route of rewriting the address the dead-load instruction loads from to be known-valid during resumption since that's easier to do cross-platform than skipping instructions. |
4 participants
This is an implementation of #1749, specifically @cfallin's roadmap, with the goal of reducing the overhead of checking for the end of epochs.
Paul ran some benchmarks on this (broadly agreeing with our real-world experiments) which tell us:
-Wepoch-interruption=yis a 14.4% hit versus doing nothing.The above numbers are from SpiderMonkey, which I deem the most representative benchmark.
Status:
DeadLoadWithContextCranelift instruction, and use it.DeadLoadWithContextemit metadata into compiled-artifact tables to tell the signal handler this is an interruption-point load.Polishing tasks:
--epoch-interruption-via-mmuis off.wasmtime rundo MMU interrupts afterWasmOptions::timeout, just as currently happens with classic epoch-interruption.--epoch-interruption-via-mmureport an error on unsupported platforms.If the TLB shootdown arising from the frobbing of privs on the "interrupt page" proves too expensive, we can try a more indirect load instead, where, instead of messing with page privs, we mess with the address we're dead-loading from so it points to either a (statically) allowed or forbidden page. (Chris floated this idea at the 2026-04-08 Cranelift meeting.) Not many of the other mechanics need change.