Hi Amit,
On 30 June 2014 22:43, Amit Kucheria <amit.kuche...@linaro.org> wrote:
> Some nits below:
>
> On Tue, Jul 1, 2014 at 5:35 AM, Lisa Nguyen <lisa.ngu...@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.ngu...@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.sm...@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.lezc...@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/PmQaSpecification#cpuhotplug_02
>> -
>> 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 0
>
> Since 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 $cpu
>> diff --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 0
>> fi
>>
>> 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 0
>> fi
>>
>> 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
>> fi
>> -
>> +
>> 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
>> fi
>> -
>> +
>> 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 0
>> fi
>>
>> # 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=$1
>
> remove cpu variable
>
Will do.
>> + local status=$2
>> +
>> + # Allow cpu0 to be tested
>> + if [[ "$cpu" == "cpu0" && $status -eq 0 ]]; then
>
> cpu == cpu0 check can be removed.
>
Okay.
>> + return 0
>> + else
>> + return 1
>> + fi
>> +}
>> --
>> 1.7.9.5
>>
Thanks for the feedback. Will respin and send v3.
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
