[PATCH] selftests/damon: introduce _common.sh to host shared function

[PATCH] selftests/damon: introduce _common.sh to host shared function

[Enze Li]

The current test scripts contain duplicated root permission checks
in multiple locations.  This patch consolidates these checks into
_common.sh to eliminate code redundancy.

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 tools/testing/selftests/damon/_common.sh           | 14 ++++++++++++++
 tools/testing/selftests/damon/lru_sort.sh          |  9 ++-------
 tools/testing/selftests/damon/reclaim.sh           |  9 ++-------
 tools/testing/selftests/damon/sysfs.sh             | 12 +-----------
 .../damon/sysfs_update_removed_scheme_dir.sh       |  9 ++-------
 5 files changed, 21 insertions(+), 32 deletions(-)
 create mode 100644 tools/testing/selftests/damon/_common.sh

diff --git a/tools/testing/selftests/damon/_common.sh b/tools/testing/selftests/damon/_common.sh
new file mode 100644
index 000000000000..3920b619c30f
--- /dev/null
+++ b/tools/testing/selftests/damon/_common.sh
@@ -0,0 +1,14 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# Kselftest frmework requirement - SKIP code is 4.
+ksft_skip=4
+
+check_dependencies()
+{
+	if [ $EUID -ne 0 ]
+	then
+		echo "Run as root"
+		exit $ksft_skip
+	fi
+}
diff --git a/tools/testing/selftests/damon/lru_sort.sh b/tools/testing/selftests/damon/lru_sort.sh
index 61b80197c896..0d128d809fd3 100755
--- a/tools/testing/selftests/damon/lru_sort.sh
+++ b/tools/testing/selftests/damon/lru_sort.sh
@@ -1,14 +1,9 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
+source _common.sh
 
-if [ $EUID -ne 0 ]
-then
-	echo "Run as root"
-	exit $ksft_skip
-fi
+check_dependencies
 
 damon_lru_sort_enabled="/sys/module/damon_lru_sort/parameters/enabled"
 if [ ! -f "$damon_lru_sort_enabled" ]
diff --git a/tools/testing/selftests/damon/reclaim.sh b/tools/testing/selftests/damon/reclaim.sh
index 78dbc2334cbe..41e450a696ae 100755
--- a/tools/testing/selftests/damon/reclaim.sh
+++ b/tools/testing/selftests/damon/reclaim.sh
@@ -1,14 +1,9 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
+source _common.sh
 
-if [ $EUID -ne 0 ]
-then
-	echo "Run as root"
-	exit $ksft_skip
-fi
+check_dependencies
 
 damon_reclaim_enabled="/sys/module/damon_reclaim/parameters/enabled"
 if [ ! -f "$damon_reclaim_enabled" ]
diff --git a/tools/testing/selftests/damon/sysfs.sh b/tools/testing/selftests/damon/sysfs.sh
index e9a976d296e2..0326b9ad55ca 100755
--- a/tools/testing/selftests/damon/sysfs.sh
+++ b/tools/testing/selftests/damon/sysfs.sh
@@ -1,8 +1,7 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-# Kselftest frmework requirement - SKIP code is 4.
-ksft_skip=4
+source _common.sh
 
 ensure_write_succ()
 {
@@ -364,14 +363,5 @@ test_damon_sysfs()
 	test_kdamonds "$damon_sysfs/kdamonds"
 }
 
-check_dependencies()
-{
-	if [ $EUID -ne 0 ]
-	then
-		echo "Run as root"
-		exit $ksft_skip
-	fi
-}
-
 check_dependencies
 test_damon_sysfs "/sys/kernel/mm/damon/admin"
diff --git a/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh b/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh
index ade35576e748..730165bd7f03 100755
--- a/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh
+++ b/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh
@@ -1,14 +1,9 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
+source _common.sh
 
-if [ $EUID -ne 0 ]
-then
-	echo "Run as root"
-	exit $ksft_skip
-fi
+check_dependencies
 
 damon_sysfs="/sys/kernel/mm/damon/admin"
 if [ ! -d "$damon_sysfs" ]

base-commit: e2291551827fe5d2d3758c435c191d32b6d1350e
-- 
2.43.0

Re: [PATCH] selftests/damon: introduce _common.sh to host shared function

[Joshua Hahn]

On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li <lienze@kylinos.cn> wrote:

Hi Enze,

Thank you for the patch! I just have a few comments about the patch.

> The current test scripts contain duplicated root permission checks
> in multiple locations.  This patch consolidates these checks into
> _common.sh to eliminate code redundancy.

Is there a reason we named the file _common.sh? IIRC there are no other files
that begin with an underscore, so it might be confusing for users. Maybe
remaining it to damon_common.sh might fit better with the convention used
by other selftests. 

[...snip...]

