Stream: git-wasmtime

Topic: wasmtime / PR #7622 mpk: optimize layout of protected str...

Wasmtime GitHub notifications bot (Nov 30 2023 at 23:44):

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_* 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.

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 30 2023 at 23:44):

abrown requested wasmtime-core-reviewers for a review on PR #7622.

Wasmtime GitHub notifications bot (Nov 30 2023 at 23:44):

abrown requested pchickey for a review on PR #7622.

Wasmtime GitHub notifications bot (Nov 30 2023 at 23:44):

abrown requested alexcrichton for a review on PR #7622.

Wasmtime GitHub notifications bot (Nov 30 2023 at 23:49):

abrown commented 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 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

Wasmtime GitHub notifications bot (Dec 01 2023 at 15:46):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2023 at 16:43):

abrown commented on PR #7622:

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?

Wasmtime GitHub notifications bot (Dec 01 2023 at 17:58):

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

Wasmtime GitHub notifications bot (Dec 01 2023 at 18:05):

abrown updated PR #7622.

Wasmtime GitHub notifications bot (Dec 01 2023 at 18:06):

abrown has enabled auto merge for PR #7622.

Wasmtime GitHub notifications bot (Dec 01 2023 at 18:19):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2023 at 18:58):

abrown merged PR #7622.

Last updated: Nov 22 2024 at 16:03 UTC