Messages in this thread | | | Subject | Re: [PATCH v2 2/2] selftest/cpuidle: Add support for cpuidle latency measurement | From | Pratik Sampat <> | Date | Tue, 21 Jul 2020 17:26:49 +0530 |
| |
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++)) >> + do >> + echo 1 > /sys/devices/system/cpu/cpu$cpu/cpuidle/state$state/disable > So, 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
| |