> diff --git a/tools/testing/selftests/damon/_common.sh b/tools/testing/selftests/damon/_common.sh
> new file mode 100644
> index 000000000000..3920b619c30f
> --- /dev/null
> +++ b/tools/testing/selftests/damon/_common.sh
> @@ -0,0 +1,14 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Kselftest frmework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> +check_dependencies()
> +{
> +	if [ $EUID -ne 0 ]
> +	then
> +		echo "Run as root"
> +		exit $ksft_skip
> +	fi
> +}
> diff --git a/tools/testing/selftests/damon/lru_sort.sh b/tools/testing/selftests/damon/lru_sort.sh
> index 61b80197c896..0d128d809fd3 100755
> --- a/tools/testing/selftests/damon/lru_sort.sh
> +++ b/tools/testing/selftests/damon/lru_sort.sh
> @@ -1,14 +1,9 @@
>  #!/bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>  
> -# Kselftest framework requirement - SKIP code is 4.
> -ksft_skip=4

Hm, I think factoring out check_dependencies() is a good idea, but maybe we
should keep ksft_skip in here since other checks in the script use the value?
My 2c is that it might make it unnecessarily opaque for others.
Same comment applies for the other files as well. 

But I will let SJ comment on this more ;)

Thank you for your patch, I hope you have a great day!
Joshua

Sent using hkml (https://github.com/sjp38/hackermail)

Re: [PATCH] selftests/damon: introduce _common.sh to host shared function

[SeongJae Park]

Hi Joshua,

On Thu, 17 Jul 2025 06:54:32 -0700 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:

> On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li <lienze@kylinos.cn> wrote:
> 
> Hi Enze,
> 
> Thank you for the patch! I just have a few comments about the patch.
> 
> > The current test scripts contain duplicated root permission checks
> > in multiple locations.  This patch consolidates these checks into
> > _common.sh to eliminate code redundancy.
> 
> Is there a reason we named the file _common.sh? IIRC there are no other files
> that begin with an underscore, so it might be confusing for users. Maybe
> remaining it to damon_common.sh might fit better with the convention used
> by other selftests. 

This is my personal pattern that I sometimes use, to distinguish files that
aimed to be only indirectly be used.  We already have a file of the pattern,
namely _damon_sysfs.py.

I don't think this pattern is particularly good, but not making something
worse, so I'm ok with current file name.

> 
> [...snip...]
> 
> > diff --git a/tools/testing/selftests/damon/_common.sh b/tools/testing/selftests/damon/_common.sh
> > new file mode 100644
> > index 000000000000..3920b619c30f
> > --- /dev/null
> > +++ b/tools/testing/selftests/damon/_common.sh
> > @@ -0,0 +1,14 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +# Kselftest frmework requirement - SKIP code is 4.
> > +ksft_skip=4
> > +
> > +check_dependencies()
> > +{
> > +	if [ $EUID -ne 0 ]
> > +	then
> > +		echo "Run as root"
> > +		exit $ksft_skip
> > +	fi
> > +}
> > diff --git a/tools/testing/selftests/damon/lru_sort.sh b/tools/testing/selftests/damon/lru_sort.sh
> > index 61b80197c896..0d128d809fd3 100755
> > --- a/tools/testing/selftests/damon/lru_sort.sh
> > +++ b/tools/testing/selftests/damon/lru_sort.sh
> > @@ -1,14 +1,9 @@
> >  #!/bin/bash
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > -# Kselftest framework requirement - SKIP code is 4.
> > -ksft_skip=4
> 
> Hm, I think factoring out check_dependencies() is a good idea, but maybe we
> should keep ksft_skip in here since other checks in the script use the value?
> My 2c is that it might make it unnecessarily opaque for others.
> Same comment applies for the other files as well. 
> 
> But I will let SJ comment on this more ;)

I agree Joshua's point.  I'd prefer keeping ksft_skip definition here.

> 
> Thank you for your patch, I hope you have a great day!

Thank you for your valuable comments, Joshua :)


Thanks,
SJ

[...]

Re: [PATCH] selftests/damon: introduce _common.sh to host shared function

[SeongJae Park]

Hi Enze,

On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li <lienze@kylinos.cn> wrote:

> The current test scripts contain duplicated root permission checks
> in multiple locations.  This patch consolidates these checks into
> _common.sh to eliminate code redundancy.

Thank you for this patch!

