-
-
Notifications
You must be signed in to change notification settings - Fork 533
fix(dietpi-cpuinfo): give each core a chance to scale down before checking its frequency #7700
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
base: dev
Are you sure you want to change the base?
Conversation
Without this sleep, the shell script would always have caused the cpu to scale up before its current frequency was measured. There is no ideal solution to this and the delay is arbitrary, but even such small sleep value significantly reduces the impact that the script has on frequency measures.
Compare by running ExamplesNote: these are very rudimentary; there's always something else running on the system as well - the point is: to be helpful at all, Odroid C2In a few dozen runs without the delay, the Before:
After:
Odroid C4
Other delays yield very similar values on average. |
do | ||
[[ -f /sys/devices/system/cpu/cpu$i/cpufreq/scaling_cur_freq ]] && read -r aCPU_CUR_FREQ["$i"] < "/sys/devices/system/cpu/cpu$i/cpufreq/scaling_cur_freq" | ||
[[ -f /sys/devices/system/cpu/cpu$i/cpufreq/scaling_cur_freq ]] && { | ||
sleep 0.05 # Necessary to give the scheduler a chance to scale down the core in (quite likely) case this script has already caused it to ramp up the frequency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sleep 0.05 # Necessary to give the scheduler a chance to scale down the core in (quite likely) case this script has already caused it to ramp up the frequency | |
G_SLEEP 0.05 # Necessary to give the scheduler a chance to scale down the core in (quite likely) case this script has already caused it to ramp up the frequency |
/bin/sleep
is an external executable, expensive to call. We have a G_SLEEP
function which provides the functionality very cheaply with pure shell features. This also reduces the processing for the sleep
call itself significantly.
Generally a good idea. We had a single sleep before the cores loop in earlier versions of the script. But removed it at some point, since it did not work so well. A sleep before each individual core should indeed work better (obviously does, as you show). On systems with many cores, this can cause a significant lag 🤔. There are DietPi systems with 240 cores, i.e. 12 seconds sleep overall with 0.05s each. On such systems, it is also very unlikely that all core's frequencies are scaled up significantly for a single tiny bash script call. So I think we'd need some negative scaling with the numbers of cores, skipping the sleep completely at some point. We could give it e.g. 0.5s overall, split across the cores, and skip if in case of 12+ cores or so, which indicates no SBC. Can you rebase the commit onto |
Without this sleep, the shell script would always have caused the cpu to scale up before its current frequency was measured. There is no ideal solution to this and the delay is arbitrary, but even such small sleep value significantly reduces the impact that the script has on frequency measurements.