[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[Xiao Yang]

1) Replace sar command with /proc/stat
2) Convert to new API
3) Remove unused/duplicated code

Signed-off-by: Xiao Yang <ice_yangxiao@163.com>
---
 runtest/cpuhotplug                            |   2 +-
 .../cpu_hotplug/functional/cpuhotplug05.sh    | 186 ++++++------------
 2 files changed, 63 insertions(+), 125 deletions(-)

diff --git a/runtest/cpuhotplug b/runtest/cpuhotplug
index ec7f11ed1..bd97e01b8 100644
--- a/runtest/cpuhotplug
+++ b/runtest/cpuhotplug
@@ -4,6 +4,6 @@
 cpuhotplug02 cpuhotplug02.sh -c 1 -l 1
 cpuhotplug03 cpuhotplug03.sh -c 1 -l 1
 cpuhotplug04 cpuhotplug04.sh -l 1
-cpuhotplug05 cpuhotplug05.sh -c 1 -l 1 -d /tmp
+cpuhotplug05 cpuhotplug05.sh -c 1
 cpuhotplug06 cpuhotplug06.sh -c 1 -l 1
 cpuhotplug07 cpuhotplug07.sh -c 1 -l 1 -d /usr/src/linux
diff --git a/testcases/kernel/hotplug/cpu_hotplug/functional/cpuhotplug05.sh b/testcases/kernel/hotplug/cpu_hotplug/functional/cpuhotplug05.sh
index 95a8f4a2d..167cbc4a7 100755
--- a/testcases/kernel/hotplug/cpu_hotplug/functional/cpuhotplug05.sh
+++ b/testcases/kernel/hotplug/cpu_hotplug/functional/cpuhotplug05.sh
@@ -1,157 +1,95 @@
 #!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
 #
-# Test Case 5 - sar
-#
+# Test Case 5:
+# Check if /proc/stat can show correct cpu statistics
+# when cpu is onlined/offlined.
 
-export TCID="cpuhotplug05"
-export TST_TOTAL=1
-export LC_TIME="POSIX"
+TST_SETUP=do_setup
+TST_CLEANUP=do_clean
+TST_TESTFUNC=do_test
+TST_USAGE=cpuhotplug_usage
+TST_OPTS="c:"
+TST_PARSE_ARGS=cpuhotplug_parse_args
 
-# Includes:
-. test.sh
+. tst_test.sh
 . cpuhotplug_testsuite.sh
 . cpuhotplug_hotplug.sh
 
-cat <<EOF
-Name:   $TCID
-Date:   `date`
-Desc:   Does sar behave properly during CPU hotplug events?
+orig_online=0
 
-EOF
-
-usage()
+cpuhotplug_usage()
 {
-	cat << EOF
-	usage: $0 -c cpu -l loop -d directory
-
-	OPTIONS
-		-c  cpu which is specified for testing
-		-l  number of cycle test
-		-d  directory used to lay file
-
-EOF
-	exit 1
+	echo "usage: $0"
+	echo "OPTIONS"
+	echo "-c cpu  which cpu is specified for testing"
 }
 
-do_clean()
+cpuhotplug_parse_args()
 {
-	pid_is_valid ${SAR_PID} && kill_pid ${SAR_PID}
-	online_cpu "$CPU_TO_TEST"
+	case $1 in
+	c) cpu_num="$2";;
+	esac
 }
 
-get_field()
+do_clean()
 {
-	echo "$1" | awk "{print \$$2}"
+	[ $orig_online -eq 1 ] && online_cpu "$cpu_num"
+	[ $orig_online -eq 0 ] && offline_cpu "$cpu_num"
 }
 
