abrown opened PR #7622 from abrown:pku-optimize-again
to bytecodealliance:main
:
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_*
withsaturating_*
(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 toStore::wasm_fault
not being able to identify which memory an OOB address belonged to. This is because theMemoryPool
was underreporting the size of the region in which OOB accesses would fault. The correct value is provided by the newSlabLayout::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) aStore
can only use one protection key. We also use (2) to guarantee thatStore::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 observeSIGABRT
from that test, it will be a regression.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
abrown requested wasmtime-core-reviewers for a review on PR #7622.
abrown requested pchickey for a review on PR #7622.
abrown requested alexcrichton for a review on PR #7622.
I manually checked that the following pass:
cargo test --package wasmtime-runtime --features pooling-allocator
passes, which means the proptested invariants still holdWASMTIME_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 machineRunning 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 submitted PR review.
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 commented on PR #7622:
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 updated PR #7622.
abrown has enabled auto merge for PR #7622.
alexcrichton submitted PR review.
abrown merged PR #7622.
Last updated: Dec 23 2024 at 13:07 UTC