Skip to content

kvm: FIX cpucorespersocket is not working on KVM #4497

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.

Sign up for GitHub

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

Merged
DaanHoogland merged 2 commits into from
Dec 9, 2020
Merged

kvm: FIX cpucorespersocket is not working on KVM #4497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c6c474
kvm: FIX cpucorespersocket is not working on KVM
ustcweizhou Jan 17, 2020
f200980
Add method setCpuTopology
ustcweizhou Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions ...rvisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2302,14 +2302,7 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
if (vmTO.getType() == VirtualMachine.Type.User) {
cmd.setFeatures(_cpuFeatures);
}
// multi cores per socket, for larger core configs
if (vcpus % 6 == 0) {
final int sockets = vcpus / 6;
cmd.setTopology(6, sockets);
} else if (vcpus % 4 == 0) {
final int sockets = vcpus / 4;
cmd.setTopology(4, sockets);
}
setCpuTopology(cmd, vcpus, vmTO.getDetails());
vm.addComp(cmd);
}

Expand Down Expand Up @@ -4230,4 +4223,26 @@ public boolean isSecureMode(String bootMode) {

return false;
}

private void setCpuTopology(CpuModeDef cmd, int vcpus, Map<String, String> details) {
Copy link
Contributor

@sureshanaparti sureshanaparti Dec 9, 2020

Choose a reason for hiding this comment

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

@ustcweizhou possible to pass coresPerSocket here, instead complete VM details here ?

Copy link
Member

@weizhouapache weizhouapache Dec 9, 2020

Choose a reason for hiding this comment

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

@sureshanaparti
I was thinking about it. at the end I decided to pass vm details based on two reasons
(1) setCpuTopology is a complete function now. if pass coresPerSocket , we need to get coresPerSocket out of the method.
(2) it is better if add more cpu settings , for example the pr #4178

Copy link
Contributor

@sureshanaparti sureshanaparti Dec 9, 2020

Choose a reason for hiding this comment

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

@sureshanaparti
I was thinking about it. at the end I decided to pass vm details based on two reasons
(1) setCpuTopology is a complete function now. if pass coresPerSocket , we need to get coresPerSocket out of the method.
(2) it is better if add more cpu settings , for example the pr #4178

Ok @weizhouapache so going forward, cpu hyperthreading detail mentioned in PR #4178 will be added to this method, right?

Copy link
Member

@weizhouapache weizhouapache Dec 9, 2020

Choose a reason for hiding this comment

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

@sureshanaparti yes. but I need to discuss with @div8cn about some details. it will be an improvement in 4.16

Copy link
Contributor

@sureshanaparti sureshanaparti Dec 9, 2020

Choose a reason for hiding this comment

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

@sureshanaparti yes. but I need to discuss with @div8cn about some details. it will be an improvement in 4.16

OK, Pls update details here. As this PR is targeted for 4.14.1.0, the proposed changes with cpu hyperthreading detail will be targeted for 4.16, Right?

Copy link
Member

@weizhouapache weizhouapache Dec 9, 2020

Choose a reason for hiding this comment

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

@sureshanaparti yes, this is a bug which should be fixed in 4.14 and 4.15.
the other pr is an improvement. it will not impact this pr.

// multi cores per socket, for larger core configs
int numCoresPerSocket = -1;
if (details != null) {
final String coresPerSocket = details.get(VmDetailConstants.CPU_CORE_PER_SOCKET);
final int intCoresPerSocket = NumbersUtil.parseInt(coresPerSocket, numCoresPerSocket);
if (intCoresPerSocket > 0 && vcpus % intCoresPerSocket == 0) {
numCoresPerSocket = intCoresPerSocket;
}
}
if (numCoresPerSocket <= 0) {
if (vcpus % 6 == 0) {
numCoresPerSocket = 6;
} else if (vcpus % 4 == 0) {
Copy link
Contributor

@sureshanaparti sureshanaparti Nov 26, 2020

Choose a reason for hiding this comment

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

when vcpus are 8, it is considered as 4 cores per socket here, instead 8 cores.

Copy link
Member

@weizhouapache weizhouapache Nov 26, 2020

Choose a reason for hiding this comment

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

@sureshanaparti this is the current behavior.

Copy link
Contributor

@sureshanaparti sureshanaparti Nov 27, 2020

Choose a reason for hiding this comment

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

@sureshanaparti this is the current behavior.

agree, any plan to address here?

Copy link
Member

@weizhouapache weizhouapache Nov 27, 2020

Choose a reason for hiding this comment

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

@sureshanaparti you mean change the current behavior ? No. I am not going to change it
It worked since cloudstack 4.13 in commit 2f53295
https://issues.apache.org/jira/browse/CLOUDSTACK-5521

Copy link
Contributor

@sureshanaparti sureshanaparti Dec 1, 2020

Choose a reason for hiding this comment

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

@sureshanaparti you mean change the current behavior ? No. I am not going to change it
It worked since cloudstack 4.13 in commit 2f53295
https://issues.apache.org/jira/browse/CLOUDSTACK-5521

ok @weizhouapache so no change in the behavior. thanks.

numCoresPerSocket = 4;
} else {
numCoresPerSocket = 1;
}
}
cmd.setTopology(numCoresPerSocket, vcpus / numCoresPerSocket);
}
}
9 changes: 9 additions & 0 deletions server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2478,6 +2478,15 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
if (ovfPropertyVO != null && ovfPropertyVO.isPassword()) {
details.put(detailName, DBEncryptionUtil.encrypt(details.get(detailName)));
}
} else if (VmDetailConstants.CPU_CORE_PER_SOCKET.equals(detailName)) {
try {
final int val = Integer.parseInt(details.get(detailName));
if (val <= 0) {
throw new InvalidParameterValueException("Please enter a positive integer value for the vm setting: " + detailName);
}
} catch (final NumberFormatException e) {
throw new InvalidParameterValueException("Please enter an integer value for vm setting: " + detailName);
}
}
}
vmInstance.setDetails(details);
Expand Down