-while getopts c:l:d: OPTION; do
-	case $OPTION in
-	c)
-		CPU_TO_TEST=$OPTARG;;
-	l)
-		HOTPLUG05_LOOPS=$OPTARG;;
-	d)
-		TMP=$OPTARG;;
-	?)
-		usage;;
-	esac
-done
-
-LOOP_COUNT=1
-
-tst_require_cmds sar
-
-if [ $(get_present_cpus_num) -lt 2 ]; then
-	tst_brkm TCONF "system doesn't have required CPU hotplug support"
-fi
-
-if [ -z "$CPU_TO_TEST" ]; then
-	tst_brkm TBROK "usage: ${0##*} <CPU to offline>"
-fi
+do_setup()
+{
+	[ $(get_present_cpus_num) -lt 2 ] && \
+		tst_brk TCONF "system doesn't have required cpu hotplug support"
 
-# Validate the specified CPU is available
-if ! cpu_is_valid "${CPU_TO_TEST}" ; then
-	tst_brkm TCONF "cpu${CPU_TO_TEST} doesn't support hotplug"
-fi
+	[ -z "$cpu_num" ] && \
+		tst_brk TBROK "didn't specify cpu number to test"
 
-# Check that the specified CPU is offline; if not, offline it
-if cpu_is_online "${CPU_TO_TEST}" ; then
-	if ! offline_cpu ${CPU_TO_TEST} ; then
-		tst_brkm TBROK "CPU${CPU_TO_TEST} cannot be offlined"
+	# Validate the specified cpu is available
+	if ! cpu_is_valid "$cpu_num" ; then
+		tst_brk TCONF "cpu${cpu_num} doesn't support hotplug"
 	fi
-fi
 
-TST_CLEANUP=do_clean
+	# Set orig_online to 1 if the specified cpu is online by default
+	cpu_is_online "$cpu_num" && orig_online=1
+}
 
-LOG_FILE="$TMP/log_$$"
+do_test()
+{
+	local passed=0
 
-until [ $LOOP_COUNT -gt $HOTPLUG05_LOOPS ]; do
+	# Online the specified cpu
+	online_cpu "$cpu_num" || \
+		tst_brk TBROK "cpu${cpu_num} cannot be onlined"
 
-	# Start up SAR and give it a couple cycles to run
-	sar 1 0 >/dev/null 2>&1 &
-	sleep 2
-	# "sar 1 0" is supported before 'sysstat-8.1.4(include sar)',
-	# after that use "sar 1" instead of. Use 'ps -C sar' to check.
-	if ps -C sar >/dev/null 2>&1; then
-		pkill sar
-		sar -P "$CPU_TO_TEST" 1 0 > "$LOG_FILE" &
-	else
-		sar -P "$CPU_TO_TEST" 1 > "$LOG_FILE" &
-	fi
-	sleep 2
-	SAR_PID=$!
-
-	# Since the CPU is offline, SAR should display all the 6 fields
-	# of CPU statistics as '0.00'
-	offline_status=$(tail -n 1 "$LOG_FILE")
-	if [ -z "$offline_status" ]; then
-		tst_brkm TBROK "SAR output file is empty"
+	if ! grep -q "^cpu${cpu_num}" /proc/stat; then
+		tst_res TFAIL \
+			"online cpu${cpu_num} doesn't appear in /proc/stat"
+		return 1
 	fi
 
-	cpu_field=$(get_field "$offline_status" "2")
-	if [ "${cpu_field}" = "CPU" ]; then
-		# Since sysstat-11.7.1, sar/sadf didn't display offline CPU
-		tst_resm TINFO "SAR didn't display offline CPU"
-	else
-		for i in $(seq 3 8); do
-			field=$(get_field "$offline_status" "$i")
-			if [ "$field" != "0.00" ]; then
-				tst_brkm TBROK "Field $i is '$field', '0.00' expected"
-			fi
-		done
-	fi
-
-	# Online the CPU
-	if ! online_cpu ${CPU_TO_TEST}; then
-		tst_brkm TBROK "CPU${CPU_TO_TEST} cannot be onlined"
-	fi
-
-	sleep 2
-
-	# Check that SAR registered the change in CPU online/offline states
-	online_status=$(tail -n 1 "$LOG_FILE")
-	check_passed=0
-	for i in $(seq 3 8); do
-		field_online=$(get_field "$online_status" "$i")
-
-		if [ "$field_online" != "0.00" ]; then
-			check_passed=1
-			break
-		fi
+	for field in $(seq 2 11); do
+		field_value=$(grep "^cpu${cpu_num}" /proc/stat | awk "{print \$$field}")
+		[ "$field_value" != "0" ] && passed=1
 	done
 
-	if [ $check_passed -eq 0 ]; then
-		tst_resm TFAIL "No change in the CPU statistics"
-		tst_exit
+	if [ $passed -eq 0 ]; then
+		tst_res TFAIL \
+			"all field of online cpu{cpu_num} shows zero in /proc/stat"
+		return 1
 	fi
 
-	offline_cpu ${CPU_TO_TEST}
-	kill_pid ${SAR_PID}
+	# Offline the specified cpu
+	offline_cpu "$cpu_num" || \
+		tst_brk TBROK "cpu${cpu_num} cannot be offlined"
 
-	LOOP_COUNT=$((LOOP_COUNT+1))
-
-done
+	if grep -q "^cpu${cpu_num}" /proc/stat; then
+		tst_res TFAIL "offline cpu${cpu_num} appears in /proc/stat"
+		return 1
+	fi
 
-tst_resm TPASS "SAR updated statistics after the CPU was turned on."
+	tst_res TPASS "/proc/stat shows correct cpu statistics"
+}
 
