Hi Amit,
On 30 June 2014 22:43, Amit Kucheria amit.kucheria@linaro.org wrote:
Some nits below:
On Tue, Jul 1, 2014 at 5:35 AM, Lisa Nguyen lisa.nguyen@linaro.org wrote:
Run the cpuhotplug test suite on all cpus including cpu0. If we wish to skip cpu0, pass the parameter, hotplug_allow_cpu0=1, to make check like in these examples:
Umm, this description is the opposite of what the default code does.
cpuhotplug testsuite does *not* run on cpu0 by default. If we wish to *allow* cpu0, we set hotplug_allow_cpu0 to 1.
Honest mistake. Somehow I was thinking hotplug_allow_cpu0=0 as true, =1 as false. You're correct: cpuhotplug does not run on cpu0 by default unless we pass in the parameter.
sudo make -C cpuhotplug hotplug_allow_cpu0=1 check sudo make hotplug_allow_cpu0=1 check
Changes from v1 to v2:
- Rename the parameter from hotplug_cpu_start to hotplug_allow_cpu0
- Add is_cpu0_allowed() function to check if we want to start running
test scripts from cpu0..cpuN
Signed-off-by: Lisa Nguyen lisa.nguyen@linaro.org
Makefile | 3 ++- cpuhotplug/Makefile | 2 +- cpuhotplug/cpuhotplug_02.sh | 3 +-- cpuhotplug/cpuhotplug_03.sh | 2 +- cpuhotplug/cpuhotplug_04.sh | 2 +- cpuhotplug/cpuhotplug_05.sh | 4 ++-- cpuhotplug/cpuhotplug_06.sh | 4 ++-- cpuhotplug/cpuhotplug_07.sh | 2 +- cpuhotplug/cpuhotplug_08.sh | 7 ++++++- include/functions.sh | 12 ++++++++++++ 10 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/Makefile b/Makefile index 731619d..9f26fd3 100644 --- a/Makefile +++ b/Makefile @@ -21,6 +21,7 @@ # Torez Smith torez.smith@linaro.org (IBM Corporation) # - initial API and implementation # +hotplug_allow_cpu0?=0
all: @(cd utils; $(MAKE)) @@ -28,7 +29,7 @@ all: check: @(cd utils; $(MAKE) check) @(cd cpufreq; $(MAKE) check)
@(cd cpuhotplug; $(MAKE) check)
@(cd cpuhotplug; $(MAKE) hotplug_allow_cpu0=${hotplug_allow_cpu0} check) @(cd cpuidle; $(MAKE) check)# @(cd suspend; $(MAKE) check) @(cd thermal; $(MAKE) check) diff --git a/cpuhotplug/Makefile b/cpuhotplug/Makefile index df0b8f4..6ee600d 100644 --- a/cpuhotplug/Makefile +++ b/cpuhotplug/Makefile @@ -21,5 +21,5 @@ # Daniel Lezcano daniel.lezcano@linaro.org (IBM Corporation) # - initial API and implementation #
+export hotplug_allow_cpu0?=0 include ../Test.mk diff --git a/cpuhotplug/cpuhotplug_02.sh b/cpuhotplug/cpuhotplug_02.sh index 3157307..58e1f02 100755 --- a/cpuhotplug/cpuhotplug_02.sh +++ b/cpuhotplug/cpuhotplug_02.sh @@ -24,7 +24,6 @@ #
# URL : https://wiki.linaro.org/WorkingGroups/PowerManagement/Resources/TestSuite/Pm...
source ../include/functions.sh
check_state() { @@ -34,7 +33,7 @@ check_state() { shift 1
if [ "$cpu" == "cpu0" ]; then
return 0
is_cpu0_allowed $cpu $hotplug_allow_cpu0 || return 0Since is_cpu0_allowed has cpu0 in its name, why pass it $cpu as a parameter? You are already checking for cpu0 in the if statement. This can be simplified as
if [ "$cpu" == "cpu0" ]; then is_cpu0_allowed $hotplug_allow_cpu0 || return 0
Yes, I agree with that.
fi set_offline $cpudiff --git a/cpuhotplug/cpuhotplug_03.sh b/cpuhotplug/cpuhotplug_03.sh index 13a0ce9..b29ff8e 100755 --- a/cpuhotplug/cpuhotplug_03.sh +++ b/cpuhotplug/cpuhotplug_03.sh @@ -34,7 +34,7 @@ check_affinity_fails() { local ret=
if [ "$cpu" == "cpu0" ]; then
return 0
is_cpu0_allowed $cpu $hotplug_allow_cpu0 || return 0fi
set_offline $cpu
diff --git a/cpuhotplug/cpuhotplug_04.sh b/cpuhotplug/cpuhotplug_04.sh index 394a512..543853f 100755 --- a/cpuhotplug/cpuhotplug_04.sh +++ b/cpuhotplug/cpuhotplug_04.sh @@ -37,7 +37,7 @@ check_task_migrate() { local ret=
if [ "$cpu" == "cpu0" ]; then
return 0
is_cpu0_allowed $cpu $hotplug_allow_cpu0 || return 0fi
taskset 0x$cpumask $CPUBURN $cpu &
diff --git a/cpuhotplug/cpuhotplug_05.sh b/cpuhotplug/cpuhotplug_05.sh index a8eb312..2f33a0c 100755 --- a/cpuhotplug/cpuhotplug_05.sh +++ b/cpuhotplug/cpuhotplug_05.sh @@ -33,9 +33,9 @@ check_procinfo() { local ret=
if [ "$cpu" == "cpu0" ]; then
return 0
is_cpu0_allowed $cpu $hotplug_allow_cpu0 || return 0
set_offline $cpu
TAB=$(printf "\t"); grep "processor$TAB: $cpuid" /proc/cpuinfo
diff --git a/cpuhotplug/cpuhotplug_06.sh b/cpuhotplug/cpuhotplug_06.sh index 347906d..528aeca 100755 --- a/cpuhotplug/cpuhotplug_06.sh +++ b/cpuhotplug/cpuhotplug_06.sh @@ -33,9 +33,9 @@ check_procinfo() { local ret=
if [ "$cpu" == "cpu0" ]; then
return 0
is_cpu0_allowed $cpu $hotplug_allow_cpu0 || return 0
set_offline $cpu
grep "CPU$cpuid" /proc/interrupts
diff --git a/cpuhotplug/cpuhotplug_07.sh b/cpuhotplug/cpuhotplug_07.sh index eaeba77..00fd009 100755 --- a/cpuhotplug/cpuhotplug_07.sh +++ b/cpuhotplug/cpuhotplug_07.sh @@ -35,7 +35,7 @@ check_notification() { local ret=
if [ "$cpu" == "cpu0" ]; then
return 0
is_cpu0_allowed $cpu $hotplug_allow_cpu0 || return 0fi
# damn ! udevadm is buffering the output, we have to use a temp file
diff --git a/cpuhotplug/cpuhotplug_08.sh b/cpuhotplug/cpuhotplug_08.sh index 9e2c355..0b1b9b6 100755 --- a/cpuhotplug/cpuhotplug_08.sh +++ b/cpuhotplug/cpuhotplug_08.sh @@ -28,7 +28,12 @@ source ../include/functions.sh
function randomize() {
- echo $[ ( $RANDOM % $1 ) + 1 ]
- #if we are skipping cpu0
- if [ $hotplug_allow_cpu0 -ne 0 ]; then
echo $[ ( $RANDOM % $1 ) + 1 ]- else
echo $[ ( $RANDOM % $1 ) ]- fi
}
random_stress() { diff --git a/include/functions.sh b/include/functions.sh index 6d75e34..3ec2270 100644 --- a/include/functions.sh +++ b/include/functions.sh @@ -372,3 +372,15 @@ is_root() { fi return $ret }
+is_cpu0_allowed() {
I suggest renaming this to is_cpu0_hotplug_allowed() to make functions.sh easy to read later
Will do.
local cpu=$1remove cpu variable
Will do.
local status=$2# Allow cpu0 to be testedif [[ "$cpu" == "cpu0" && $status -eq 0 ]]; thencpu == cpu0 check can be removed.
Okay.
return 0elsereturn 1fi+}
1.7.9.5
Thanks for the feedback. Will respin and send v3.