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

[finalize-warmboot.sh] reset cpufreq governor to default #19634

Merged
merged 4 commits into from
Nov 4, 2024
Merged

[finalize-warmboot.sh] reset cpufreq governor to default #19634

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51ef6a2
[finalize-warmboot.sh] reset cpufreq governor to default
stepanblyschak Jul 18, 2024
d53703e
Handle review comments
stepanblyschak Jul 18, 2024
21a6851
set only on mellanox platform
stepanblyschak Oct 15, 2024
8aef814
fix grep
stepanblyschak Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions files/image_config/warmboot-finalizer/finalize-warmboot.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,30 @@ function check_list()
echo ${RET_LIST}
}

function set_cpufreq_governor() {
local -r governor="$1"
echo "$governor" | tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor 1> /dev/null \
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this setting (or the way we are writing to a file) be effective at runtime?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

&& debug "Set CPUFreq scaling governor to $governor" \
|| debug "Failed to set CPUFreq scaling governor to $governor"
}

function finalize_common() {
# Read default governor from kernel config
local -r default_governor=$(cat "/boot/config-$(uname -r)" | grep -o 'CONFIG_CPU_FREQ_DEFAULT_GOV_[^=]*=y')
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the guarantee that "/boot/config-$(uname -r)" will have the default governor setting?

Do you need to sanity check on the value and type of default_governor here? What if the cat or grep result in a failure? Wouldn't the code proceed to assign invalid value to /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's part of our sonic linux kernel config, why would we remove it or set it to invalid value?

set_cpufreq_governor "$default_governor"
Copy link
Contributor

Choose a reason for hiding this comment

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

The utilities change is made for MLNX platforms only. I think it would be best to make this call only for MLNX platforms as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @vaibhavhd I applied your suggestion. While testing that modification found that the | grep is incorrect causing governor to not reset to default. That bug was introduced in "Handle review comments" commit, don't remember why I did it, but I reverted to previous default governor parsing method.

}

function finalize_warm_boot()
{
debug "Finalizing warmboot..."
finalize_common
sudo config warm_restart disable
}

function finalize_fast_reboot()
{
debug "Finalizing fast-reboot..."
finalize_common
sonic-db-cli STATE_DB hset "FAST_RESTART_ENABLE_TABLE|system" "enable" "false" &>/dev/null
sonic-db-cli CONFIG_DB DEL "WARM_RESTART|teamd" &>/dev/null
}
Expand Down
Loading