Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

move mmap into sysdeps #524

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented May 15, 2020

The patch is relatively easy; what's not clear to me is how something
like this really wants to be exported. Should sysdeps export a
MappedRegion that would be backed by VirtualAlloc on Windows?
Should sysdeps just export everything with their platform names and
let lucet-runtime use conditionals to choose what to export?

This patch punts on those questions for now; we're just trying to move
the obvious system-dependent code into sysdeps.

The patch is relatively easy; what's not clear to me is how something
like this really wants to be exported.  Should `sysdeps` export a
`MappedRegion` that would be backed by `VirtualAlloc` on Windows?
Should `sysdeps` just export everything with their platform names and
let `lucet-runtime` use conditionals to choose what to export?

This patch punts on those questions for now; we're just trying to move
the obvious system-dependent code into `sysdeps`.
@froydnj froydnj requested a review from acfoltzer May 18, 2020 14:12
@acfoltzer acfoltzer requested review from fst-crenshaw and removed request for acfoltzer May 21, 2020 19:11
@fst-crenshaw
Copy link
Contributor

Hello! Thanks for this work!

I know that you requested that @acfoltzer review this PR. He asked me to review the PR so that you could get a timely response. My intention is to continue the conversation you've started around this valuable reorganization of system-dependent Region implementations.

At a high-level, I appreciate the sensibility of putting system-dependent code into a directory called sysdeps. I saw that this was done recently for host_page_size and that was a very worthwhile refactor.

System-dependent Region code requires a less-straightforward refactor. As you can see from the merge conflicts on this PR, other folks have also been working on Regions in the lucet-runtime portion of this repo. Most notably, a recent PR introduced another system-dependent implementation of Regions, this time using the Linux Kernel’s userfaultfd mechanism. See it here:
#492

What I like about the timing of your PR alongside the above-mentioned PR is that the repo now has a concrete example of what it looks like when there are side-by-side system-dependent implementations of Region in the repo. Here are some highlights:

First, there is some work to conditionally include the uffd version:
https://github.com/bytecodealliance/lucet/blob/master/lucet-runtime/lucet-runtime-internals/src/region.rs#L3

Second, there are some fiddly things that take place to run the tests — once for mmap and once for userfaultfd:
https://github.com/bytecodealliance/lucet/blob/master/lucet-runtime/tests/entrypoint.rs

Finally, the repo has taken on a new CI (CircleCI) so that it has Linux containers that are privileged enough to use the userfaultfd mechanism.

Certainly, this concrete example isn't ideal. But it is something to look at and may spark insights.

You asked really good questions. To paraphrase, “How does something like this really want to be exported?”

Here’s one way that’s done now in a Fastly repo:

use lucet_runtime::{MmapRegion as LucetRegion, Region};
I don’t offer this as a good example of how something like this really wants to be exported, but rather as a concrete example of what is done today.

Now that the userfaultfd work has been folded into the repo, now that we have a concrete example of side-by-side Region implementations, I'd love to hear any reactions you have about eventually adding a Windows-dependent version into the mix.

@pchickey pchickey closed this Jun 26, 2020
@pchickey
Copy link
Contributor

This PR was closed as a byproduct of deleting the branch named master. If this is still an active PR, re-open as a new PR against main.

@acfoltzer acfoltzer reopened this Jun 26, 2020
@acfoltzer acfoltzer changed the base branch from master to main June 26, 2020 00:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants