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.

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

Jump to bottom

Introduce isolated threads CPUs #260

Merged
merged 2 commits into from May 8, 2022
Merged

Introduce isolated threads CPUs #260

merged 2 commits into from May 8, 2022

Conversation

liranr23
Copy link
Member

This patch introduce the logic for isolated threads CpuPinningPolicy. In
this policy, each virtual CPU (thread) is taking a whole physical core
on the host. These cores are exclusive for the VM to use.

Change-Id: I1a3c00ea73f92208725f287c5b6de8d01365a3c0
Signed-off-by: Liran Rotenberg lrotenbe@redhat.com

@liranr23 liranr23 added the virt label Apr 12, 2022
ljelinkova
ljelinkova reviewed Apr 12, 2022
View reviewed changes
Copy link
Collaborator

@ljelinkova ljelinkova left a comment

Choose a reason for hiding this comment

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

I'd like to propose to rename some of the methods from dedicated to exclusivelyPinned. We've used dedicated as a synonym to exclusively pinned so far but now that we're introducing a new policy we should distinguish them.

backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java Outdated Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Outdated Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Outdated Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/CpuPinningHelper.java Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/CpuPinningHelper.java Outdated Show resolved Hide resolved
@liranr23 liranr23 force-pushed the isolate_threads branch 2 times, most recently from 93dac04 to a5672bb Compare April 12, 2022 16:14
@liranr23 liranr23 marked this pull request as ready for review April 13, 2022 06:49
@liranr23
Copy link
Member Author

/ost

ljelinkova
ljelinkova approved these changes Apr 13, 2022
View reviewed changes
Copy link
Collaborator

@ljelinkova ljelinkova left a comment

Choose a reason for hiding this comment

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

Looks good

ahadas
ahadas reviewed Apr 14, 2022
View reviewed changes
Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

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

generally looks good
I think changing 'dedicated' to 'exclusive' makes sense to distinguish that from the dedicated policy as well - changing in all places requires a bit effort so I commented only on some places that I think the benefit would worth it
besides that, I think the only major comment is about how the isolated threads should be sent to the host - if we do it this way, then why should VDSM even know about the policy? something doesn't add up, therefore added @nyoxi

...broker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/LibvirtVmXmlBuilder.java Outdated Show resolved Hide resolved
...dmin/modules/uicompat/src/main/resources/org/ovirt/engine/ui/uicompat/UIConstants.properties Outdated Show resolved Hide resolved
...dmin/modules/uicompat/src/main/resources/org/ovirt/engine/ui/uicompat/UIConstants.properties Outdated Show resolved Hide resolved
...n/modules/uicompat/src/main/resources/org/ovirt/engine/ui/uicompat/LocalizedEnums.properties Outdated Show resolved Hide resolved
...nager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/NumaPinningHelper.java Outdated Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Outdated Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Outdated Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Outdated Show resolved Hide resolved
@liranr23
Copy link
Member Author

/ost

@liranr23
Copy link
Member Author

/ost

@liranr23 liranr23 force-pushed the isolate_threads branch 2 times, most recently from 2866c4f to ca57089 Compare April 28, 2022 17:04
@liranr23
Copy link
Member Author

/ost

@liranr23 liranr23 force-pushed the isolate_threads branch 2 times, most recently from 56ed161 to 0c5eec7 Compare May 2, 2022 11:19
@liranr23
Copy link
Member Author

liranr23 commented May 2, 2022

/ost

ahadas
ahadas reviewed May 3, 2022
View reviewed changes
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java Outdated Show resolved Hide resolved
...bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuPinningPolicyUnit.java Outdated Show resolved Hide resolved
...bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuPinningPolicyUnit.java Outdated Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/CpuPinningHelper.java Outdated Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/CpuPinningHelper.java Outdated Show resolved Hide resolved
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java Outdated Show resolved Hide resolved
@liranr23 liranr23 force-pushed the isolate_threads branch 2 times, most recently from 37cdd67 to b5b9316 Compare May 3, 2022 16:32
@liranr23
core: introduce isolated threads cpus
b79f880 
This patch introduce the logic for isolated threads CpuPinningPolicy. In
this policy, each virtual CPU (thread) is taking a whole physical core
on the host. These cores are exclusive for the VM to use.

Change-Id: I1a3c00ea73f92208725f287c5b6de8d01365a3c0
Bug-Url: https://bugzilla.redhat.com/1782077
Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
@liranr23
frontend: introduce isolate threads
c8fa057 
This patch adds the isolate threads option to the UI.

Change-Id: I7b7c095b2acad2f0693f238a2dd3b4c86f9fb63b
Bug-Url: https://bugzilla.redhat.com/1782077
Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
@ahadas
Copy link
Member

ahadas commented May 8, 2022

/ost

@ahadas
Copy link
Member

ahadas commented May 8, 2022

OST failed on UI tests

@ahadas ahadas merged commit f9a19e2 into oVirt:master May 8, 2022
@liranr23 liranr23 deleted the isolate_threads branch May 9, 2022 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyoxi nyoxi nyoxi left review comments

@ljelinkova ljelinkova ljelinkova approved these changes

@ahadas ahadas ahadas approved these changes

@sgratch sgratch Awaiting requested review from sgratch sgratch is a code owner

@bennyz bennyz Awaiting requested review from bennyz bennyz is a code owner

@emesika emesika Awaiting requested review from emesika emesika is a code owner

@mwperina mwperina Awaiting requested review from mwperina mwperina is a code owner

@michalskrivanek michalskrivanek Awaiting requested review from michalskrivanek michalskrivanek is a code owner

@oliel oliel Awaiting requested review from oliel oliel is a code owner

@didib didib Awaiting requested review from didib didib is a code owner

Assignees
No one assigned
Labels
virt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@liranr23 @ahadas @ljelinkova @nyoxi