> 
> Signed-off-by: Enze Li <lienze@kylinos.cn>
> ---
>  tools/testing/selftests/damon/_common.sh           | 14 ++++++++++++++
>  tools/testing/selftests/damon/lru_sort.sh          |  9 ++-------
>  tools/testing/selftests/damon/reclaim.sh           |  9 ++-------
>  tools/testing/selftests/damon/sysfs.sh             | 12 +-----------
>  .../damon/sysfs_update_removed_scheme_dir.sh       |  9 ++-------
>  5 files changed, 21 insertions(+), 32 deletions(-)
>  create mode 100644 tools/testing/selftests/damon/_common.sh
> 
> diff --git a/tools/testing/selftests/damon/_common.sh b/tools/testing/selftests/damon/_common.sh
> new file mode 100644
> index 000000000000..3920b619c30f
> --- /dev/null
> +++ b/tools/testing/selftests/damon/_common.sh
> @@ -0,0 +1,14 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Kselftest frmework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> +check_dependencies()
> +{
> +	if [ $EUID -ne 0 ]
> +	then
> +		echo "Run as root"
> +		exit $ksft_skip
> +	fi
> +}
> diff --git a/tools/testing/selftests/damon/lru_sort.sh b/tools/testing/selftests/damon/lru_sort.sh
> index 61b80197c896..0d128d809fd3 100755
> --- a/tools/testing/selftests/damon/lru_sort.sh
> +++ b/tools/testing/selftests/damon/lru_sort.sh
> @@ -1,14 +1,9 @@
>  #!/bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>  
> -# Kselftest framework requirement - SKIP code is 4.
> -ksft_skip=4

As Joshua also pointed out, let's keep ksft_skip definition in each file,
unless there is no more use of the variable.

> +source _common.sh
>  
> -if [ $EUID -ne 0 ]
> -then
> -	echo "Run as root"
> -	exit $ksft_skip
> -fi
> +check_dependencies
>  
>  damon_lru_sort_enabled="/sys/module/damon_lru_sort/parameters/enabled"
>  if [ ! -f "$damon_lru_sort_enabled" ]
[...]

Other than ksft_skip, looks good to me.  Looking forward to your next version
:)


Thanks,
SJ

Re: [PATCH] selftests/damon: introduce _common.sh to host shared function

[Enze Li]

Hi SJ, Joshua,

On Thu, Jul 17 2025 at 09:14:54 AM -0700, SeongJae Park wrote:

> Hi Joshua,
>
> On Thu, 17 Jul 2025 06:54:32 -0700 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
>
>> On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li <lienze@kylinos.cn> wrote:
>> 
>> Hi Enze,
>> 
>> Thank you for the patch! I just have a few comments about the patch.
>> 
>> > The current test scripts contain duplicated root permission checks
>> > in multiple locations.  This patch consolidates these checks into
>> > _common.sh to eliminate code redundancy.
>> 
>> Is there a reason we named the file _common.sh? IIRC there are no other files
>> that begin with an underscore, so it might be confusing for users. Maybe
>> remaining it to damon_common.sh might fit better with the convention used
>> by other selftests. 
>
> This is my personal pattern that I sometimes use, to distinguish files that
> aimed to be only indirectly be used.  We already have a file of the pattern,
> namely _damon_sysfs.py.
>
> I don't think this pattern is particularly good, but not making something
> worse, so I'm ok with current file name.

Yes, I've noted the naming convention from _damon_sysfs.py and have
maintained consistency with the existing pattern in this patch. :)

>
>> 
>> [...snip...]
>> 
>> > diff --git a/tools/testing/selftests/damon/_common.sh b/tools/testing/selftests/damon/_common.sh
>> > new file mode 100644
>> > index 000000000000..3920b619c30f
>> > --- /dev/null
>> > +++ b/tools/testing/selftests/damon/_common.sh
>> > @@ -0,0 +1,14 @@
>> > +#!/bin/bash
>> > +# SPDX-License-Identifier: GPL-2.0
>> > +
>> > +# Kselftest frmework requirement - SKIP code is 4.
>> > +ksft_skip=4
>> > +
>> > +check_dependencies()
>> > +{
>> > +	if [ $EUID -ne 0 ]
>> > +	then
>> > +		echo "Run as root"
>> > +		exit $ksft_skip
>> > +	fi
>> > +}
>> > diff --git a/tools/testing/selftests/damon/lru_sort.sh b/tools/testing/selftests/damon/lru_sort.sh
>> > index 61b80197c896..0d128d809fd3 100755
>> > --- a/tools/testing/selftests/damon/lru_sort.sh
>> > +++ b/tools/testing/selftests/damon/lru_sort.sh
>> > @@ -1,14 +1,9 @@
>> >  #!/bin/bash
>> >  # SPDX-License-Identifier: GPL-2.0
>> >  
>> > -# Kselftest framework requirement - SKIP code is 4.
>> > -ksft_skip=4
>> 
>> Hm, I think factoring out check_dependencies() is a good idea, but maybe we
>> should keep ksft_skip in here since other checks in the script use the value?
>> My 2c is that it might make it unnecessarily opaque for others.
>> Same comment applies for the other files as well. 
>> 
>> But I will let SJ comment on this more ;)
>
> I agree Joshua's point.  I'd prefer keeping ksft_skip definition here.
>

Thank you for the review.  I'll send v2 addressing your comments to the
list soon.

BR,
Enze

>> 
>> Thank you for your patch, I hope you have a great day!
>
> Thank you for your valuable comments, Joshua :)
>
>
> Thanks,
> SJ
>
> [...]