Implement the pooling instance allocator. #2518

peterhuene · 2020-12-17T07:15:32Z

This PR implements the pooling instance allocator.

The allocation strategy can be set with Config::with_allocation_strategy.

The pooling strategy uses the pooling instance allocator to preallocate a
contiguous region of memory for instantiating modules that adhere to various
limits.

The intention of the pooling instance allocator is to reserve as much of the
host address space needed for instantiating modules ahead of time.

This PR also implements the uffd feature in Wasmtime that enables
handling page faults in user space; this can help to reduce kernel lock
contention and thus increase throughput when many threads are
continually allocating and deallocating instances.

See the related RFC.

peterhuene · 2020-12-17T07:45:58Z

Hmm, MADV_DONTNEED may not be implemented for aarch64-linux. I'll look into the test failure soon.

github-actions · 2020-12-17T07:47:09Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

cfallin · 2020-12-17T08:04:36Z

Hmm, MADV_DONTNEED may not be implemented for aarch64-linux. I'll look into the test failure soon.

The aarch64 tests run on qemu, which has this lovely bit of implementation in its syscall handling (link):

    case TARGET_NR_madvise:
        /* A straight passthrough may not be safe because qemu sometimes
           turns private file-backed mappings into anonymous mappings.
           This will break MADV_DONTNEED.
           This is a hint, so ignoring and returning success is ok.  */
        return 0;

At some point we'd ideally run our tests on real hardware, but for now I suspect the best option would just be to disable the unit test and anything that depends on the zeroing semantics. (Or maybe a feature flag to eplicitly memset/bzero instead? That's bad if it was only sparsely committed though...)

alexcrichton · 2020-12-18T16:20:38Z

We already have a different flag for running in qemu to reduce virtual memory usage overhead since QEMU behaves differently than native processes in that respect, so perhaps that could also be where we configure "hey wasmtime madvise doesn't work as expected"

github-actions · 2021-02-05T23:39:51Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

crates/runtime/src/memory.rs

This change makes the storage of `Table` more internally consistent. Elements are stored as raw pointers for both static and dynamic table storage. Explicitly storing elements as pointers removes assumptions being made by the pooling allocator in terms of the size and default representation of the elements. However, care must be made to properly clone externrefs for table operations.

This commit fails translation of modules that have an segment offset, when added to the data length, overflows.

This commit adds a "pooling" variant to the wast tests that uses the pooling instance allocation strategy. This should help with the test coverage of the pooling instance allocator.

This commit extracts out a common pattern of finding a passive element or data segment into a `find_passive_segment` method.

peterhuene · 2021-03-06T07:08:57Z

@alexcrichton I think all of the feedback has now been addressed. FYI: the commit you stopped your review at was "fix bad merge".

This is also running the wast tests with the pooling allocator (+uffd on linux).

My TODO list following this PR:

Investigate fuzzing the pooling allocator
Move the fiber creation into the allocators (Wasmtime: lift fiber creation implementation into instance allocator #2708)

alexcrichton

Everything here's looking great to me, thanks again for being patient with me :)

As one final follow-up question as well, in the future (not as part of this PR since it's fine as-is), do you think it would be possible to use malloc/free to manage instance/table pools? The memory pool makes sense to use mmap directly since we're doing such funky things with permissions (and it's all page-aligned anyway), and the stack pool makes sense since it's all page aligned too. For instance/tables, however, it seems like there will inevitable be some degree of fragmentation because we round up to page sizes, but I don't think it's strictly necessary (especially now that uffd isn't watching those areas) and we could use malloc/free perhaps?

Otherwise I've got a few more questions/follow-ups below, but otherwise r=me

crates/wasmtime/src/module.rs

crates/runtime/src/table.rs

crates/runtime/src/instance.rs

crates/runtime/src/instance/allocator/pooling.rs

crates/runtime/src/instance/allocator/pooling/unix.rs

crates/runtime/src/instance/allocator/pooling/uffd.rs

* Improve comments. * Drop old table element *after* updating the table. * Extract out the same `cfg_if!` to a single constant.

peterhuene · 2021-03-08T18:04:14Z

Regarding the page alignment for instances and tables: we don't need it for instances for the pooling allocator, but we do need it for tables because the pooling allocator "decommits" table memory by page and you don't want any page to have elements from multiple tables.

The reason instances are paged-aligned is that this is an artifact from when the pooling allocator had one giant mmap'd region and the memories/tables of the instance came next, so they needed page-alignment back then.

I'll fix the instance pool to not page-align the instances and instead align to Instance's alignment requirements in a follow-up PR.

Regarding malloc/free for instances and tables, I think there's no inherent reason why they can't be used, but part of the intention of this design was to preallocate both (a desirable thing to do for a service) and as Instance is a magical DST, managing one's own memory mapping is probably the easiest way to accomplish that.

This commit moves the tracking for faulted guard pages in a linear memory into `Memory`.

This commit updates the error enums used in instantiation errors to encapsulate an `anyhow::Error` rather than a string.

We currently skip some tests when running our qemu-based tests for aarch64 and s390x. Qemu has broken madvise(MADV_DONTNEED) semantics -- specifically, it just ignores madvise() [1]. We could continue to whack-a-mole the tests whenever we create new functionality that relies on madvise() semantics, but ideally we'd just have emulation that properly emulates! The earlier discussions on the qemu mailing list [2] had a proposed patch for this, but (i) this patch doesn't seem to apply cleanly anymore (it's 3.5 years old) and (ii) it's pretty complex due to the need to handle qemu's ability to emulate differing page sizes on host and guest. It turns out that we only really need this for CI when host and guest have the same page size (4KiB), so we *could* just pass the madvise()s through. I wouldn't expect such a patch to ever land upstream in qemu, but it satisfies our needs I think. So this PR modifies our CI setup to patch qemu before building it locally with a little one-off patch. [1] bytecodealliance#2518 (comment) [2] https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05416.html

We currently skip some tests when running our qemu-based tests for aarch64 and s390x. Qemu has broken madvise(MADV_DONTNEED) semantics -- specifically, it just ignores madvise() [1]. We could continue to whack-a-mole the tests whenever we create new functionality that relies on madvise() semantics, but ideally we'd just have emulation that properly emulates! The earlier discussions on the qemu mailing list [2] had a proposed patch for this, but (i) this patch doesn't seem to apply cleanly anymore (it's 3.5 years old) and (ii) it's pretty complex due to the need to handle qemu's ability to emulate differing page sizes on host and guest. It turns out that we only really need this for CI when host and guest have the same page size (4KiB), so we *could* just pass the madvise()s through. I wouldn't expect such a patch to ever land upstream in qemu, but it satisfies our needs I think. So this PR modifies our CI setup to patch qemu before building it locally with a little one-off patch. [1] #2518 (comment) [2] https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05416.html

We currently skip some tests when running our qemu-based tests for aarch64 and s390x. Qemu has broken madvise(MADV_DONTNEED) semantics -- specifically, it just ignores madvise() [1]. We could continue to whack-a-mole the tests whenever we create new functionality that relies on madvise() semantics, but ideally we'd just have emulation that properly emulates! The earlier discussions on the qemu mailing list [2] had a proposed patch for this, but (i) this patch doesn't seem to apply cleanly anymore (it's 3.5 years old) and (ii) it's pretty complex due to the need to handle qemu's ability to emulate differing page sizes on host and guest. It turns out that we only really need this for CI when host and guest have the same page size (4KiB), so we *could* just pass the madvise()s through. I wouldn't expect such a patch to ever land upstream in qemu, but it satisfies our needs I think. So this PR modifies our CI setup to patch qemu before building it locally with a little one-off patch. [1] bytecodealliance#2518 (comment) [2] https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05416.html

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm wasmtime:api Related to the API of the `wasmtime` crate itself labels Dec 17, 2020

peterhuene force-pushed the add-allocator branch 4 times, most recently from b6abb64 to fbcb0c6 Compare January 29, 2021 19:54

peterhuene force-pushed the add-allocator branch 4 times, most recently from b55a66a to 404812e Compare February 5, 2021 23:12

github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Feb 5, 2021

peterhuene force-pushed the add-allocator branch 4 times, most recently from b2841d7 to 03118ad Compare February 11, 2021 05:37

peterhuene commented Feb 11, 2021

View reviewed changes

crates/runtime/src/memory.rs Outdated Show resolved Hide resolved

peterhuene force-pushed the add-allocator branch from 03118ad to f229ba7 Compare February 11, 2021 05:47

peterhuene mentioned this pull request Feb 11, 2021

Refactor module instantiation in the runtime. #2454

Closed

peterhuene force-pushed the add-allocator branch 4 times, most recently from 80c1804 to db9e04d Compare February 18, 2021 06:15

github-actions bot added the wasmtime:docs Issues related to Wasmtime's documentation label Feb 18, 2021

peterhuene force-pushed the add-allocator branch from 2ab95ca to 98853b1 Compare February 18, 2021 19:30

peterhuene added 2 commits March 5, 2021 18:36

Fail module translation for segments with overflowing offset+length.

9801c68

This commit fails translation of modules that have an segment offset, when added to the data length, overflows.

peterhuene force-pushed the add-allocator branch 4 times, most recently from f624882 to c1231e7 Compare March 6, 2021 06:01

peterhuene added 2 commits March 5, 2021 22:28

Run wast tests with both instance allocators.

57dfe99

This commit adds a "pooling" variant to the wast tests that uses the pooling instance allocation strategy. This should help with the test coverage of the pooling instance allocator.

Extract out finding a passive segment.

8e51aef

This commit extracts out a common pattern of finding a passive element or data segment into a `find_passive_segment` method.

peterhuene force-pushed the add-allocator branch from c1231e7 to 8e51aef Compare March 6, 2021 06:29

peterhuene requested a review from alexcrichton March 6, 2021 07:05

alexcrichton approved these changes Mar 8, 2021

View reviewed changes

Code review feedback.

7a93132

* Improve comments. * Drop old table element *after* updating the table. * Extract out the same `cfg_if!` to a single constant.

peterhuene force-pushed the add-allocator branch 2 times, most recently from fdc1b09 to 3d05a4f Compare March 8, 2021 17:45

peterhuene force-pushed the add-allocator branch from 3d05a4f to f1c0c73 Compare March 8, 2021 19:20

peterhuene added 2 commits March 8, 2021 11:27

Move linear memory faulted guard page tracking into Memory.

5fa0f8d

This commit moves the tracking for faulted guard pages in a linear memory into `Memory`.

Use anyhow::Error in instantiation errors.

623290d

This commit updates the error enums used in instantiation errors to encapsulate an `anyhow::Error` rather than a string.

peterhuene force-pushed the add-allocator branch from f1c0c73 to 623290d Compare March 8, 2021 19:27

peterhuene merged commit f8cc824 into bytecodealliance:main Mar 8, 2021

peterhuene deleted the add-allocator branch March 8, 2021 20:20

alexcrichton mentioned this pull request Mar 8, 2021

Pooling instance allocator: use natural alignment for instances in the pool #2715

Open

bkolobara mentioned this pull request Apr 10, 2021

Improve process spawning performance lunatic-solutions/lunatic#37

Open

akirilov-arm mentioned this pull request Jun 17, 2021

Enable more tests on AArch64 #2994

Merged

cfallin mentioned this pull request Feb 6, 2022

Patch qemu in CI to fix madvise semantics. #3770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the pooling instance allocator. #2518

Implement the pooling instance allocator. #2518

peterhuene commented Dec 17, 2020 •

edited

Loading

peterhuene commented Dec 17, 2020 •

edited

Loading

github-actions bot commented Dec 17, 2020

cfallin commented Dec 17, 2020 •

edited

Loading

alexcrichton commented Dec 18, 2020

github-actions bot commented Feb 5, 2021

peterhuene commented Mar 6, 2021

alexcrichton left a comment

peterhuene commented Mar 8, 2021

Implement the pooling instance allocator. #2518

Implement the pooling instance allocator. #2518

Conversation

peterhuene commented Dec 17, 2020 • edited Loading

peterhuene commented Dec 17, 2020 • edited Loading

github-actions bot commented Dec 17, 2020

Subscribe to Label Action

cfallin commented Dec 17, 2020 • edited Loading

alexcrichton commented Dec 18, 2020

github-actions bot commented Feb 5, 2021

Subscribe to Label Action

peterhuene commented Mar 6, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

peterhuene commented Mar 8, 2021

peterhuene commented Dec 17, 2020 •

edited

Loading

peterhuene commented Dec 17, 2020 •

edited

Loading

cfallin commented Dec 17, 2020 •

edited

Loading