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

feat: Support dedicated CPU placement #19

Merged
merged 1 commit into from
Aug 5, 2022
Merged

Conversation

carezkh
Copy link
Contributor

@carezkh carezkh commented Aug 1, 2022

No description provided.

@carezkh carezkh requested a review from fengye87 August 1, 2022 10:21
@codecov-commenter
Copy link

Codecov Report

Merging #19 (e1d27a2) into main (00cb1dc) will increase coverage by 3.38%.
The diff coverage is 72.30%.

@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
+ Coverage   26.79%   30.17%   +3.38%     
==========================================
  Files           2        3       +1     
  Lines         780      845      +65     
==========================================
+ Hits          209      255      +46     
- Misses        526      545      +19     
  Partials       45       45              
Impacted Files Coverage Δ
pkg/cpuset/cpuset.go 72.30% <72.30%> (ø)
pkg/controller/vm_controller.go 24.33% <0.00%> (-0.27%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@carezkh carezkh changed the title feat: Support dedicated CPU placement [WIP]feat: Support dedicated CPU placement Aug 1, 2022
@carezkh carezkh force-pushed the care-dev-dedicated-cpu branch from e1d27a2 to 43388c5 Compare August 1, 2022 12:50
@carezkh carezkh changed the title [WIP]feat: Support dedicated CPU placement feat: Support dedicated CPU placement Aug 1, 2022
@@ -45,9 +46,17 @@ func main() {
if vmConfig.Cmdline != nil {
cloudHypervisorCmd = append(cloudHypervisorCmd, "--cmdline", fmt.Sprintf("'%s'", vmConfig.Cmdline.Args))
}
cloudHypervisorCmd = append(cloudHypervisorCmd, "--cpus", fmt.Sprintf("boot=%d,topology=%d:%d:%d:%d",
vcpuToPCPU := []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line above would be better

cmd/virt-prerunner/main.go Show resolved Hide resolved
for i := 0; i < numVCPUs; i++ {
vmConfig.Cpus.Affinity = append(vmConfig.Cpus.Affinity, &cloudhypervisor.CpuAffinity{
Vcpu: i,
HostCpus: []int{pcpus[i%numPCPUs]},
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a webhook validation for CPU resources? If the CPU resource is not guaranteed, it may not be a good idea to bind vCPUs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's optional for user to configure kubelet CPU manager policy to Static and use Guaranteed pod to get exclusive CPUs on the node, but user can get better performance by using Static policy and Guaranteed QOS. Without any configuration, we bind vCPU to avoid cache missing causing by CPU migration.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the VM pod is not in the Guaranteed group, it could see all PCPUs are available for binding? If so, the i%numPCPUs part would always choose to bind the first several PCPUs?

Copy link
Contributor Author

@carezkh carezkh Aug 2, 2022

Choose a reason for hiding this comment

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

Right, maybe a random algorithm of binding vCPUs could be better. For example, binding vCPUs[0,1,2,3,4,5] to pCPUs[0,1,2,3] can be resolved as follows:

step1: Use each pCPU in a balanced manner. vCPU -> pCPU [0 -> 0, 1 -> 1, 2 -> 2, 3 -> 3]
step2: Bind the other vCPUs randomly. vCPU -> pCPU [4 -> one of 0-3, 5 -> one of 0-3]

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still having a bad feeling about this. It's neither dedicated nor efficient to bind vCPUs to physical cores if they have unmatched numbers.

@carezkh carezkh force-pushed the care-dev-dedicated-cpu branch 2 times, most recently from 1ca5c9c to 163e8ee Compare August 2, 2022 06:13
@carezkh carezkh requested a review from fengye87 August 2, 2022 06:14
@carezkh carezkh force-pushed the care-dev-dedicated-cpu branch from 163e8ee to 834faef Compare August 4, 2022 08:36
Copy link
Contributor

@fengye87 fengye87 left a comment

Choose a reason for hiding this comment

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

LGTM

@fengye87 fengye87 merged commit d146dc5 into main Aug 5, 2022
@fengye87 fengye87 deleted the care-dev-dedicated-cpu branch August 5, 2022 02:25
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.

3 participants