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

lib: tidy up overlay page migration & reduce memory usage #861

Merged
merged 9 commits into from
Feb 20, 2025

Conversation

gjcolombo
Copy link
Contributor

Tweak a few bits of Hyper-V overlay migration:

  • Don't store page kinds separately from page contents. Instead, a page's kind determines its contents. Most overlays have static contents or contents that depend on only a couple of parameters (e.g. the TSC scale/offset for the TSC reference page), so this saves a fair bit of memory for most kinds of pending overlay pages.

  • Change the overlay ordering discipline to depend entirely on the ordering of OverlayKinds. (Previously the first overlay assigned to a PFN would become active and then would remain active until it was removed, but pending overlays were promoted in order by kind.) This obviates the need to export/import a PFN's current active overlay during migration (or to remember the order in which overlays were applied), since a set of overlays at a single PFN will always have the same active overlay (...provided the ordering remains stable across Propolis versions; there's a comment about this caveat in the new code.)

  • Continue to manually transfer the original contents of guest pages that are covered by an overlay (since these still need to be restored), but wrap them in an enum with a variant that specifies that the overlaid page contained all zeroes. Because VM memory is initially zeroed, and many guests will establish overlay pages early in boot over pages they haven't otherwise touched, this can significantly reduce the amount of serialized page data that the manager needs to send over a migration.

See #853 (comment) for a bit more context.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like this approach: treating the set of overlays as ordered by "priority" based on what kind of overlay they are, and considering whatever is the highest-priority overlay in the set the "active" one feels like a nicer design than the previous approaches of having a separate field for tracking the active overlay, and having activeness be part of the map key. It's nice that (unless we screw up and reorder the overlay kind variants by mistake) we can deterministically reason about how we select the active overlay based on what kind of overlay it is, rather than the order that the overlays were created.

I also like the optimizations to reduce how many bytes we have to send during migration; in particular, I think you're almost certainly right that guests will likely be using zeroed pages for overlays most of the time and it seems goofy to migrate a bunch of zeros, so that's a good case to optimize for.

I assume that the rest of the migration plumbing will be added in this branch if we decide to actually go forwards with it?

@gjcolombo gjcolombo marked this pull request as ready for review February 19, 2025 00:01
@gjcolombo
Copy link
Contributor Author

Thanks for the review! I've moved this out of draft since I think I'm inclined to move forward with it. (This will supersede #853.)

I assume that the rest of the migration plumbing will be added in this branch if we decide to actually go forwards with it?

I'm not sure there's any additional plumbing to add, though I may well have missed something. I think the additional checks on the import path in #853 are covered implicitly in this design:

  • Hypercall MSR says the overlay should be enabled, but there's no record for that PFN in the exported page contents: this will produce a length mismatch when importing those contents
  • Hypercall MSR says the overlay shouldn't be enabled, but there's an entry for it in the exported page contents: same as above
  • Source and target disagree on the PFN at which the hypercall overlay is enabled: this should also be caught by original-page import. The target will always apply the hypercall overlay exactly where the source told it to; if the source's overlay manager and MSR disagree about where that overlay is actually located (yikes!), import will fail because the target's manager won't find a set into which to import the original page contents

The export and import routines on OverlayManager in #853 are obsoleted by their counterparts in this PR.

The other big difference between this and #853 is that this PR doesn't represent the original overlay contents as a separately-versioned type with its own Schema impl. The risk of doing that is that if the overlay contents' format ever changes, we'll need to rev the entire HyperVEnlightenmentV1 structure instead of just a component thereof. I think that's OK for a couple of reasons:

  • the format of the map doesn't seem that likely to change: we're just mapping from PFNs to saved page contents and not encoding anything about the structure of the overlay manager's tables
  • because only the Hyper-V stack exports/imports overlay state, we're not really limiting the number of things we have to update by giving it its own schema (cf. something like device-agnostic PCI state, where we want to be able to update the schema without having to revise the device state structures for every PCI device type in the system)

Let me know if you had something else in mind here.

@gjcolombo
Copy link
Contributor Author

I've pushed one more commit, a0690e4, that takes a still somewhat different approach to the migration problem (but tries to keep some of the goodies from elsewhere in the PR).

The idea is to notice that the Hyper-V manager is now reconstituting pending overlays from their "parameters" during a migration--i.e., instead of sending a pending overlay's pending contents in the raw, it now just recreates the overlay from scratch on the target. a0690e4 extends this to active overlays: when the Hyper-V manager pauses, it drops all its active overlays, allowing all the original guest memory contents to be sent in the RAM transfer phase instead of being serialized and sent in the device state phase. If migration succeeds, the target will recreate all necessary overlays during device state import; if it fails, the source will reapply them when the enlightenment resumes.

The downside of this change is that it strictly requires vCPUs to pause before devices (otherwise a vCPU running after the enlightenment has paused can read from a page whose overlay is "missing"). Fortunately, this is already the ordering we use in both propolis-server and propolis-standalone. If we decide much later that we want to allow (some) devices to pause before vCPUs have stopped, we can always break pause into "before vCPUs" and "after vCPUs" phases.

@hawkw
Copy link
Member

hawkw commented Feb 19, 2025

I'm not sure there's any additional plumbing to add, though I may well have missed something. I think the additional checks on the import path in #853 are covered implicitly in this design:

Nope, I just scrolled past the changes to the migration functions and thought you hadn't done them yet --- my bad. Disregard that comment from last night.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I'm sold on this approach!

Copy link
Collaborator

@pfmooney pfmooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this looks from a high level, but have not done more thorough review.

@gjcolombo gjcolombo force-pushed the gjcolombo/overlay-pages-vii-remake branch from a0690e4 to 60b280e Compare February 20, 2025 14:18
@gjcolombo
Copy link
Contributor Author

Thanks for the reviews! I've amended a few comments in 767a05e to address the latest feedback. Will merge once CI is green.

@gjcolombo gjcolombo linked an issue Feb 20, 2025 that may be closed by this pull request
@gjcolombo gjcolombo merged commit f839831 into master Feb 20, 2025
11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/overlay-pages-vii-remake branch February 20, 2025 16:24
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.

lib: want better Hyper-V overlay page management
3 participants