-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
There was a problem hiding this 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?
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'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:
The 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
Let me know if you had something else in mind here. |
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. |
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. |
There was a problem hiding this 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!
There was a problem hiding this 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.
a0690e4
to
60b280e
Compare
Thanks for the reviews! I've amended a few comments in 767a05e to address the latest feedback. Will merge once CI is green. |
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
OverlayKind
s. (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.