diff mbox series

[1/3] selftests/zram: Remove obsolete max_comp_streams interface

Message ID 1639562171-4434-1-git-send-email-xuyang2018.jy@fujitsu.com
State New
Headers show
Series [1/3] selftests/zram: Remove obsolete max_comp_streams interface | expand

Commit Message

Yang Xu Dec. 15, 2021, 9:56 a.m. UTC
Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams internals"), zram has
switched to per-cpu streams. Even kernel still keep this interface for some reasons, but
writing to max_comp_stream doesn't take any effect. So remove it.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 tools/testing/selftests/zram/zram01.sh   |  4 ----
 tools/testing/selftests/zram/zram02.sh   |  4 ----
 tools/testing/selftests/zram/zram_lib.sh | 22 ----------------------
 3 files changed, 30 deletions(-)

Comments

Yang Xu Jan. 13, 2022, 6:26 a.m. UTC | #1
Hi Ping

Best Regards
Yang Xu
> Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams internals"), zram has
> switched to per-cpu streams. Even kernel still keep this interface for some reasons, but
> writing to max_comp_stream doesn't take any effect. So remove it.
> 
> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> ---
>   tools/testing/selftests/zram/zram01.sh   |  4 ----
>   tools/testing/selftests/zram/zram02.sh   |  4 ----
>   tools/testing/selftests/zram/zram_lib.sh | 22 ----------------------
>   3 files changed, 30 deletions(-)
> 
> diff --git a/tools/testing/selftests/zram/zram01.sh b/tools/testing/selftests/zram/zram01.sh
> index 114863d9fb87..28583e4ae546 100755
> --- a/tools/testing/selftests/zram/zram01.sh
> +++ b/tools/testing/selftests/zram/zram01.sh
> @@ -15,9 +15,6 @@ ERR_CODE=0
> 
>   # Test will create the following number of zram devices:
>   dev_num=1
> -# This is a list of parameters for zram devices.
> -# Number of items must be equal to 'dev_num' parameter.
> -zram_max_streams="2"
> 
>   # The zram sysfs node 'disksize' value can be either in bytes,
>   # or you can use mem suffixes. But in some old kernels, mem
> @@ -72,7 +69,6 @@ zram_fill_fs()
> 
>   check_prereqs
>   zram_load
> -zram_max_streams
>   zram_compress_alg
>   zram_set_disksizes
>   zram_set_memlimit
> diff --git a/tools/testing/selftests/zram/zram02.sh b/tools/testing/selftests/zram/zram02.sh
> index e83b404807c0..d664974a1317 100755
> --- a/tools/testing/selftests/zram/zram02.sh
> +++ b/tools/testing/selftests/zram/zram02.sh
> @@ -14,9 +14,6 @@ ERR_CODE=0
> 
>   # Test will create the following number of zram devices:
>   dev_num=1
> -# This is a list of parameters for zram devices.
> -# Number of items must be equal to 'dev_num' parameter.
> -zram_max_streams="2"
> 
>   # The zram sysfs node 'disksize' value can be either in bytes,
>   # or you can use mem suffixes. But in some old kernels, mem
> @@ -30,7 +27,6 @@ zram_mem_limits="1M"
> 
>   check_prereqs
>   zram_load
> -zram_max_streams
>   zram_set_disksizes
>   zram_set_memlimit
>   zram_makeswap
> diff --git a/tools/testing/selftests/zram/zram_lib.sh b/tools/testing/selftests/zram/zram_lib.sh
> index 6f872f266fd1..0c49f9d1d563 100755
> --- a/tools/testing/selftests/zram/zram_lib.sh
> +++ b/tools/testing/selftests/zram/zram_lib.sh
> @@ -82,28 +82,6 @@ zram_load()
>   	fi
>   }
> 
> -zram_max_streams()
> -{
> -	echo "set max_comp_streams to zram device(s)"
> -
> -	local i=0
> -	for max_s in $zram_max_streams; do
> -		local sys_path="/sys/block/zram${i}/max_comp_streams"
> -		echo $max_s>  $sys_path || \
> -			echo "FAIL failed to set '$max_s' to $sys_path"
> -		sleep 1
> -		local max_streams=$(cat $sys_path)
> -
> -		[ "$max_s" -ne "$max_streams" ]&&  \
> -			echo "FAIL can't set max_streams '$max_s', get $max_stream"
> -
> -		i=$(($i + 1))
> -		echo "$sys_path = '$max_streams' ($i/$dev_num)"
> -	done
> -
> -	echo "zram max streams: OK"
> -}
> -
>   zram_compress_alg()
>   {
>   	echo "test that we can set compression algorithm"
Shuah Khan Jan. 25, 2022, 8:33 p.m. UTC | #2
On 12/15/21 2:56 AM, Yang Xu wrote:
> Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams internals"), zram has
> switched to per-cpu streams. Even kernel still keep this interface for some reasons, but
> writing to max_comp_stream doesn't take any effect. So remove it.

I get that max_comp_stream doesn't do anything since this referenced
commit. Don't we need this test on older kernels since older kernels
still support max_comp_stream?

thanks,
-- Shuah
Shuah Khan Jan. 25, 2022, 8:37 p.m. UTC | #3
On 12/15/21 2:56 AM, Yang Xu wrote:
> zram01 uses `free -m` to measure zram memory usage. The results are nonsense
> because they are polluted by all running processes on the system.
> 

Are the results inaccurate or does /sys/block/zram<id>/mm_stat is a quick
way to get the information?

In any case, this patch and all 3 patches in this series have:

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)

Please run checkpatch.pl and clean these up.

thanks,
-- Shuah
Shuah Khan Jan. 25, 2022, 8:40 p.m. UTC | #4
On 12/15/21 2:56 AM, Yang Xu wrote:
> On some linux distributions, if it used /dev/zram0 as default by zram-generate
> service, then this case will fail or report EBUSY.
> 
> To fix this, use hot_add/hot_remove interface. Also, move module check code into
> zram_load from zram.sh. We can use /sys/class/zram-control to detect the
> module whether existed.
> 
> After this patch, we still run case pass even /dev/zram0 is being used.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>   tools/testing/selftests/zram/zram.sh     | 16 +----
>   tools/testing/selftests/zram/zram01.sh   |  2 +-
>   tools/testing/selftests/zram/zram_lib.sh | 75 ++++++++++++------------
>   3 files changed, 41 insertions(+), 52 deletions(-)
> 

This patch looks good to me. A few checkpatch warns to fix and also
it probably depends on the other two patches in this series.

Please fix and send v2 for all 3 patches.

thanks,
-- Shuah
Shuah Khan Jan. 25, 2022, 8:52 p.m. UTC | #5
On 1/12/22 11:26 PM, xuyang2018.jy@fujitsu.com wrote:
> Hi Ping
> 
> Best Regards
> Yang Xu
>> Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams internals"), zram has
>> switched to per-cpu streams. Even kernel still keep this interface for some reasons, but
>> writing to max_comp_stream doesn't take any effect. So remove it.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>

Reviewed and changes requested.

thanks,
-- Shuah
Yang Xu Jan. 26, 2022, 5:19 a.m. UTC | #6
on 2022/1/26 4:33, Shuah Khan wrote :
> On 12/15/21 2:56 AM, Yang Xu wrote:
>> Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams
>> internals"), zram has
>> switched to per-cpu streams. Even kernel still keep this interface for
>> some reasons, but
>> writing to max_comp_stream doesn't take any effect. So remove it.
>
> I get that max_comp_stream doesn't do anything since this referenced
> commit. Don't we need this test on older kernels since older kernels
> still support max_comp_stream?

I read the following info from kernel selftest documentation
https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html

"The kernel contains a set of “self tests” under the 
tools/testing/selftests/ directory. These are intended to be small tests 
to exercise individual code paths in the kernel. Tests are intended to 
be run after building, installing and booting a kernel."

So, we can build older kernel(use older kernel source) if we want to 
test older kernel.

IMO, kerenl selftest is different from other testsuit(ie ltp, this 
shuould think about api changes because ltp may test on different kernel).
Also cc ltp co-maintainer Petr

Or, did I miss something?

Best Regards
Yang Xu


>
> thanks,
> -- Shuah
Yang Xu Jan. 26, 2022, 6:08 a.m. UTC | #7
on 2022/1/26 4:37, Shuah Khan wrote:
> On 12/15/21 2:56 AM, Yang Xu wrote:
>> zram01 uses `free -m` to measure zram memory usage. The results are
>> nonsense
>> because they are polluted by all running processes on the system.
>>
>
> Are the results inaccurate or does /sys/block/zram<id>/mm_stat is a quick
> way to get the information?
The "free -m" result is inaccurate because it caculted global systemd 
free memory instead of process that used zram device.

We should use mm_stat as Documentation/admin-guide/blockdev/zram.rst wrote:

File /sys/block/zram<id>/mm_stat

The mm_stat file represents the device's mm statistics. It consists of a 
single
line of text and contains the following stats separated by whitespace:

  ================ 
=============================================================
  orig_data_size   uncompressed size of data stored in this disk.
                   Unit: bytes
  compr_data_size  compressed size of data stored in this disk
  mem_used_total   the amount of memory allocated for this disk. This
                   includes allocator fragmentation and metadata
>
> In any case, this patch and all 3 patches in this series have:
>
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
>
> Please run checkpatch.pl and clean these up.
Ok, Will do it in v2.

Best Regards
Yang Xu
>
> thanks,
> -- Shuah
Petr Vorel Jan. 26, 2022, 7:13 a.m. UTC | #8
Hi all,

> on 2022/1/26 4:33, Shuah Khan wrote :
> > On 12/15/21 2:56 AM, Yang Xu wrote:
> >> Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams
> >> internals"), zram has
> >> switched to per-cpu streams. Even kernel still keep this interface for
> >> some reasons, but
> >> writing to max_comp_stream doesn't take any effect. So remove it.

> > I get that max_comp_stream doesn't do anything since this referenced
> > commit. Don't we need this test on older kernels since older kernels
> > still support max_comp_stream?

> I read the following info from kernel selftest documentation
> https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html

> "The kernel contains a set of “self tests” under the 
> tools/testing/selftests/ directory. These are intended to be small tests 
> to exercise individual code paths in the kernel. Tests are intended to 
> be run after building, installing and booting a kernel."

> So, we can build older kernel(use older kernel source) if we want to 
> test older kernel.

> IMO, kernel selftest is different from other testsuit(ie ltp, this 
> shuould think about api changes because ltp may test on different kernel).
Yes, that's how I understand the difference with approach of in kselftest - the
kernel tree testsuite and LTP - the out-of-tree testsuite.

Kind regards,
Petr

> Also cc ltp co-maintainer Petr

> Or, did I miss something?

> Best Regards
> Yang Xu



> > thanks,
> > -- Shuah
Shuah Khan Jan. 26, 2022, 5:35 p.m. UTC | #9
On 1/26/22 12:13 AM, Petr Vorel wrote:
> Hi all,
> 
>> on 2022/1/26 4:33, Shuah Khan wrote :
>>> On 12/15/21 2:56 AM, Yang Xu wrote:
>>>> Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams
>>>> internals"), zram has
>>>> switched to per-cpu streams. Even kernel still keep this interface for
>>>> some reasons, but
>>>> writing to max_comp_stream doesn't take any effect. So remove it.
> 
>>> I get that max_comp_stream doesn't do anything since this referenced
>>> commit. Don't we need this test on older kernels since older kernels
>>> still support max_comp_stream?
> 
>> I read the following info from kernel selftest documentation
>> https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html
> 
>> "The kernel contains a set of “self tests” under the
>> tools/testing/selftests/ directory. These are intended to be small tests
>> to exercise individual code paths in the kernel. Tests are intended to
>> be run after building, installing and booting a kernel."
> 
>> So, we can build older kernel(use older kernel source) if we want to
>> test older kernel.
> 
>> IMO, kernel selftest is different from other testsuit(ie ltp, this
>> shuould think about api changes because ltp may test on different kernel).
> Yes, that's how I understand the difference with approach of in kselftest - the
> kernel tree testsuite and LTP - the out-of-tree testsuite.
> 

Removing max_comp_stream test appears to be motivated by the fact it isn't
needed on newer kernels.

Kselftest from mainline can be run on older stable kernels. This is a use-case
for a lot test rings. The idea is that when a new test gets added for older
code to regression test a bug, we should be able to run that test on an older
kernel. This is the reason why we don't remove code that can still test an older
kernel and make sure it skips gracefully.

Hence, I won't be taking this patch.

thanks,
-- Shuah
Petr Vorel Jan. 26, 2022, 6:24 p.m. UTC | #10
> On 1/26/22 12:13 AM, Petr Vorel wrote:
> > Hi all,

> > > on 2022/1/26 4:33, Shuah Khan wrote :
> > > > On 12/15/21 2:56 AM, Yang Xu wrote:
> > > > > Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams
> > > > > internals"), zram has
> > > > > switched to per-cpu streams. Even kernel still keep this interface for
> > > > > some reasons, but
> > > > > writing to max_comp_stream doesn't take any effect. So remove it.

> > > > I get that max_comp_stream doesn't do anything since this referenced
> > > > commit. Don't we need this test on older kernels since older kernels
> > > > still support max_comp_stream?

> > > I read the following info from kernel selftest documentation
> > > https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html

> > > "The kernel contains a set of “self tests” under the
> > > tools/testing/selftests/ directory. These are intended to be small tests
> > > to exercise individual code paths in the kernel. Tests are intended to
> > > be run after building, installing and booting a kernel."

> > > So, we can build older kernel(use older kernel source) if we want to
> > > test older kernel.

> > > IMO, kernel selftest is different from other testsuit(ie ltp, this
> > > shuould think about api changes because ltp may test on different kernel).
> > Yes, that's how I understand the difference with approach of in kselftest - the
> > kernel tree testsuite and LTP - the out-of-tree testsuite.


> Removing max_comp_stream test appears to be motivated by the fact it isn't
> needed on newer kernels.

> Kselftest from mainline can be run on older stable kernels. This is a use-case
> for a lot test rings. The idea is that when a new test gets added for older
> code to regression test a bug, we should be able to run that test on an older
> kernel. This is the reason why we don't remove code that can still test an older
> kernel and make sure it skips gracefully.

Thanks for clarifying this approach. It might be worth of documenting it in
dev-tools/kselftest.rst.

Kind regards,
Petr

> Hence, I won't be taking this patch.

