Sort CPUs/cores in disable turboboost script numerically. #95
Conversation
This results in a slightly easier to understand outcome. On CPUs like where there are 8+8 cores with hyperthreading and cpu0 is on the same core as cpu9, cpu1 and cpu10 etc. Since the script went through cpus lexicographically, it did traverse cpu 10-15 before doing 2-9. This resulted in disabling cpus 2-9, while cpu0, cpu1 and cpu 10-15 were online which is very confusing. Now it sorts things numerically, so goes through them in a more predictable order and the results is more predictable: in this particular case, cpu 0-7 are online, and cpu 8-15 are disabled. In more general words: the cpu with the lowest number of each core is still online, higher numbers are disabled.
meisterT
force-pushed
the
meisterT-patch-1
branch
from
d7dd56c
to
cf046c1
Compare
There was a problem hiding this comment.
LGTM, had this exact issue and this is definitely an improvement!
Have made a suggestion for fixing shellcheck. The fix is less legible than the current change for me. Since I think legibility is important here my preferred solution would be the ls approach. Hence the approval.
Testing on two multi (2) cpu machines (intel, ubuntu 22.04, kernel 5.15.0) shows that running this disables the entire second cpu. I cannot recall whether testing showed that this should be better for timing. But it is also confusing that this happens, and (I think, definitely in my case) oftentimes undesired.
To fix this, line 24 needs to be changed to something similar to
core_id=$(cat $cpu/topology/core_id $cpu/topology/physical_package_id) maybe add a | tr '\n' ',' to have a 'nice' single-line key. This should make the key unique for every core on every cpu.
| @@ -5,7 +5,7 @@ shopt -s extglob | |||
|
|
|||
| declare -A core_ids | |||
|
|
|||
| for cpu in /sys/devices/system/cpu/cpu* ; do | |||
| for cpu in $(ls -1d /sys/devices/system/cpu/cpu* | sort --version-sort) ; do | |||
There was a problem hiding this comment.
The following command should give the same output (verified locally) and should make shellcheck happy:
find /sys/devices/system/cpu -maxdepth 1 -mindepth 1 -name 'cpu*' | sort --version-sort.
The max- and mindepth flags are needed to ensure it only lists the cpu's and skips the directory itself.
There was a problem hiding this comment.
What is shellcheck unhappy about? I'm fine with suppressing an issue that is unlikely to cause harm.
There was a problem hiding this comment.
./provision-contest/disable-turboboost_ht:8:14: note: Use find instead of ls to better handle non-alphanumeric filenames. [SC2012]
use find instead of ls
There was a problem hiding this comment.
Pfff, that's just random advice that doesn't apply here inside /sys/.... Let's suppress.
There was a problem hiding this comment.
@eldering so this can be merged as is?
This results in a slightly easier to understand outcome.
On CPUs like where there are 8+8 cores with hyperthreading and cpu0 is on the same core as cpu9, cpu1 and cpu10 etc. Since the script went through cpus lexicographically, it did traverse cpu 10-15 before doing 2-9. This resulted in disabling cpus 2-9, while cpu0, cpu1 and cpu 10-15 were online which is very confusing.
Now it sorts things numerically, so goes through them in a more predictable order and the results is more predictable: in this particular case, cpu 0-7 are online, and cpu 8-15 are disabled.
In more general words: the cpu with the lowest number of each core is still online, higher numbers are disabled.