Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

mpk: optimize layout of protected stripes, again #7622

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Nov 30, 2023

This is another attempt at #7603, attempting reduce the slab layout sizes of MPK-protected stripes. While experimenting with the limits of MPK-protected memory pools, @alexcrichton and I discovered that the current slab layout calculations were too conservative. This meant that the memory pool could not pack in as many memories as it should have been able: we were expecting, but not seeing, ~15x more memory slots over non-MPK memory pools.

This change brings together several fixes:

  • it more aggressively divides up the stripes (as in b212152)
  • it eliminates an extra stripe (as in 8813a30)
  • it replaces some uses of checked_* with saturating_* (as in fb22a20)
  • it improves some documentation
  • and, crucially, it reports back a larger value for memory_and_guard_size

The failures observed with #7603 when run with MPK (WASMTIME_TEST_FORCE_MPK=1 cargo test) were due to Store::wasm_fault not being able to identify which memory an OOB address belonged to. This is because the MemoryPool was underreporting the size of the region in which OOB accesses would fault. The correct value is provided by the new SlabLayout::bytes_to_next_stripe_slot: any OOB access within that larger region must fault because (1) the other stripes have different protection keys and (2) a Store can only use one protection key. We also use (2) to guarantee that Store::wasm_fault will be able to calculate the Wasm address from the raw address.

This change also provides a new traps test that will reproduce the failures from #7603; if we observe SIGABRT from that test, it will be a regression.

This is another attempt at bytecodealliance#7603, attempting reduce the slab layout
sizes of MPK-protected stripes. While experimenting with the limits of
MPK-protected memory pools, @alexcrichton and I discovered that the
current slab layout calculations were too conservative. This meant that
the memory pool could not pack in as many memories as it should have
been able: we were expecting, but not seeing, ~15x more memory slots
over non-MPK memory pools.

This change brings together several fixes:
- it more aggressively divides up the stripes (as in b212152)
- it eliminates an extra stripe (as in 8813a30)
- it replaces some uses of `checked_*` with `saturating_*` (as in
  fb22a20)
- it improves some documentation
- and, crucially, it reports back a larger value for
  `memory_and_guard_size`

The failures observed with bytecodealliance#7603 when run with MPK
(`WASMTIME_TEST_FORCE_MPK=1 cargo test`) were due to `Store::wasm_fault`
not being able to identify which memory an OOB address belonged to.
This is because the `MemoryPool` was underreporting the size of the
region in which OOB accesses would fault. The correct value is provided
by the new `SlabLayout::bytes_to_next_stripe_slot`: any OOB access
within that larger region must fault because (1) the other stripes have
different protection keys and (2) a `Store` can only use one protection
key. We also use (2) to guarantee that `Store::wasm_fault` will be able
to calculate the Wasm address from the raw address.

This change also provides a new `traps` test that will reproduce the
failures from bytecodealliance#7603; if we observe `SIGABRT` from that test, it will be
a regression.
@abrown abrown requested a review from a team as a code owner November 30, 2023 23:44
@abrown abrown requested review from pchickey and alexcrichton and removed request for a team November 30, 2023 23:44
@abrown
Copy link
Contributor Author

abrown commented Nov 30, 2023

I manually checked that the following pass:

  • cargo test --package wasmtime-runtime --features pooling-allocator passes, which means the proptested invariants still hold
  • WASMTIME_TEST_FORCE_MPK=1 cargo test passes, which means that the subset of tests (e.g., spec tests) that we force to use MPK work on my MPK-enabled machine

Running the example program from #7609, I see expected improvements in the number of slots:

$ cargo run --example mpk
without MPK:    14713 memory slots      86.2 TiB reserved
with MPK:       220685 memory slots     86.2 TiB reserved
                14.999x more slots per reserved memory

@alexcrichton alexcrichton added this pull request to the merge queue Dec 1, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 1, 2023
@abrown
Copy link
Contributor Author

abrown commented Dec 1, 2023

This is kind of interesting: arm64 is miscomputing the WasmFault address by 8.

memory fault at wasm address 0xdeadbee8 in linear memory of size 0x10000

It should be 0xdeadbeef, right?

@alexcrichton
Copy link
Member

Ah that's expected that some platforms report a different faulting address rounded to some boundary. s390x will round it to a page boundary I think.

There's a similar test to what you're doing here which should be ok to copy

@abrown abrown enabled auto-merge December 1, 2023 18:06
@abrown abrown added this pull request to the merge queue Dec 1, 2023
Merged via the queue into bytecodealliance:main with commit 03a14ac Dec 1, 2023
19 checks passed
@abrown abrown deleted the pku-optimize-again branch December 1, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants