[1/1] Fix compression ratio calculation in zram01
diff mbox series

Message ID 20191104125228.17173-2-mdoucha@suse.cz
State Superseded
Delegated to: Alexey Kodanev
Headers show
Series
  • Fix zram01.sh
Related show

Commit Message

Martin Doucha Nov. 4, 2019, 12:52 p.m. UTC
zram01 uses `free -m` to measure zram memory usage. The results are nonsense
because they are polluted by all running processes on the system.

Use /sys/block/zram<id>/mm_stat to measure memory usage instead. The file is
available since kernel 4.1.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 .../kernel/device-drivers/zram/zram01.sh      | 36 +++++++++----------
 1 file changed, 16 insertions(+), 20 deletions(-)

Comments

Petr Vorel Nov. 4, 2019, 3:16 p.m. UTC | #1
Hi Martin,

> zram01 uses `free -m` to measure zram memory usage. The results are nonsense
> because they are polluted by all running processes on the system.

> Use /sys/block/zram<id>/mm_stat to measure memory usage instead. The file is
> available since kernel 4.1.
+1 even older kernels won't be covered.
Thanks!

...
> --- a/testcases/kernel/device-drivers/zram/zram01.sh
>  zram_fill_fs()
>  {
> -	tst_test_cmds awk bc dd free
> -	local mem_free0=$(free -m | awk 'NR==2 {print $4}')
> +	tst_test_cmds awk bc dd

>  	for i in $(seq 0 $(($dev_num - 1))); do
>  		tst_resm TINFO "fill zram$i..."
> @@ -75,29 +74,26 @@ zram_fill_fs()
>  			tst_brkm TBROK "cannot fill zram"
>  		fi
>  		tst_resm TINFO "zram$i can be filled with '$b' KB"
> -	done
> -
> -	local mem_free1=$(free -m | awk 'NR==2 {print $4}')
> -	local used_mem=$(($mem_free0 - $mem_free1))
> -
> -	local total_size=0
> -	for sm in $zram_sizes; do
> -		local s=$(echo $sm | sed 's/M//')
> -		total_size=$(($total_size + $s))
> -	done

> -	[ $used_mem -eq 0 ] && tst_brkm TBROK "no memory used by zram"
> +		if [ ! -f "/sys/block/zram$i/mm_stat" ]; then
> +			if [ $i -eq 0 ]; then
> +				tst_resm TCONF "zram compression ratio test requires zram mm_stat sysfs file"
I wonder if /sys/block/zram0/mm_stat is missing whether it can be on any other
zram<id>. Is it it's presence file system specific? Shouldn't we just quit whole
test wit tst_brk TCONF.

Kind regards,
Petr
Martin Doucha Nov. 4, 2019, 3:51 p.m. UTC | #2
On 11/4/19 4:16 PM, Petr Vorel wrote:
> I wonder if /sys/block/zram0/mm_stat is missing whether it can be on any other
> zram<id>. Is it it's presence file system specific? Shouldn't we just quit whole
> test wit tst_brk TCONF.

If /sys/block/zram0/mm_stat is missing then all /sys/block/zram*/mm_stat
files should be missing. But I don't want to terminate the test there
because the remaining 3 write tests could still find a regression. So
print a TCONF message on the first pass and silently skip the remaining
compression ratio checks.

I was also thinking about checking whether the write test filled the
test file at least up to 50% of memory limit if mm_stat doesn't exist.
But it'd mostly add unnecessary complexity.
Petr Vorel Nov. 5, 2019, 8:23 a.m. UTC | #3
Hi Martin,

> On 11/4/19 4:16 PM, Petr Vorel wrote:
> > I wonder if /sys/block/zram0/mm_stat is missing whether it can be on any other
> > zram<id>. Is it it's presence file system specific? Shouldn't we just quit whole
> > test wit tst_brk TCONF.

> If /sys/block/zram0/mm_stat is missing then all /sys/block/zram*/mm_stat
> files should be missing. But I don't want to terminate the test there
> because the remaining 3 write tests could still find a regression. So
> print a TCONF message on the first pass and silently skip the remaining
> compression ratio checks.
Do you mean that dd filling zram could find a regression?

I'm asking because it's a bit strange to have test,
which doesn't lead to any result (TPASS/TFAIL/TBROK/TCONF), which will be

If this part is also a test, maybe following TINFO should be changed to TPASS.
+ Also new shell API allows to use loop in API (code simplify), but that
requires for each run to produce a result.


> I was also thinking about checking whether the write test filled the
> test file at least up to 50% of memory limit if mm_stat doesn't exist.
> But it'd mostly add unnecessary complexity.
Agree.

Kind regards,
Petr
Martin Doucha Nov. 5, 2019, 8:46 a.m. UTC | #4
On 11/5/19 9:23 AM, Petr Vorel wrote:
> Do you mean that dd filling zram could find a regression?
> 
> I'm asking because it's a bit strange to have test,
> which doesn't lead to any result (TPASS/TFAIL/TBROK/TCONF), which will be
> 
> If this part is also a test, maybe following TINFO should be changed to TPASS.
> + Also new shell API allows to use loop in API (code simplify), but that
> requires for each run to produce a result.

Note the TBROK if `dd` fails to write anything at all. But I'll change
the TINFO to TPASS and resubmit.

Patch
diff mbox series

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 9508211ab..2a8189de2 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -58,8 +58,7 @@  TST_CLEANUP="zram_cleanup"
 
 zram_fill_fs()
 {
-	tst_test_cmds awk bc dd free
-	local mem_free0=$(free -m | awk 'NR==2 {print $4}')
+	tst_test_cmds awk bc dd
 
 	for i in $(seq 0 $(($dev_num - 1))); do
 		tst_resm TINFO "fill zram$i..."
@@ -75,29 +74,26 @@  zram_fill_fs()
 			tst_brkm TBROK "cannot fill zram"
 		fi
 		tst_resm TINFO "zram$i can be filled with '$b' KB"
-	done
-
-	local mem_free1=$(free -m | awk 'NR==2 {print $4}')
-	local used_mem=$(($mem_free0 - $mem_free1))
-
-	local total_size=0
-	for sm in $zram_sizes; do
-		local s=$(echo $sm | sed 's/M//')
-		total_size=$(($total_size + $s))
-	done
 
-	[ $used_mem -eq 0 ] && tst_brkm TBROK "no memory used by zram"
+		if [ ! -f "/sys/block/zram$i/mm_stat" ]; then
+			if [ $i -eq 0 ]; then
+				tst_resm TCONF "zram compression ratio test requires zram mm_stat sysfs file"
+			fi
 
-	tst_resm TINFO "zram used ${used_mem}M, zram disk sizes ${total_size}M"
+			continue
+		fi
 
-	local v=$((100 * $total_size / $used_mem))
+		local compr_size=`cat "/sys/block/zram$i/mm_stat" | awk '{print $2}'`
+		local v=$((100 * 1024 * $b / $compr_size))
+		local r=`echo "scale=2; $v / 100 " | bc`
 
-	if [ "$v" -lt 100 ]; then
-		tst_resm TFAIL "compression ratio: 0.$v:1"
-		return
-	fi
+		if [ "$v" -lt 100 ]; then
+			tst_resm TFAIL "compression ratio: $r:1"
+			break
+		fi
 
-	tst_resm TPASS "compression ratio: $(echo "scale=2; $v / 100 " | bc):1"
+		tst_resm TPASS "compression ratio: $r:1"
+	done
 }
 
 zram_load