> thanks,
> -- Shuah
Shuah Khan Jan. 26, 2022, 6:37 p.m. UTC | #11
On 1/26/22 11:24 AM, Petr Vorel wrote:
>> On 1/26/22 12:13 AM, Petr Vorel wrote:
>>> Hi all,
> 
>>>> on 2022/1/26 4:33, Shuah Khan wrote :
>>>>> On 12/15/21 2:56 AM, Yang Xu wrote:
>>>>>> Since kernel commit 43209ea2d17a ("zram: remove max_comp_streams
>>>>>> internals"), zram has
>>>>>> switched to per-cpu streams. Even kernel still keep this interface for
>>>>>> some reasons, but
>>>>>> writing to max_comp_stream doesn't take any effect. So remove it.
> 
>>>>> I get that max_comp_stream doesn't do anything since this referenced
>>>>> commit. Don't we need this test on older kernels since older kernels
>>>>> still support max_comp_stream?
> 
>>>> I read the following info from kernel selftest documentation
>>>> https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html
> 
>>>> "The kernel contains a set of “self tests” under the
>>>> tools/testing/selftests/ directory. These are intended to be small tests
>>>> to exercise individual code paths in the kernel. Tests are intended to
>>>> be run after building, installing and booting a kernel."
> 
>>>> So, we can build older kernel(use older kernel source) if we want to
>>>> test older kernel.
> 
>>>> IMO, kernel selftest is different from other testsuit(ie ltp, this
>>>> shuould think about api changes because ltp may test on different kernel).
>>> Yes, that's how I understand the difference with approach of in kselftest - the
>>> kernel tree testsuite and LTP - the out-of-tree testsuite.
> 
> 
>> Removing max_comp_stream test appears to be motivated by the fact it isn't
>> needed on newer kernels.
> 
>> Kselftest from mainline can be run on older stable kernels. This is a use-case
>> for a lot test rings. The idea is that when a new test gets added for older
>> code to regression test a bug, we should be able to run that test on an older
>> kernel. This is the reason why we don't remove code that can still test an older
>> kernel and make sure it skips gracefully.
> 
> Thanks for clarifying this approach. It might be worth of documenting it in
> dev-tools/kselftest.rst.
> 

I will send out a patch clarifying this.

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/tools/testing/selftests/zram/zram01.sh b/tools/testing/selftests/zram/zram01.sh
index 114863d9fb87..28583e4ae546 100755
--- a/tools/testing/selftests/zram/zram01.sh
+++ b/tools/testing/selftests/zram/zram01.sh
@@ -15,9 +15,6 @@  ERR_CODE=0
 
 # Test will create the following number of zram devices:
 dev_num=1
-# This is a list of parameters for zram devices.
-# Number of items must be equal to 'dev_num' parameter.
-zram_max_streams="2"
 
 # The zram sysfs node 'disksize' value can be either in bytes,
 # or you can use mem suffixes. But in some old kernels, mem
@@ -72,7 +69,6 @@  zram_fill_fs()
 
 check_prereqs
 zram_load
-zram_max_streams
 zram_compress_alg
 zram_set_disksizes
 zram_set_memlimit
diff --git a/tools/testing/selftests/zram/zram02.sh b/tools/testing/selftests/zram/zram02.sh
index e83b404807c0..d664974a1317 100755
--- a/tools/testing/selftests/zram/zram02.sh
+++ b/tools/testing/selftests/zram/zram02.sh
@@ -14,9 +14,6 @@  ERR_CODE=0
 
 # Test will create the following number of zram devices:
 dev_num=1
-# This is a list of parameters for zram devices.
-# Number of items must be equal to 'dev_num' parameter.
-zram_max_streams="2"
 
 # The zram sysfs node 'disksize' value can be either in bytes,
 # or you can use mem suffixes. But in some old kernels, mem
@@ -30,7 +27,6 @@  zram_mem_limits="1M"
 
 check_prereqs
 zram_load
-zram_max_streams
 zram_set_disksizes
 zram_set_memlimit
 zram_makeswap
diff --git a/tools/testing/selftests/zram/zram_lib.sh b/tools/testing/selftests/zram/zram_lib.sh
index 6f872f266fd1..0c49f9d1d563 100755
--- a/tools/testing/selftests/zram/zram_lib.sh
+++ b/tools/testing/selftests/zram/zram_lib.sh
@@ -82,28 +82,6 @@  zram_load()
 	fi
 }
 
-zram_max_streams()
-{
-	echo "set max_comp_streams to zram device(s)"
-
-	local i=0
-	for max_s in $zram_max_streams; do
-		local sys_path="/sys/block/zram${i}/max_comp_streams"
-		echo $max_s > $sys_path || \
-			echo "FAIL failed to set '$max_s' to $sys_path"
-		sleep 1
-		local max_streams=$(cat $sys_path)
-
-		[ "$max_s" -ne "$max_streams" ] && \
-			echo "FAIL can't set max_streams '$max_s', get $max_stream"
-
-		i=$(($i + 1))
-		echo "$sys_path = '$max_streams' ($i/$dev_num)"
-	done
-
-	echo "zram max streams: OK"
-}
-
 zram_compress_alg()
 {
 	echo "test that we can set compression algorithm"