Skip to content

Conversation

@div8cn
Copy link
Contributor

@div8cn div8cn commented Jun 25, 2020

Description

Supplement the vcpu cores Per Socket configuration of KVM VM
Increase the vcpu hyperthreading configuration of KVM VM

Use vm.details for configuration

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

@luhaijiao
Copy link

This feature aims to allow user to specify how many CPU sockets the target VM would have by using the 'cores per socket' parameter.

Assume user assign 16 vCPUs to a new VM, without manually specify the 'cores per socket', CloudStack would try to divide the 16 vCPUs by 6, 4 and 1 in order. In this case, CloudStack would assign 4 sockets (16vCPUs /4) to this VM and every scoket has 4 vCPUs.

In most cases, it shall casue no issue, however if there's sort of CPU limits due to Guest OS or application, e.g. SQL Sever version, it could turn into a big performance issue.

In our real case, user deployed an Windows 10 guest OS with 16 vCPUs, and CloudStack assgins 4 virtual CPU sockets by default. Due to Windows 10 only supports 2 CPU sockets (https://answers.microsoft.com/en-us/windows/forum/windows_10-win_upgrade/windows-10-versions-cpu-limits/905c24ad-ad54-4122-b730-b9e7519c823f?auth=1) , it turns out this guest OS is only able to utilize two sockets (8 vCPUs) even though we have assigned 16 vCPUs in toal. If user has CPU intensive application running on this VM, this could introduce bottelneck.

Copy link
Member

@GabrielBrascher GabrielBrascher left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @div8cn. Overal code LGTM (I have not tested it); however, I see some improvements in terms of extracting a few lines to a method(s) and then adding unit test cases.

This is not a -1, it is a tip of what (I think) can be improved. We are trying to cover tests as much as possible on CloudStack codebase and breaking big methods into smaller is a great way of doing it.

} else if (vcpus % 4 == 0) {
final int sockets = vcpus / 4;
cmd.setTopology(4, sockets);
}
Copy link
Member

@GabrielBrascher GabrielBrascher Jun 29, 2020

Choose a reason for hiding this comment

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

What do you think of extracting lines 2300 - 2325 into a method, document, and then create JUnit tests for the new method(s)?

@DaanHoogland
Copy link
Contributor

@GabrielBrascher @div8cn @luhaijiao what is the status of this PR?

@weizhouapache
Copy link
Member

#4497

@div8cn div8cn closed this Jan 4, 2021
@div8cn div8cn deleted the kvm-vcpu-socket-threads branch January 4, 2021 08:02
@DaanHoogland DaanHoogland removed this from the 4.16.0.0 milestone Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants