[4.7] CLOUDSTACK-9004: Add features to HyperVEnlightenmentFeatureDef #1013
Conversation
|
I can see that your code does what you say in the description @jharsman but I totally lack the background in windows to judge whether this make sense. Can you expand on the reason behind this change here or in the jira ticket? Code looks good, change I cannot judge. Also I can see you created this as a subtask but not to what. Can you give that context? If you are already discussing this with someone else in the community please ping them here so they can comment and compensate for my ignorance. Thanks for working on Apache CloudStack! |
|
My apologies Daan, The PR for this Jira ticket (9004), was created as a subtask of CLOUDSTACK-8978. It's intent is to add the ability to set certain hv parameters with an end goal in 8978 of implementing hyper-V enlightenment features for Windows Server 2008 guests. The current code for HyperVEnlightenmentFeatureDef only allowed the setting of the hv_relaxed bit. This bit disables a windows sanity check that commonly results in a BSOD when the VM is running on a host under heavy load. The change here adds the ability to set the hv_vapic and hv_spinlocks bits as well as retry time. The hv_vapic bit tries to reduce interrupt overhead in guests. The hv_spinlock bit is used by the guest to notify the hypervisor that the calling virtual processor is attempting to access a resource that may be held by another virtual processor. For the host, the retry value for hv_spinlock indicates the number of times the virtual processor should attempt to access before the spinlock is considered excessive. Basically these changes are setting the stage for changes I am going to submit in CLOUDSTACK-8974. |
|
@jharshman Very clear Josh. This would be for 4.7 then. based on review you have my LGTM. We will need to run a test suite before merging but let's wait as it seems 4.6 can be out next week. I've seen a convention coming up where people prefix their PRs with [4.7] in the title. it makes sense for you to do the same Can you assign both (and any other related) tickets to yourself? Don't hesitate to call in help if you need to. |
jharshman
changed the title
|
@DaanHoogland, sounds good, I went ahead and edited the name of this PR to add the prefix [4.7]. I might be missing something, but I can't seem to find an anything along the lines of an "assign" option on the issues. Somewhat new to Jira so I might very well be over looking something obvious. As for an integration / test environment, recently, I have made use of a spare compute node and a current production ACS cluster. I'm working on putting together something a bit more suitable for testing though. |
|
as for jira, i don't think you had the rights yet, just gave em. you should see an assign to me option in the top right corner somewhere. |
|
@DaanHoogland, that was it, just assigned it to myself. Thanks! |
|
@DaanHoogland |
|
yes, 4.6 is only for bugfixes, any features go into later versions. |
jharshman
force-pushed
the
CLOUDSTACK-9004
branch
from
a34f269 to
615f724
Compare
|
@DaanHoogland from reading the mailing lists. Looks like there is a freeze for 4.7 features starting Dec 7. Can we get this change merged in before then? The change is very simplistic but it lays some ground work for making CloudStack a bit more stable for windows guests. |
|
sure, that is the goal. |
|
@jharshman I will run some tests and report back. Can you please squash the commits? |
Add function to set vapic, spinlock and retries Add function to get retry value Modify toString to output appropriate XML for spinlock value if set CLOUDSTACK-9004: Add features to HyperVEnlightenmentFeatureDef Refactored set methods to get rid of code duplication. Modified unit tests accordingly
jharshman
force-pushed
the
CLOUDSTACK-9004
branch
from
615f724 to
b6c900f
Compare
|
@remibergsma just squashed the commits, let me know if there is anything else I can do to assist testing. |
|
LGTM based on these tests: Result: And: Result: Haven't tested the actual functionality, this just shows you didn't break anything else. |
|
LGTM @jharshman can you make sure to add tests in follow-ups (both unit and integration)? |
[4.7] CLOUDSTACK-9004: Add features to HyperVEnlightenmentFeatureDefAdd function to set vapic, spinlock and retries Add function to get retry value Modify toString to output appropriate XML for spinlock value if set * pr/1013: CLOUDSTACK-9004: Add features to HyperVEnlightenmentFeatureDef Signed-off-by: Daan Hoogland <[email protected]>
|
@DaanHoogland I will make sure to do so! |
…#2870) Windows has support for several paravirt features that it will use when running on Hyper-V, Microsoft's hypervisor. These features are called enlightenments. Many of the features are similar to paravirt functionality that exists with Linux on KVM (virtio, kvmclock, PV EOI, etc.) Nowadays QEMU/KVM can also enable support for several Hyper-V enlightenments. When enabled, Windows VMs running on KVM will use many of the same paravirt optimizations they would use when running on Hyper-V. A number of years ago, a PR was introduced that added a good portion of the code to enable this feature set, but it was never completed. This PR enables the existing features. The previous patch set detailed in #1013 also included the tests. By selecting Windows PV, the enlightenment additions will be applied to the libvirt configuration. This is support on Windows Server 2008 and beyond, so all currently supported versions of Windows Server. In our testing, we've seen benchmark improvements of around 20-25% running on Centos 7 hosts and it is also supported on Centos/RHEL 6.5 and later. Testing on Ubuntu would be appreciated.
4 participants
Add function to set vapic, spinlock and retries
Add function to get retry value
Modify toString to output appropriate XML for spinlock value if set