[kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs #6051

shlomibitton · 2020-11-26T10:09:46Z

Signed-off-by: Shlomi Bitton shlomibi@nvidia.com

- Why I did it
Usually for a use case like networking - should not be configured to reach c6, the maximum used is c1e – due to the added latency getting in & out of states (bad for networking).

Following a recommendation by Intel, networking system should avoid getting in & out of states which introduce latency. The recommended state is c1e and no state change enabling.

In addition, c-state sole purpose is to save power and when inside a networking switch its really negligent being such a tiny consumer vs. the whole cluster.

- How I did it
Added a new argument to the grub cmdline to set c-states to 0.

- How to verify it

install 'linux-cpupower' tool.
Run 'turbostat --debug' command
Observe the c-states is constantly on state 1.

- Which release branch to backport (provide reason below if selected)

201811
201911
202006

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

lguohan · 2020-11-26T11:46:34Z

retest vsimage please

lguohan · 2020-11-26T11:46:55Z

what if the platform uses non intel cpu? like amd cpu or arm cpu?

liat-grozovik · 2020-11-30T15:52:33Z

@shlomibitton please check if we can read a parameter which identify this is intel cpu and add it only for it.

shlomibitton · 2020-11-30T20:27:51Z

@lguohan I added a condition to change it only for 'Intel' CPUs.
can you please review the change?

installer/x86_64/install.sh

qiluo-msft

As comment

lguohan · 2020-12-03T03:20:00Z

Observe the c-states is constantly on state 1.

is this incorrect? state should be 0, right?

lguohan · 2020-12-03T03:29:53Z

check out this one. why do they set both processor.max-cstate=1 and intel_idle.max_cstate=0? why do you only set intel_idle.max_cstate?

https://gist.github.com/wmealing/2dd2b543c4d3cff6cab7

shlomibitton · 2020-12-03T12:18:42Z

@lguohan

Observe the c-states is constantly on state 1.

is this incorrect? state should be 0, right?

No, state 0 means the CPU is currently active.
by the turbostat tool man page:
Busy% percent of the measurement interval that the CPU executes instructions, aka. % of time in "C0" state.
This is an example of an output when running the tool with c-states = 0.
As you can see when it is not "busy" i.e. "idle" it is in state C1, which is expected and desired.

check out this one. why do they set both processor.max-cstate=1 and intel_idle.max_cstate=0? why do you only set intel_idle.max_cstate?

https://gist.github.com/wmealing/2dd2b543c4d3cff6cab7

by using "processor.max-cstate=1" we limit the c-states level to a certain level, but this is not needed when using "intel_idle.max_cstate=0" because this is disables the driver, forcing it to stay active all the time.
you can check in the kernel code base the file "drivers/idle/intel_idle.c" there is an indication for it:
/* intel_idle.max_cstate=0 disables driver */

Unblock, and LGTM

…6051) Usually for a use case like networking - should not be configured to reach c6, the maximum used is c1e – due to the added latency getting in & out of states (bad for networking). Following a recommendation by Intel, networking system should avoid getting in & out of states which introduce latency. The recommended state is c1e and no state change enabling. In addition, c-state sole purpose is to save power and when inside a networking switch its really negligent being such a tiny consumer vs. the whole cluster. Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