-tst_exit
+tst_run
-- 
2.21.0

[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[Martin Doucha]

On 12/2/19 11:19 AM, Xiao Yang wrote:
> +do_test()
> +{

First of all, your patch changes the test scenario. The original
scenario is this:
(setup) Deactivate selected CPU
1. Check that load statistics are zero or empty for selected CPU
2. Activate selected CPU
3. Check that load statistics are valid for selected CPU
4. Deactivate selected CPU
(cleanup) Activate selected CPU

I have a few kernel builds where step 3 correctly detects regression.

Your changed scenario looks like this:
1. Activate selected CPU
2. Check that load statistics are non-zero for selected CPU
3. Deactivate selected CPU
4. Check that load statistics are empty for selected CPU
(cleanup) Return selected CPU back to original state

Your new scenario has two problems:

- You have to run at least two test loops to verify that reactivating a
CPU doesn't break /proc/stat entries.

- You only check that load statistics are non-zero. This works with sar
because the program already normalizes load statistics for you. But it
won't detect the regression mentioned above because the bug sets one of
the entries to something absurdly high. This will confuse sar enough to
print all load values as 0 but it'll pass your checks.

In addition to checking that at least some values are non-zero, you need
to check a few more things:
- Call `getconf CLK_TCK` to find /proc/stat timer resolution
- Calculate system uptime in seconds
- Check that sum of all /proc/stat values for selected CPU is less than
or equal to (timer resolution * (uptime+1))
- Also consider doing the above check for all CPUs to increase test
coverage.

> +	for field in $(seq 2 11); do
> +		field_value=$(grep "^cpu${cpu_num}" /proc/stat | awk "{print \$$field}")
> +		[ "$field_value" != "0" ] && passed=1
>  	done
>  
> -	if [ $check_passed -eq 0 ]; then
> -		tst_resm TFAIL "No change in the CPU statistics"
> -		tst_exit
> +	if [ $passed -eq 0 ]; then
> +		tst_res TFAIL \
> +			"all field of online cpu{cpu_num} shows zero in /proc/stat"
> +		return 1
>  	fi

It'd be better to write this check (including the upper bound check
explained above) entirely in Awk.

if ! awk "[test script]" /proc/stat; then
	tst_res TFAIL ...
	return 1
fi

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[Xiao Yang]

Hi Martin,

Thanks for your comment.
Also sorry for the late reply.

On 12/4/19 11:11 PM, Martin Doucha wrote:
> On 12/2/19 11:19 AM, Xiao Yang wrote:
>> +do_test()
>> +{
> First of all, your patch changes the test scenario. The original
> scenario is this:
> (setup) Deactivate selected CPU
> 1. Check that load statistics are zero or empty for selected CPU
> 2. Activate selected CPU
> 3. Check that load statistics are valid for selected CPU
> 4. Deactivate selected CPU
> (cleanup) Activate selected CPU
>
> I have a few kernel builds where step 3 correctly detects regression.
>
> Your changed scenario looks like this:
> 1. Activate selected CPU
> 2. Check that load statistics are non-zero for selected CPU
> 3. Deactivate selected CPU
> 4. Check that load statistics are empty for selected CPU
> (cleanup) Return selected CPU back to original state
>
> Your new scenario has two problems:
>
> - You have to run at least two test loops to verify that reactivating a
> CPU doesn't break /proc/stat entries.

I think running cpuhotplug05.sh with -i 2 can verfiy this point.

Perhaps we can add -i option to runtest/cpuhotplug, or do you prefer to 
keep the original scenario?

>
> - You only check that load statistics are non-zero. This works with sar
> because the program already normalizes load statistics for you. But it
> won't detect the regression mentioned above because the bug sets one of
> the entries to something absurdly high. This will confuse sar enough to
> print all load values as 0 but it'll pass your checks.

Good point.

I will looking into the code of sar.

>
> In addition to checking that at least some values are non-zero, you need
> to check a few more things:
> - Call `getconf CLK_TCK` to find /proc/stat timer resolution
> - Calculate system uptime in seconds
> - Check that sum of all /proc/stat values for selected CPU is less than
> or equal to (timer resolution * (uptime+1))
I will try to add them as you suggested.
> - Also consider doing the above check for all CPUs to increase test
> coverage.

Is it necessary to add above check for all CPUs?

This test is designed to test specified CPU so you can test each CPU by 
using -c option.


Thanks,

Xiao Yang

>
>> +	for field in $(seq 2 11); do
>> +		field_value=$(grep "^cpu${cpu_num}" /proc/stat | awk "{print \$$field}")
>> +		[ "$field_value" != "0" ] && passed=1
>>   	done
>>   
>> -	if [ $check_passed -eq 0 ]; then
>> -		tst_resm TFAIL "No change in the CPU statistics"
>> -		tst_exit
>> +	if [ $passed -eq 0 ]; then
>> +		tst_res TFAIL \
>> +			"all field of online cpu{cpu_num} shows zero in /proc/stat"
>> +		return 1
>>   	fi
> It'd be better to write this check (including the upper bound check
> explained above) entirely in Awk.
>
> if ! awk "[test script]" /proc/stat; then
> 	tst_res TFAIL ...
> 	return 1
> fi
>

[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[Martin Doucha]

On 12/9/19 4:13 AM, Xiao Yang wrote:
>> Your new scenario has two problems:
>>
>> - You have to run at least two test loops to verify that reactivating a
>> CPU doesn't break /proc/stat entries.
> 
> I think running cpuhotplug05.sh with -i 2 can verfiy this point.
> 
> Perhaps we can add -i option to runtest/cpuhotplug, or do you prefer to
> keep the original scenario?

I'd prefer keeping the original scenario (with your version of cleanup).
Running extra iterations of the same test should not be required to get
the full intended test coverage.

>> - Also consider doing the above check for all CPUs to increase test
>> coverage.
> 
> Is it necessary to add above check for all CPUs?
> 
> This test is designed to test specified CPU so you can test each CPU by
> using -c option.

It's not necessary but it'd be a nice addition. Note that running the
test script multiple times with different -c arguments does not fully
cover this extra check. Turning a CPU core off and on again might reset
its /proc/stat line into a sane state while breaking all the others.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[Petr Vorel]

Hi,

> On 12/9/19 4:13 AM, Xiao Yang wrote:
> >> Your new scenario has two problems:

> >> - You have to run at least two test loops to verify that reactivating a
> >> CPU doesn't break /proc/stat entries.

> > I think running cpuhotplug05.sh with -i 2 can verfiy this point.

> > Perhaps we can add -i option to runtest/cpuhotplug, or do you prefer to
> > keep the original scenario?

> I'd prefer keeping the original scenario (with your version of cleanup).
> Running extra iterations of the same test should not be required to get
> the full intended test coverage.
Agree with Martin. Yang, could you, please, send new version, where you keep it?


Kind regards,
Petr

[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[Xiao Yang]

On 2019/12/10 5:06, Petr Vorel wrote:
> Agree with Martin. Yang, could you, please, send new version, where you keep it?
Hi Petr, Martin.

Sorry,  I am busy with other things recently.
Don't worry, I will send new version this week or next week. :-)

Thanks,
Xiao Yang
>
> Kind regards,
> Petr

[LTP] [PATCH] cpuhotplug05.sh: Rewrite test case

[Petr Vorel]

Hi,

> Sorry,  I am busy with other things recently.
> Don't worry, I will send new version this week or next week. :-)
Yang, sure, no rush.

Martin, thanks a lot for very good review!

Kind regards,
Petr