Hi Gautham, Thanks for the review.
On 20/07/20 11:22 am, Gautham R Shenoy wrote:
Hi Pratik,
On Fri, Jul 17, 2020 at 02:48:01PM +0530, Pratik Rajesh Sampat wrote:
This patch adds support to trace IPI based and timer based wakeup latency from idle states
Latches onto the test-cpuidle_latency kernel module using the debugfs interface to send IPIs or schedule a timer based event, which in-turn populates the debugfs with the latency measurements.
Currently for the IPI and timer tests; first disable all idle states and then test for latency measurements incrementally enabling each state
Signed-off-by: Pratik Rajesh Sampat psampat@linux.ibm.com
A few comments below.
tools/testing/selftests/Makefile | 1 + tools/testing/selftests/cpuidle/Makefile | 6 + tools/testing/selftests/cpuidle/cpuidle.sh | 257 +++++++++++++++++++++ tools/testing/selftests/cpuidle/settings | 1 + 4 files changed, 265 insertions(+) create mode 100644 tools/testing/selftests/cpuidle/Makefile create mode 100755 tools/testing/selftests/cpuidle/cpuidle.sh create mode 100644 tools/testing/selftests/cpuidle/settings
[..skip..]
+ins_mod() +{
- if [ ! -f "$MODULE" ]; then
printf "$MODULE module does not exist. Exitting\n"If the module has been compiled into the kernel (due to a localyesconfig, for instance), then it is unlikely that we will find it in /lib/modules. Perhaps you want to check if the debugfs directories created by the module exist, and if so, print a message saying that the modules is already loaded or some such?
That's a good idea. I can can grep for this module within /proc/modules and not insert it, if it is already there
exit $ksft_skip- fi
- printf "Inserting $MODULE module\n\n"
- insmod $MODULE
- if [ $? != 0 ]; then
printf "Insmod $MODULE failed\n"exit $ksft_skip- fi
+}
+compute_average() +{
- arr=("$@")
- sum=0
- size=${#arr[@]}
- for i in "${arr[@]}"
- do
sum=$((sum + i))- done
- avg=$((sum/size))
It would be good to assert that "size" isn't 0 here.
Sure
+}
+# Disable all stop states +disable_idle() +{
- for ((cpu=0; cpu<NUM_CPUS; cpu++))
- do
for ((state=0; state<NUM_STATES; state++))doecho 1 > /sys/devices/system/cpu/cpu$cpu/cpuidle/state$state/disableSo, on offlined CPUs, we won't see /sys/devices/system/cpu/cpu$cpu/cpuidle/state$state directory. You should probably perform this operation only on online CPUs.
Right. I should make CPU operations only on online CPUs all over the script
[..snip..]
Thanks Pratik