The motivation of these changes is to fix (#6051): - Why I did it To fix CPU cstates configuration - How I did it Updated code to be POSIX compatible - How to verify it root@sonic:/home/admin# sonic_installer install sonic-mellanox.bin Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>

The motivation of these changes is to fix (sonic-net#6051): - Why I did it To fix CPU cstates configuration - How I did it Updated code to be POSIX compatible - How to verify it root@sonic:/home/admin# sonic_installer install sonic-mellanox.bin Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>

Why I did it This is a fix for PR [kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs by shlomibitton · Pull Request #6051 · sonic-net/sonic-buildimage (github.com) The original PR will disable intel idle driver but it cannot limit the max c-state to 1 due to system will fall back to acpi idle driver. Currently intel_idle.max_cstate=0 is already present, which will disable intel idle driver. With the added option, common idle driver will be disabled as well, so there will not be idle management. This is to prevent a bug that can be triggered by idle instruction on intel platform. How I did it Add the option to installer file beside intel_idle.max_cstate=0

Why I did it This is a fix for PR [kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs by shlomibitton · Pull Request sonic-net#6051 · sonic-net/sonic-buildimage (github.com) The original PR will disable intel idle driver but it cannot limit the max c-state to 1 due to system will fall back to acpi idle driver. Currently intel_idle.max_cstate=0 is already present, which will disable intel idle driver. With the added option, common idle driver will be disabled as well, so there will not be idle management. This is to prevent a bug that can be triggered by idle instruction on intel platform. How I did it Add the option to installer file beside intel_idle.max_cstate=0

Why I did it This is a fix for PR [kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs by shlomibitton · Pull Request #6051 · sonic-net/sonic-buildimage (github.com) The original PR will disable intel idle driver but it cannot limit the max c-state to 1 due to system will fall back to acpi idle driver. Currently intel_idle.max_cstate=0 is already present, which will disable intel idle driver. With the added option, common idle driver will be disabled as well, so there will not be idle management. This is to prevent a bug that can be triggered by idle instruction on intel platform. How I did it Add the option to installer file beside intel_idle.max_cstate=0

…tel cpu (#16371) This is a fix for PR #6051 The original PR will disable intel idle driver but it cannot limit the max c-state to 1 due to system will fall back to acpi idle driver. Currently intel_idle.max_cstate=0 is already present, which will disable intel idle driver. With the added option, common idle driver will be disabled as well, so there will not be idle management. This is to prevent a bug that can be triggered by idle instruction on intel platform. Work item tracking Microsoft ADO (number only): 24867921 How I did it Add the option to installer file beside intel_idle.max_cstate=0 Signed-off-by: Xichen Lin <lukelin0907@gmail.com>

Why I did it This is a fix for PR [kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs by shlomibitton · Pull Request sonic-net#6051 · sonic-net/sonic-buildimage (github.com) The original PR will disable intel idle driver but it cannot limit the max c-state to 1 due to system will fall back to acpi idle driver. Currently intel_idle.max_cstate=0 is already present, which will disable intel idle driver. With the added option, common idle driver will be disabled as well, so there will not be idle management. This is to prevent a bug that can be triggered by idle instruction on intel platform. How I did it Add the option to installer file beside intel_idle.max_cstate=0

This is a fix for PR [kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs by shlomibitton · Pull Request #6051 · sonic-net/sonic-buildimage (github.com) The original PR will disable intel idle driver but it cannot limit the max c-state to 1 due to system will fall back to acpi idle driver. Currently intel_idle.max_cstate=0 is already present, which will disable intel idle driver. With the added option, common idle driver will be disabled as well, so there will not be idle management. This is to prevent a bug that can be triggered by idle instruction on intel platform. Work item tracking Microsoft ADO (number only): 24867921

Change grub cmdline to set c-states to 0

858434b

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

Check if CPU vendor is Intel

b211331

shlomibitton changed the title ~~[kernel] Change grub cmdline to set c-states to 0~~ [kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs Dec 1, 2020

lguohan reviewed Dec 2, 2020

View reviewed changes

installer/x86_64/install.sh Outdated Show resolved Hide resolved

lguohan requested a review from qiluo-msft December 2, 2020 20:08

qiluo-msft reviewed Dec 2, 2020

View reviewed changes

installer/x86_64/install.sh Show resolved Hide resolved

qiluo-msft previously requested changes Dec 2, 2020

View reviewed changes

Update install.sh

48a0298

lguohan approved these changes Dec 7, 2020

View reviewed changes

qiluo-msft approved these changes Dec 7, 2020

View reviewed changes

lguohan added the Request for 201911 Branch label Dec 7, 2020

lguohan merged commit 43a32e6 into sonic-net:master Dec 7, 2020

abdosi added the Included in 201911 Branch label Dec 10, 2020

shlomibitton deleted the shlomi_disable_cstates branch March 24, 2021 20:37

nazariig mentioned this pull request Apr 2, 2021

[install.sh] Fix CPU cstates configuration #7222

Merged

4 tasks

This was referenced Aug 31, 2023

[installer] add processor.max_cstate=1 to intel kernel cmdline #16339

Merged

[installer] add processor.max_cstate=1 to intel kernel cmdline #16340

Merged

mssonicbld mentioned this pull request Aug 31, 2023

[action] [PR:16339] [installer] add processor.max_cstate=1 to intel kernel cmdline #16365

Merged

11 tasks

Xichen96 mentioned this pull request Sep 1, 2023

[installer] add processor.max_cstate=1 to intel kernel cmdline #16371

Merged

11 tasks

mssonicbld mentioned this pull request Sep 3, 2023

[action] [PR:16339] [installer] add processor.max_cstate=1 to intel kernel cmdline #16425

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs #6051

[kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs #6051

shlomibitton commented Nov 26, 2020

lguohan commented Nov 26, 2020

lguohan commented Nov 26, 2020

liat-grozovik commented Nov 30, 2020

shlomibitton commented Nov 30, 2020 •

edited

Loading

qiluo-msft left a comment

lguohan commented Dec 3, 2020

lguohan commented Dec 3, 2020

shlomibitton commented Dec 3, 2020 •

edited

Loading

[kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs #6051

[kernel] Change grub cmdline to set c-states to 0 for "Intel" CPUs #6051

Conversation

shlomibitton commented Nov 26, 2020

lguohan commented Nov 26, 2020

lguohan commented Nov 26, 2020

liat-grozovik commented Nov 30, 2020

shlomibitton commented Nov 30, 2020 • edited Loading

qiluo-msft left a comment

Choose a reason for hiding this comment

lguohan commented Dec 3, 2020

lguohan commented Dec 3, 2020

shlomibitton commented Dec 3, 2020 • edited Loading

shlomibitton commented Nov 30, 2020 •

edited

Loading

shlomibitton commented Dec 3, 2020 •

edited

Loading