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

balloons: add support for power efficient & high performance cores #354

Merged
merged 2 commits into from
Sep 2, 2024

Conversation

fmuyassarov
Copy link
Collaborator

@fmuyassarov fmuyassarov commented Aug 13, 2024

Introduce a new parameter coreType in Balloons policy for allowing users to express their preference of selecting P-high performance or power efficient cores.

@fmuyassarov fmuyassarov requested review from askervin, klihub and kad and removed request for askervin August 13, 2024 08:48
@fmuyassarov
Copy link
Collaborator Author

@klihub @askervin @kad PTAL. Updated according to the discussion we had yesterday.

@fmuyassarov fmuyassarov force-pushed the p/e-cores branch 2 times, most recently from 2aad4db to ccbafbc Compare August 21, 2024 10:40
@fmuyassarov fmuyassarov changed the title balloons: add support for P/E cores balloons: add support for power efficient & high performance cores Aug 21, 2024
Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

I have one nit and spotted a few typos. Otherwise LGTM.

klihub
klihub previously approved these changes Aug 26, 2024
cmd/plugins/balloons/policy/balloons-policy.go Outdated Show resolved Hide resolved
@klihub klihub self-requested a review August 27, 2024 07:28
@klihub klihub dismissed their stale review August 27, 2024 07:29

A few remaining things to fix as mentioned in the review comments...

This commit introduces a new field in the balloons configuration CR,
enabling users to specify their efficient/performance core preferences
for their workloads. When a user explicitly requests efficient or
performance cores, the policy will attempt to allocate according
to the user preferences. If the request cannot be satisfied, the
policy will fall back to other available cores  on the host.

If a user adds `preferIsolCpus` in addition to specifying coreTypes,
the policy will prioritize allocating isolated CPUs cores over the
preferred efficient/performance cores, in case these sets are different.

Signed-off-by: Feruzjon Muyassarov <[email protected]>
@fmuyassarov
Copy link
Collaborator Author

@klihub @askervin PTAL. Addressed the last comments.

@klihub
Copy link
Collaborator

klihub commented Sep 2, 2024

@klihub @askervin PTAL. Addressed the last comments.

@fmuyassarov Just to make sure I understand the latest changes: we have explicit positive preferences but no implied negative ones, right ? IOW: a core type preference is simply translated to a virtual device close to the right types of cores, but we don't try to generate/inject avoided virtual devices to avoid taking cores of non-preferred types. Is this correct ?

@fmuyassarov
Copy link
Collaborator Author

@klihub @askervin PTAL. Addressed the last comments.

@fmuyassarov Just to make sure I understand the latest changes: we have explicit positive preferences but no implied negative ones, right ?

That's right. Basically, try to use cores of the preferred device as much as we can if not we continue to use non-preferred ones.

@klihub
Copy link
Collaborator

klihub commented Sep 2, 2024

@klihub @askervin PTAL. Addressed the last comments.

@fmuyassarov Just to make sure I understand the latest changes: we have explicit positive preferences but no implied negative ones, right ?

That's right. Basically, try to use cores of the preferred device as much as we can if not we continue to use non-preferred ones.

Thanks ! I am happy with this. Let's wait for @askervin to check if his concerns have now all been addressed.

Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

LGTM.

I think we can have the e2e test added in a separate PR.

@klihub klihub merged commit ed7cecc into containers:main Sep 2, 2024
3 checks passed
@fmuyassarov fmuyassarov deleted the p/e-cores branch September 14, 2024 10:23
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.

4 participants