KEP-2621: Add llc affinity to cpu manager. #2684

enzoyes · 2021-05-06T13:08:56Z

design for issue 2621

k8s-ci-robot · 2021-05-06T13:08:59Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-05-06T13:09:04Z

Welcome @ranchothu!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-05-06T13:09:05Z

Hi @ranchothu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-05-06T13:09:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ranchothu
To complete the pull request process, please assign derekwaynecarr after the PR has been reviewed.
You can assign the PR to them by writing /assign @derekwaynecarr in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/sig-node/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2021-05-06T13:54:16Z

@ranchothu: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ehashman · 2021-05-06T23:32:10Z

Hi @ranchothu,

This currently isn't being tracked for SIG Node planning for 1.22: https://docs.google.com/document/d/1U10J0WwgWXkdYrqWGGvO8iH2HKeerQAlygnqgDgWv4E/edit#

/hold

It also appears that you will need to sign the Kubernetes CLA (see the bot comment above: #2684 (comment)).

pacoxu · 2021-05-07T02:15:59Z

I suspect you are using a different 'GitHub user' name when you committed. Check Github user setting env or config in your develop env.

enzoyes · 2021-05-07T03:16:08Z

I suspect you are using a different 'GitHub user' name when you committed. Check Github user setting env or config in your develop env.

@pacoxu maybe a ok-to-test is in need, seems cla not recheck after force-pushes. And when i reply with I signed it also not works.

pacoxu · 2021-05-07T03:28:56Z

/ok-to-test

enzoyes · 2021-05-07T03:31:01Z

/retest

enzoyes · 2021-05-07T03:43:11Z

I signed it

swatisehgal

Took an initial pass at the KEP. The motivation is clear to me but I would recommend to add more specific use case in terms of the performance sensitive workloads that have strictly need resource allocation while taking L3 cache into consideration.

swatisehgal · 2021-05-17T09:44:08Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

+### Risks and Mitigations
+
+ Currently no risks was found.
+ Feature is enbled by a gate - a new kube feature with default false, potential risk effects could be limited.


NIT: typo enbled -> enabled

swatisehgal · 2021-05-17T09:49:13Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

+
+- Feature Gate
+  - Add `CPUManagerUncoreCacheAlign` to kubelet's feature-gates to enable(true)/disable(false) the feature.
+  - Also, more than one l3 cache should exist in a single socket/package.


It is probably implied but maybe we should explicitly capture the scenario where CPUManagerUncoreCacheAlign is enabled and in case only one l3 cache is present, we would obtain the current behaviour.

swatisehgal · 2021-05-17T09:50:34Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

+
+- General Design
+  - Logic Elaboration
+  Try to allocate cpus sharing the same cache if demand is larger than one core. Add L3 cache affinity before tring core affinity best-fit.


NIT: typo tring -> trying

swatisehgal · 2021-05-17T09:53:01Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

+
+![design_overview](design_overview.png "design_overview")
+
+  - feature-gates `CPUManagerUncoreCacheAlign`


Looks like some formatting issue in the way this line is rendered?

enzoyes · 2021-05-19T08:20:10Z

@swatisehgal , thanks, and modifies are updated.

ffromani · 2021-05-19T08:24:34Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

-  - Add `CPUManagerUncoreCacheAlign` to kubelet's feature-gates to enable(true)/disable(false) the feature.
-  - Also, more than one l3 cache should exist in a single socket/package.
-
+  - C1: Add `CPUManagerUncoreCacheAlign` to kubelet's feature-gates to enable(true)/disable(false) the feature.


So this feature (per my previous comment, see history) will need to depend on a feature gate. But you don't necessarily need a separate feature gate, you can depend on the one we added on #2626.

This CPUManager feature gate will most likely be alpha in the 1.23 cycle, which fit the needs of this KEP.

I think you can conditionally enable this optimization depending on a cpu manager policy options. This way you keep the conditional logic you already need to support the feature gate, without extra burden.

Fitting in the new CPUManager policy options is also a very nice and clean design.

hi, @fromanirh since i've post a patch kubernetes/kubernetes#102307, it maybe help you to understand why i choose a kubelet running option other than a separate cpu manager policy. IMHO, attach to a policy is also optional, but it will cause some redundant logic.

Thanks for sharing the implementation. I don't see yet where we need redundant logic, however. We could:

get the enable/disable flag from cpumanager options. Probably the option should be on by default

propagate the flag down to the cpuAccumulator

consume the flag into isUncoreCacheAlignEnabled

IMHO this is also a nicer and more integrated design.
Now: implementation wise, this flow seems compliant with the production-readiness review (see the details in https://kubernetes.slack.com/archives/CPNHUMN74/p1620312071045800) because new features should depend on A compatible and related feature gate; a new feature gate would be alpha level, and the cpuManagerOptions feature gate will still be alpha in 1.23.

So reshaping to use the cpuManagerOptions still seems a totally valid option and I think is cleaner from overall design perspective.
Let's see what other reviewers (@klueska :) ) think, and please let me know if there are concerns or requirements that I missed.

ffromani · 2021-05-19T08:24:58Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

-  - Also, more than one l3 cache should exist in a single socket/package.
-
+  - C1: Add `CPUManagerUncoreCacheAlign` to kubelet's feature-gates to enable(true)/disable(false) the feature.
+  - C2: More than one l3 cache should exist in a single socket/package(uncore-cache exists).


I'm not quite sure what "C1" and "C2" mean in this context.

dchen1107 · 2021-06-08T17:01:41Z

Thanks for making LLC cache cadvisor aware first. Here is some native questions form the top of my head since I didn't find the answer from the first pass of the KEP:

Are you proposing to include current CPU management proposal as one of the policies? Or an enhancement to the proposed policy?
Do you plan to extend this to cluster aware scheduling policy or just keep at Kubelet / Node level? If KEP, looks like you proposed a node level optimization.
If Kubelet cannot commit such requests, should Kubelet reject the admission of some pods? Or this is a best effort?

marquiz · 2021-06-09T04:07:55Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

+	Id           int     `json:"core_id"`
+	Threads      []int   `json:"thread_ids"`
+	Caches       []Cache `json:"caches"`
+	UncoreCaches []Cache `json:"uncore_caches"`


Hmm, this looks like a unnecessary/confusing kludge. Would it be possible to present this similarly to how the linux kernel does, i.e. add the information about cpus in the Cache structure (like /sys/devices/system/cpu/cpu*/cache/index*/shared_cpu_list)?

Hi, @marquiz, it is an interesting problem. Previously, i've made a consideration here. And then, i want desgign here to be decoupling. But seems no other structure is more fit to place the uncore-cache information.
I think:

For a cache, it shouldn't get knowleage of whether it is uncore, that's not a cache's charater. Infos in /sys/devices/system/cpu/cpu*/cache/index*/ could not tell us if the cache is uncore without information about socket/core.

But, core is awareness of the cache and uncore-cache it uses, and for a core, it include {id,threads, caches, uncore caches}.
Dicussion is welcomed, thanks.

Hi @ranchothu , in my understanding you'd like to differentiate between node level cache (typically on Intel chips) and uncore cache (typically on AMD chips), if so, why do we need to differentiate between them? they are both L3 cache and it seems that they can both be aligned when assign CPU.

Here I agree that we need additional layer of abstraction since number of grouped CPU per L3 cache is not a feature of only AMD's processor, for example Kunpen920 (armv8) had 4 cpu per L3 cache, and had 24 of such blocks.

enzoyes · 2021-06-13T02:18:23Z

Hi, @dchen1107 , thanks for your advice, and sorry for miss of the meeting.

I am adding an enhancement to existing policy, and logic to basic cpu allocation(1st problem you proposed).
As i've descrived in Feature gate, a new feature gate is added to enable/disable the feature. And also, if no uncore-cache(captured from cadvisor api) exists in the architecture, the feature is disabled even feature gate is open.(2nd)
In General Design, i think the kubelet cpu allocation in each level is always best effort(try node->socket->core->cpu), and the path for me is (node->socket->uncore-cache->core->cpu) , each level is cluster of cpus sharing some common character. It will try cpus in same core if uncore-cache affinity failed.(3th)

May the explanation above answer your question. And look forword for future process.:smile:

jfbai · 2021-07-07T08:29:25Z

keps/sig-node/2621-cpu-allocation-llc-affinity/README.md

+### Graduation Criteria
+#### Alpha
+
+ - Implement the new policy.


According to the implementation, strictly it is not a new policy but an enhancement against existing static policy right?

ffromani · 2021-08-10T08:23:38Z

Hi! just a friendly reminder that the deadline for 1.23 KEP planning is 9th of September 2021: https://docs.google.com/document/d/1U10J0WwgWXkdYrqWGGvO8iH2HKeerQAlygnqgDgWv4E/edit# . If we want this enhancement to be in 1.23 we need some actions. sig-node is planning the 1.23 enhancements in today's meeting: https://docs.google.com/document/d/1Ne57gvidMEWXR70OxxnRkYquAoMpt56o75oZtg-OeBg/edit - please consider proposing this change in this meeting or next-week meeting.

k8s-ci-robot · 2021-09-10T17:23:45Z

@ranchothu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-enhancements-test	`09ffea5`	link	`/test pull-enhancements-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-triage-robot · 2021-12-09T18:06:13Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-01-08T18:26:10Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-02-07T19:25:23Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-02-07T19:25:40Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ffromani · 2024-06-25T06:52:39Z

@enzoyes hi! are still interested in pushing forward this work?

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 6, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 6, 2021

k8s-ci-robot requested review from dchen1107 and derekwaynecarr May 6, 2021 13:09

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels May 6, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 6, 2021

enzoyes force-pushed the master branch 3 times, most recently from ba5f08d to ccf74f1 Compare May 6, 2021 13:43

enzoyes force-pushed the master branch from ccf74f1 to c05d08d Compare May 6, 2021 14:00

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2021

enzoyes force-pushed the master branch from c05d08d to 12cdee5 Compare May 7, 2021 02:09

enzoyes force-pushed the master branch 3 times, most recently from 0a40710 to 40c8fd4 Compare May 7, 2021 03:08

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 7, 2021

k8s-ci-robot removed the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 7, 2021

enzoyes added 2 commits May 14, 2021 20:20

KEP-2621: Add llc affinity to cpu manager.

667b56d

KEP-2621: Add feature gate.

b3013b2

enzoyes force-pushed the master branch from 7fb3c8c to b3013b2 Compare May 14, 2021 12:21

swatisehgal reviewed May 17, 2021

View reviewed changes

KEP-2621: Clarify more, fix some spelling problems.

09ffea5

ffromani reviewed May 19, 2021

View reviewed changes

enzoyes mentioned this pull request May 26, 2021

[WIP]add uncore-cache align to cpu allocation kubernetes/kubernetes#102307

Closed

marquiz reviewed Jun 9, 2021

View reviewed changes

ffromani mentioned this pull request Jun 10, 2021

Add cpumanager policy options kubernetes/kubernetes#102055

Closed

jfbai reviewed Jul 7, 2021

View reviewed changes

swatisehgal mentioned this pull request Sep 8, 2021

KEP-2625: Update CPU Manager Policy Options 1.23 Beta #2933

Merged

swatisehgal mentioned this pull request Sep 30, 2021

REQUEST: New kubernetes-sigs membership for @swatisehgal kubernetes/org#3023

Closed

7 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 9, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 8, 2022

k8s-ci-robot closed this Feb 7, 2022

sphrasavath mentioned this pull request Jul 17, 2024

Enhance CPU manager with L3 cache aware #2621

Closed

4 tasks


		![design_overview](design_overview.png "design_overview")

		- feature-gates `CPUManagerUncoreCacheAlign`

KEP-2621: Add llc affinity to cpu manager. #2684

KEP-2621: Add llc affinity to cpu manager. #2684

Conversation

enzoyes commented May 6, 2021 • edited Loading

k8s-ci-robot commented May 6, 2021

k8s-ci-robot commented May 6, 2021

k8s-ci-robot commented May 6, 2021

k8s-ci-robot commented May 6, 2021

k8s-ci-robot commented May 6, 2021

ehashman commented May 6, 2021

pacoxu commented May 7, 2021

enzoyes commented May 7, 2021

pacoxu commented May 7, 2021

enzoyes commented May 7, 2021

enzoyes commented May 7, 2021

swatisehgal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enzoyes commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enzoyes commented Jun 13, 2021

Choose a reason for hiding this comment

ffromani commented Aug 10, 2021

k8s-ci-robot commented Sep 10, 2021

k8s-triage-robot commented Dec 9, 2021

k8s-triage-robot commented Jan 8, 2022

k8s-triage-robot commented Feb 7, 2022

k8s-ci-robot commented Feb 7, 2022

ffromani commented Jun 25, 2024

enzoyes commented May 6, 2021 •

edited

Loading