[PATCH 0/2] fix damos_commit_ops_filters iteration problem with

[PATCH 0/2] fix damos_commit_ops_filters iteration problem with

[Sang-Heon Jeon]

While studying, I found a section in damos_commit_ops_filters() that
looks like a bug. I first verified this issue through testing, and then
modified the code to fix problem. After applying these patch set, the
test which was added this time no longer fail.

---

Sang-Heon Jeon (2):
  selftests/damon: test no-op commit broke DAMON status
  mm/damon/core : fix commit_ops_filters by using correct nth function

 mm/damon/core.c                               | 14 +++-
 tools/testing/selftests/damon/Makefile        |  1 +
 .../damon/sysfs_no_op_commit_break.py         | 78 +++++++++++++++++++
 3 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/damon/sysfs_no_op_commit_break.py

-- 
2.43.0

[PATCH 1/2] selftests/damon: test no-op commit broke DAMON status

[Sang-Heon Jeon]

Add test to verify that DAMON status is not changed after a no-op
commit. Currently, it is failing. but after fix the bug it should be
success.

Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
 tools/testing/selftests/damon/Makefile        |  1 +
 .../damon/sysfs_no_op_commit_break.py         | 78 +++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100755 tools/testing/selftests/damon/sysfs_no_op_commit_break.py

diff --git a/tools/testing/selftests/damon/Makefile b/tools/testing/selftests/damon/Makefile
index 5b230deb19e8..44a4a819df55 100644
--- a/tools/testing/selftests/damon/Makefile
+++ b/tools/testing/selftests/damon/Makefile
@@ -17,6 +17,7 @@ TEST_PROGS += reclaim.sh lru_sort.sh
 TEST_PROGS += sysfs_update_removed_scheme_dir.sh
 TEST_PROGS += sysfs_update_schemes_tried_regions_hang.py
 TEST_PROGS += sysfs_memcg_path_leak.sh
+TEST_PROGS += sysfs_no_op_commit_break.py
 
 EXTRA_CLEAN = __pycache__
 
diff --git a/tools/testing/selftests/damon/sysfs_no_op_commit_break.py b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
new file mode 100755
index 000000000000..300cdbaa7eda
--- /dev/null
+++ b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
@@ -0,0 +1,78 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+import json
+import os
+import subprocess
+import sys
+
+import _damon_sysfs
+
+def dump_damon_status_dict(pid):
+    try:
+        subprocess.check_output(['which', 'drgn'], stderr=subprocess.DEVNULL)
+    except:
+        return None, 'drgn not found'
+    file_dir = os.path.dirname(os.path.abspath(__file__))
+    dump_script = os.path.join(file_dir, 'drgn_dump_damon_status.py')
+    rc = subprocess.call(['drgn', dump_script, pid, 'damon_dump_output'],
+        stderr=subprocess.DEVNULL)
+
+    if rc != 0:
+        return None, f'drgn fail : return code({rc})'
+    try:
+        with open('damon_dump_output', 'r') as f:
+            return json.load(f), None
+    except Exception as e:
+        return None, 'json.load fail (%s)' % e
+
+def main():
+    # We can extend this test to change only the Given and reuse the When/Then.
+
+    # Given : setup filter with allow = True
+    proc = subprocess.Popen(['sleep', '10'])
+    kdamonds = _damon_sysfs.Kdamonds(
+        [_damon_sysfs.Kdamond(
+            contexts=[_damon_sysfs.DamonCtx(
+                targets=[_damon_sysfs.DamonTarget(pid=proc.pid)],
+                schemes=[_damon_sysfs.Damos(
+                    ops_filters=[
+                        _damon_sysfs.DamosFilter(type_='anon', matching=True, allow=True)
+                    ]
+                )],
+            )])]
+    )
+
+    err = kdamonds.start()
+    if err is not None:
+        print('kdamond start failed: %s' % err)
+        exit(1)
+
+    before_commit_status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
+    if err is not None:
+        print(err)
+        exit(1)
+
+    # When : No-op commit, it should not break anything
+    kdamonds.kdamonds[0].commit()
+
+    # Then : Check value with drgn
+    after_commit_status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
+    if err is not None:
+        print(err)
+        exit(1)
+
+    before_commit_dump = json.dumps(before_commit_status, sort_keys=True, indent=2)
+    after_commit_dump = json.dumps(after_commit_status, sort_keys=True, indent=2)
+    if before_commit_dump != after_commit_dump:
+        print("[TEST FAILED] : no-op commit break damon status")
+        print("===== before_commit =====")
+        print(before_commit_dump)
+        print("===== after_commit =====")
+        print(after_commit_dump)
+        exit(1)
+
+    kdamonds.stop()
+
+if __name__ == '__main__':
+    main()
-- 
2.43.0

Re: [PATCH 1/2] selftests/damon: test no-op commit broke DAMON status

[SeongJae Park]

Hello Sang-Heon,

On Sat,  9 Aug 2025 04:55:17 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> Add test to verify that DAMON status is not changed after a no-op
> commit.

Thank you for this patch!  I have some comments below, though.

> Currently, it is failing. but after fix the bug it should be
> success.

To keep bisecting goes smoothly, let's introduce failing tests _after_ their
fixes are landed.

> 
> Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> ---
>  tools/testing/selftests/damon/Makefile        |  1 +
>  .../damon/sysfs_no_op_commit_break.py         | 78 +++++++++++++++++++
>  2 files changed, 79 insertions(+)
>  create mode 100755 tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> 
> diff --git a/tools/testing/selftests/damon/Makefile b/tools/testing/selftests/damon/Makefile
> index 5b230deb19e8..44a4a819df55 100644
> --- a/tools/testing/selftests/damon/Makefile
> +++ b/tools/testing/selftests/damon/Makefile
> @@ -17,6 +17,7 @@ TEST_PROGS += reclaim.sh lru_sort.sh
>  TEST_PROGS += sysfs_update_removed_scheme_dir.sh
>  TEST_PROGS += sysfs_update_schemes_tried_regions_hang.py
>  TEST_PROGS += sysfs_memcg_path_leak.sh
> +TEST_PROGS += sysfs_no_op_commit_break.py
>  
>  EXTRA_CLEAN = __pycache__
>  
> diff --git a/tools/testing/selftests/damon/sysfs_no_op_commit_break.py b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> new file mode 100755
> index 000000000000..300cdbaa7eda
> --- /dev/null
> +++ b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> @@ -0,0 +1,78 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import json
> +import os
> +import subprocess
> +import sys
> +
> +import _damon_sysfs
> +
> +def dump_damon_status_dict(pid):
> +    try:
> +        subprocess.check_output(['which', 'drgn'], stderr=subprocess.DEVNULL)
> +    except:
> +        return None, 'drgn not found'
> +    file_dir = os.path.dirname(os.path.abspath(__file__))
> +    dump_script = os.path.join(file_dir, 'drgn_dump_damon_status.py')
> +    rc = subprocess.call(['drgn', dump_script, pid, 'damon_dump_output'],
> +        stderr=subprocess.DEVNULL)
> +
> +    if rc != 0:
> +        return None, f'drgn fail : return code({rc})'

Let's not add a space before ':', for the consistency.

> +    try:
> +        with open('damon_dump_output', 'r') as f:
> +            return json.load(f), None
> +    except Exception as e:
> +        return None, 'json.load fail (%s)' % e
> +
> +def main():
> +    # We can extend this test to change only the Given and reuse the When/Then.

Above comment seems unnecessary to me, since that's obvious.

> +
> +    # Given : setup filter with allow = True

Again, I don't think the above comment is necessary.  And this looks like a
pseudo code more than a formal comment.  If you think it is necessary, please
use more formal tone.

Also let's not add a space before ':', for consistency.

> +    proc = subprocess.Popen(['sleep', '10'])
> +    kdamonds = _damon_sysfs.Kdamonds(
> +        [_damon_sysfs.Kdamond(
> +            contexts=[_damon_sysfs.DamonCtx(
> +                targets=[_damon_sysfs.DamonTarget(pid=proc.pid)],

Can't we start this kdamond with paddr ops, and drop the 'sleep' process
invocation?

> +                schemes=[_damon_sysfs.Damos(
> +                    ops_filters=[
> +                        _damon_sysfs.DamosFilter(type_='anon', matching=True, allow=True)

Let's keep 80 columns limit[1].

> +                    ]
> +                )],
> +            )])]
> +    )
> +
> +    err = kdamonds.start()
> +    if err is not None:
> +        print('kdamond start failed: %s' % err)
> +        exit(1)
> +
> +    before_commit_status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> +    if err is not None:
> +        print(err)
> +        exit(1)
> +
> +    # When : No-op commit, it should not break anything

Again, I don't think the above comment is necessary.  And this looks like a
pseudo code more than a formal comment.  If you think it is necessary, please
use more formal tone.

Also let's not add a space before ':', for consistency.

> +    kdamonds.kdamonds[0].commit()
> +
> +    # Then : Check value with drgn

Again, I don't think the above comment is necessary.  And this looks like a
pseudo code more than a formal comment.  If you think it is necessary, please
use more formal tone.

Also let's not add a space before ':', for consistency.

> +    after_commit_status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> +    if err is not None:
> +        print(err)
> +        exit(1)
> +
> +    before_commit_dump = json.dumps(before_commit_status, sort_keys=True, indent=2)
> +    after_commit_dump = json.dumps(after_commit_status, sort_keys=True, indent=2)

Let's keep 80 columns limit[1].

> +    if before_commit_dump != after_commit_dump:

Can't we jsut compare before_commit_status and after_commit_status?

> +        print("[TEST FAILED] : no-op commit break damon status")

Let's not add a space before ':', for consistency.  In this case, I think ':'
is not necessary at all.  Actually, since this test is for the single test
case, and kselftest framework will say this failed, I think the above print()
is not necessary at all?

> +        print("===== before_commit =====")
> +        print(before_commit_dump)
> +        print("===== after_commit =====")
> +        print(after_commit_dump)
> +        exit(1)
> +
> +    kdamonds.stop()
> +
> +if __name__ == '__main__':
> +    main()
> -- 
> 2.43.0

[1] https://docs.kernel.org/process/coding-style.html#breaking-long-lines-and-strings


Thanks,
SJ

Re: [PATCH 1/2] selftests/damon: test no-op commit broke DAMON status

[Sang-Heon Jeon]

[-- Attachment #1: Type: text/plain, Size: 7608 bytes --]

Hi SeongJae

On Sat, Aug 9, 2025 at 7:05 AM SeongJae Park <sj@kernel.org> wrote:
>
> Hello Sang-Heon,
>
> On Sat,  9 Aug 2025 04:55:17 +0900 Sang-Heon Jeon <ekffu200098@gmail.com>
wrote:
>
> > Add test to verify that DAMON status is not changed after a no-op
> > commit.
>
> Thank you for this patch!  I have some comments below, though.
>
> > Currently, it is failing. but after fix the bug it should be
> > success.
>
> To keep bisecting goes smoothly, let's introduce failing tests _after_
their
> fixes are landed.

I see. As mentioned in another comment, I'll merge this patch set into one
patch. I think this comment will be resolved then.

> >
> > Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> > ---
> >  tools/testing/selftests/damon/Makefile        |  1 +
> >  .../damon/sysfs_no_op_commit_break.py         | 78 +++++++++++++++++++
> >  2 files changed, 79 insertions(+)
> >  create mode 100755
tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> >
> > diff --git a/tools/testing/selftests/damon/Makefile
b/tools/testing/selftests/damon/Makefile
> > index 5b230deb19e8..44a4a819df55 100644
> > --- a/tools/testing/selftests/damon/Makefile
> > +++ b/tools/testing/selftests/damon/Makefile
> > @@ -17,6 +17,7 @@ TEST_PROGS += reclaim.sh lru_sort.sh
> >  TEST_PROGS += sysfs_update_removed_scheme_dir.sh
> >  TEST_PROGS += sysfs_update_schemes_tried_regions_hang.py
> >  TEST_PROGS += sysfs_memcg_path_leak.sh
> > +TEST_PROGS += sysfs_no_op_commit_break.py
> >
> >  EXTRA_CLEAN = __pycache__
> >
> > diff --git a/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> > new file mode 100755
> > index 000000000000..300cdbaa7eda
> > --- /dev/null
> > +++ b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> > @@ -0,0 +1,78 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +import json
> > +import os
> > +import subprocess
> > +import sys
> > +
> > +import _damon_sysfs
> > +
> > +def dump_damon_status_dict(pid):
> > +    try:
> > +        subprocess.check_output(['which', 'drgn'],
stderr=subprocess.DEVNULL)
> > +    except:
> > +        return None, 'drgn not found'
> > +    file_dir = os.path.dirname(os.path.abspath(__file__))
> > +    dump_script = os.path.join(file_dir, 'drgn_dump_damon_status.py')
> > +    rc = subprocess.call(['drgn', dump_script, pid,
'damon_dump_output'],
> > +        stderr=subprocess.DEVNULL)
> > +
> > +    if rc != 0:
> > +        return None, f'drgn fail : return code({rc})'
>
> Let's not add a space before ':', for the consistency.

I see. I made so many mistakes like these; (I worked this very late
night...) I will fix them all. Thank you for checking this moment.

> > +    try:
> > +        with open('damon_dump_output', 'r') as f:
> > +            return json.load(f), None
> > +    except Exception as e:
> > +        return None, 'json.load fail (%s)' % e
> > +
> > +def main():
> > +    # We can extend this test to change only the Given and reuse the
When/Then.
>
> Above comment seems unnecessary to me, since that's obvious.

I agree. As you said, it is obvious. I'll remove all pseudo like comment.

> > +
> > +    # Given : setup filter with allow = True
>
> Again, I don't think the above comment is necessary.  And this looks like
a
> pseudo code more than a formal comment.  If you think it is necessary,
please
> use more formal tone.
>
> Also let's not add a space before ':', for consistency.
>
> > +    proc = subprocess.Popen(['sleep', '10'])
> > +    kdamonds = _damon_sysfs.Kdamonds(
> > +        [_damon_sysfs.Kdamond(
> > +            contexts=[_damon_sysfs.DamonCtx(
> > +                targets=[_damon_sysfs.DamonTarget(pid=proc.pid)],
>
> Can't we start this kdamond with paddr ops, and drop the 'sleep' process
> invocation?

I just referred other tests. And i think we can remove unnecessary targets
and sleep process. I'll fix it on v2 patch.

> > +                schemes=[_damon_sysfs.Damos(
> > +                    ops_filters=[
> > +                        _damon_sysfs.DamosFilter(type_='anon',
matching=True, allow=True)
>
> Let's keep 80 columns limit[1].

Oh, I thought it only applied to 'C' related codes. I'm wrong. Thank you
for reminding me. i'll fix it.

> > +                    ]
> > +                )],
> > +            )])]
> > +    )
> > +
> > +    err = kdamonds.start()
> > +    if err is not None:
> > +        print('kdamond start failed: %s' % err)
> > +        exit(1)
> > +
> > +    before_commit_status, err =
dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> > +    if err is not None:
> > +        print(err)
> > +        exit(1)
> > +
> > +    # When : No-op commit, it should not break anything
>
> Again, I don't think the above comment is necessary.  And this looks like
a
> pseudo code more than a formal comment.  If you think it is necessary,
please
> use more formal tone.

ditto (wil be removed)

> Also let's not add a space before ':', for consistency.
>
> > +    kdamonds.kdamonds[0].commit()
> > +
> > +    # Then : Check value with drgn
>
> Again, I don't think the above comment is necessary.  And this looks like
a
> pseudo code more than a formal comment.  If you think it is necessary,
please
> use more formal tone.
>
> Also let's not add a space before ':', for consistency.

ditto.

> > +    after_commit_status, err =
dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> > +    if err is not None:
> > +        print(err)
> > +        exit(1)
> > +
> > +    before_commit_dump = json.dumps(before_commit_status,
sort_keys=True, indent=2)
> > +    after_commit_dump = json.dumps(after_commit_status,
sort_keys=True, indent=2)
>
> Let's keep 80 columns limit[1].

ditto.

> > +    if before_commit_dump != after_commit_dump:
>
> Can't we jsut compare before_commit_status and after_commit_status?

Yeah, I think i'm just confused. I'll change it.

> > +        print("[TEST FAILED] : no-op commit break damon status")
>
> Let's not add a space before ':', for consistency.  In this case, I think
':'
> is not necessary at all.  Actually, since this test is for the single test
> case, and kselftest framework will say this failed, I think the above
print()
> is not necessary at all?

So, kselftest framework identifies whether the test was successful or not
by exit code. Then, i'll remove this one.

To be honest, I didn't use kselftest framework while making these patch set
because I don't know how kernel developers use it.

If you are okay, could you share how you use it? I just build kernel with
custom patch, and execute output images by vm, and setup env, then just
execute tests(it's so raw.. i know)

Maybe is there a kernel build option that automatically includes tests to
output image? Or just a general or smarter way to execute the whole test?

> > +        print("===== before_commit =====")
> > +        print(before_commit_dump)
> > +        print("===== after_commit =====")
> > +        print(after_commit_dump)
> > +        exit(1)
> > +
> > +    kdamonds.stop()
> > +
> > +if __name__ == '__main__':
> > +    main()
> > --
> > 2.43.0
>
> [1]
https://docs.kernel.org/process/coding-style.html#breaking-long-lines-and-strings
>
>
> Thanks,
> SJ

Thanks for reviewing! Have a nice weekend. I'll do this job after my
personal schedule finishes.

Best Regards.
Sang-Heon Jeon
Sang-Heon Jeon

[-- Attachment #2: Type: text/html, Size: 10559 bytes --]

Re: [PATCH 1/2] selftests/damon: test no-op commit broke DAMON status

[Sang-Heon Jeon]

Hi SeongJae

On Sat, Aug 9, 2025 at 7:05 AM SeongJae Park <sj@kernel.org> wrote:
>
> Hello Sang-Heon,
>
> On Sat,  9 Aug 2025 04:55:17 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> > Add test to verify that DAMON status is not changed after a no-op
> > commit.
>
> Thank you for this patch!  I have some comments below, though.
>
> > Currently, it is failing. but after fix the bug it should be
> > success.
>
> To keep bisecting goes smoothly, let's introduce failing tests _after_ their
> fixes are landed.

I see. As mentioned in another comment, I'll merge this patch set into
one patch. I think this comment will be resolved then.

> >
> > Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> > ---
> >  tools/testing/selftests/damon/Makefile        |  1 +
> >  .../damon/sysfs_no_op_commit_break.py         | 78 +++++++++++++++++++
> >  2 files changed, 79 insertions(+)
> >  create mode 100755 tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> >
> > diff --git a/tools/testing/selftests/damon/Makefile b/tools/testing/selftests/damon/Makefile
> > index 5b230deb19e8..44a4a819df55 100644
> > --- a/tools/testing/selftests/damon/Makefile
> > +++ b/tools/testing/selftests/damon/Makefile
> > @@ -17,6 +17,7 @@ TEST_PROGS += reclaim.sh lru_sort.sh
> >  TEST_PROGS += sysfs_update_removed_scheme_dir.sh
> >  TEST_PROGS += sysfs_update_schemes_tried_regions_hang.py
> >  TEST_PROGS += sysfs_memcg_path_leak.sh
> > +TEST_PROGS += sysfs_no_op_commit_break.py
> >
> >  EXTRA_CLEAN = __pycache__
> >
> > diff --git a/tools/testing/selftests/damon/sysfs_no_op_commit_break.py b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> > new file mode 100755
> > index 000000000000..300cdbaa7eda
> > --- /dev/null
> > +++ b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> > @@ -0,0 +1,78 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +import json
> > +import os
> > +import subprocess
> > +import sys
> > +
> > +import _damon_sysfs
> > +
> > +def dump_damon_status_dict(pid):
> > +    try:
> > +        subprocess.check_output(['which', 'drgn'], stderr=subprocess.DEVNULL)
> > +    except:
> > +        return None, 'drgn not found'
> > +    file_dir = os.path.dirname(os.path.abspath(__file__))
> > +    dump_script = os.path.join(file_dir, 'drgn_dump_damon_status.py')
> > +    rc = subprocess.call(['drgn', dump_script, pid, 'damon_dump_output'],
> > +        stderr=subprocess.DEVNULL)
> > +
> > +    if rc != 0:
> > +        return None, f'drgn fail : return code({rc})'
>
> Let's not add a space before ':', for the consistency.

I see. I made so many mistakes like these; (I worked this very late
night...) I will fix them all. Thank you for checking this moment.

> > +    try:
> > +        with open('damon_dump_output', 'r') as f:
> > +            return json.load(f), None
> > +    except Exception as e:
> > +        return None, 'json.load fail (%s)' % e
> > +
> > +def main():
> > +    # We can extend this test to change only the Given and reuse the When/Then.
>
> Above comment seems unnecessary to me, since that's obvious.

I agree. As you said, it is obvious. I'll remove all pseudo like comment.

> > +
> > +    # Given : setup filter with allow = True
>
> Again, I don't think the above comment is necessary.  And this looks like a
> pseudo code more than a formal comment.  If you think it is necessary, please
> use more formal tone.
>
> Also let's not add a space before ':', for consistency.
>
> > +    proc = subprocess.Popen(['sleep', '10'])
> > +    kdamonds = _damon_sysfs.Kdamonds(
> > +        [_damon_sysfs.Kdamond(
> > +            contexts=[_damon_sysfs.DamonCtx(
> > +                targets=[_damon_sysfs.DamonTarget(pid=proc.pid)],
>
> Can't we start this kdamond with paddr ops, and drop the 'sleep' process
> invocation?

I just referred other tests. And i think we can remove unnecessary
targets and sleep process. I'll fix it on v2 patch.

> > +                schemes=[_damon_sysfs.Damos(
> > +                    ops_filters=[
> > +                        _damon_sysfs.DamosFilter(type_='anon', matching=True, allow=True)
>
> Let's keep 80 columns limit[1].

Oh, I thought it only applied to 'C' related codes. I'm wrong. Thank
you for reminding me. i'll fix it.


> > +                    ]
> > +                )],
> > +            )])]
> > +    )
> > +
> > +    err = kdamonds.start()
> > +    if err is not None:
> > +        print('kdamond start failed: %s' % err)
> > +        exit(1)
> > +
> > +    before_commit_status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> > +    if err is not None:
> > +        print(err)
> > +        exit(1)
> > +
> > +    # When : No-op commit, it should not break anything
>
> Again, I don't think the above comment is necessary.  And this looks like a
> pseudo code more than a formal comment.  If you think it is necessary, please
> use more formal tone.
> Also let's not add a space before ':', for consistency.
>

ditto (wil be removed)

> > +    kdamonds.kdamonds[0].commit()
> > +
> > +    # Then : Check value with drgn
>
> Again, I don't think the above comment is necessary.  And this looks like a
> pseudo code more than a formal comment.  If you think it is necessary, please
> use more formal tone.
>
> Also let's not add a space before ':', for consistency.

ditto.

> > +    after_commit_status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> > +    if err is not None:
> > +        print(err)
> > +        exit(1)
> > +
> > +    before_commit_dump = json.dumps(before_commit_status, sort_keys=True, indent=2)
> > +    after_commit_dump = json.dumps(after_commit_status, sort_keys=True, indent=2)
>
> Let's keep 80 columns limit[1].

ditto.

> > +    if before_commit_dump != after_commit_dump:
>
> Can't we jsut compare before_commit_status and after_commit_status?

Yeah, I think i'm just confused. I'll change it.

> > +        print("[TEST FAILED] : no-op commit break damon status")
>
> Let's not add a space before ':', for consistency.  In this case, I think ':'
> is not necessary at all.  Actually, since this test is for the single test
> case, and kselftest framework will say this failed, I think the above print()
> is not necessary at all?

So, kselftest framework identifies whether the test was successful or
not by exit code. Then, i'll remove this one.

To be honest, I didn't use kselftest framework while making these
patch set because I don't know how kernel developers use it.

If you are okay, could you share how you use it? I just build kernel
with custom patch, and execute output images by vm, and setup env,
then just execute tests(it's so raw.. i know)

Maybe is there a kernel build option that automatically includes tests
to output image? Or just a general or smarter way to execute the whole
test?

> > +        print("===== before_commit =====")
> > +        print(before_commit_dump)
> > +        print("===== after_commit =====")
> > +        print(after_commit_dump)
> > +        exit(1)
> > +
> > +    kdamonds.stop()
> > +
> > +if __name__ == '__main__':
> > +    main()
> > --
> > 2.43.0
>
> [1] https://docs.kernel.org/process/coding-style.html#breaking-long-lines-and-strings
>
>
> Thanks,
> SJ

Thanks for reviewing! Have a nice weekend. I'll do this job after my
personal schedule finishes.

Best Regards.
Sang-Heon Jeon

Re: [PATCH 1/2] selftests/damon: test no-op commit broke DAMON status

[SeongJae Park]

On Sat, 9 Aug 2025 15:39:41 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> Hi SeongJae
> 
> On Sat, Aug 9, 2025 at 7:05 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > Hello Sang-Heon,
> >
> > On Sat,  9 Aug 2025 04:55:17 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> >
> > > Add test to verify that DAMON status is not changed after a no-op
> > > commit.
> >
> > Thank you for this patch!  I have some comments below, though.
> >
> > > Currently, it is failing. but after fix the bug it should be
> > > success.
> >
> > To keep bisecting goes smoothly, let's introduce failing tests _after_ their
> > fixes are landed.
> 
> I see. As mentioned in another comment, I'll merge this patch set into
> one patch. I think this comment will be resolved then.

That could make the patch be unnecessarily difficult to apply on older kernels
due to the test part.  Please keep those separated.

[...]
> > > +    if rc != 0:
> > > +        return None, f'drgn fail : return code({rc})'
> >
> > Let's not add a space before ':', for the consistency.
> 
> I see. I made so many mistakes like these; (I worked this very late
> night...) I will fix them all. Thank you for checking this moment.

I'm sorry to hear that you had to work on late night :'(  I hope you to have
sufficient rests.

[...]
> To be honest, I didn't use kselftest framework while making these
> patch set because I don't know how kernel developers use it.
> 
> If you are okay, could you share how you use it?

Sure.  I do below for running DAMON selftests.

    $ sudo make -C tools/testing/selftests/damon run_tsets

You could also run DAMON kunit tests like below.

    $ ./tools/testing/kunit/kunit.py run --kunitconfig mm/damon/tests/

I usually run all those together with additional tests, using DAMON tests
suite[2].

> I just build kernel
> with custom patch, and execute output images by vm, and setup env,
> then just execute tests(it's so raw.. i know)

It's ok.  No one fits all, so different tests are needed for different hacks.
Please keep using whatever works best for you :)

> 
> Maybe is there a kernel build option that automatically includes tests
> to output image? Or just a general or smarter way to execute the whole
> test?

To be honest I'm also not an expert of kselftest and kunit :)  You coule refer
to the kselftest documentation[1] for more details.

[...]
> Thanks for reviewing! Have a nice weekend. I'll do this job after my
> personal schedule finishes.

No worry, take your time and fun! :)

[...]

[1] https://docs.kernel.org/dev-tools/kselftest.html
[2] https://github.com/damonitor/damon-tests/blob/master/corr/run.sh


Thanks,
SJ

Re: [PATCH 1/2] selftests/damon: test no-op commit broke DAMON status

[Sang-Heon Jeon]

Hi SeongJae

On Sun, Aug 10, 2025 at 2:36 AM SeongJae Park <sj@kernel.org> wrote:
>
> On Sat, 9 Aug 2025 15:39:41 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> > Hi SeongJae
> >
> > On Sat, Aug 9, 2025 at 7:05 AM SeongJae Park <sj@kernel.org> wrote:
> > >
> > > Hello Sang-Heon,
> > >
> > > On Sat,  9 Aug 2025 04:55:17 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > >
> > > > Add test to verify that DAMON status is not changed after a no-op
> > > > commit.
> > >
> > > Thank you for this patch!  I have some comments below, though.
> > >
> > > > Currently, it is failing. but after fix the bug it should be
> > > > success.
> > >
> > > To keep bisecting goes smoothly, let's introduce failing tests _after_ their
> > > fixes are landed.
> >
> > I see. As mentioned in another comment, I'll merge this patch set into
> > one patch. I think this comment will be resolved then.
>
> That could make the patch be unnecessarily difficult to apply on older kernels
> due to the test part.  Please keep those separated.

Oh, I misunderstand your previous comment. Now, I understand your
point. I'll separate them back and make two v3 patch(one for fix bug,
one for test) not a patch set.
Thanks for the explanation, I feel like I'm learning more and more.

> [...]
> > > > +    if rc != 0:
> > > > +        return None, f'drgn fail : return code({rc})'
> > >
> > > Let's not add a space before ':', for the consistency.
> >
> > I see. I made so many mistakes like these; (I worked this very late
> > night...) I will fix them all. Thank you for checking this moment.
>
> I'm sorry to hear that you had to work on late night :'(  I hope you to have
> sufficient rests.

Never mind. just I went with that :) Thanks as always.

> [...]
> > To be honest, I didn't use kselftest framework while making these
> > patch set because I don't know how kernel developers use it.
> >
> > If you are okay, could you share how you use it?
>
> Sure.  I do below for running DAMON selftests.
>
>     $ sudo make -C tools/testing/selftests/damon run_tsets
>
> You could also run DAMON kunit tests like below.
>
>     $ ./tools/testing/kunit/kunit.py run --kunitconfig mm/damon/tests/

Thanks for sharing. I'll try it with my vm environment.

> I usually run all those together with additional tests, using DAMON tests
> suite[2].
>
> > I just build kernel
> > with custom patch, and execute output images by vm, and setup env,
> > then just execute tests(it's so raw.. i know)
>
> It's ok.  No one fits all, so different tests are needed for different hacks.
> Please keep using whatever works best for you :)

You're so right. Maybe I just want to learn some tips from the experts. haha

> >
> > Maybe is there a kernel build option that automatically includes tests
> > to output image? Or just a general or smarter way to execute the whole
> > test?
>
> To be honest I'm also not an expert of kselftest and kunit :)  You coule refer
> to the kselftest documentation[1] for more details.

Thanks for sharing!

> [...]
> > Thanks for reviewing! Have a nice weekend. I'll do this job after my
> > personal schedule finishes.
>
> No worry, take your time and fun! :)
> [...]

:)

> [1] https://docs.kernel.org/dev-tools/kselftest.html
> [2] https://github.com/damonitor/damon-tests/blob/master/corr/run.sh
>
>
> Thanks,
> SJ

Best Regards.
Sang-Heon Jeon

[PATCH 2/2] mm/damon/core : fix commit_ops_filters by using correct nth function

[Sang-Heon Jeon]

damos_commit_ops_filters() incorrectly uses damos_nth_filter() which
iterates core_filters. As a result, performing a commit unintentionally
corrupts ops_filters.

Add damos_nth_ops_filter() which iterates ops_filters. Use this function
to fix issues caused by wrong iteration.

Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
 mm/damon/core.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 883d791a10e5..19c8f01fc81a 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -862,6 +862,18 @@ static struct damos_filter *damos_nth_filter(int n, struct damos *s)
 	return NULL;
 }
 
+static struct damos_filter *damos_nth_ops_filter(int n, struct damos *s)
+{
+	struct damos_filter *filter;
+	int i = 0;
+
+	damos_for_each_ops_filter(filter, s) {
+		if (i++ == n)
+			return filter;
+	}
+	return NULL;
+}
+
 static void damos_commit_filter_arg(
 		struct damos_filter *dst, struct damos_filter *src)
 {
@@ -925,7 +937,7 @@ static int damos_commit_ops_filters(struct damos *dst, struct damos *src)
 	int i = 0, j = 0;
 
 	damos_for_each_ops_filter_safe(dst_filter, next, dst) {
-		src_filter = damos_nth_filter(i++, src);
+		src_filter = damos_nth_ops_filter(i++, src);
 		if (src_filter)
 			damos_commit_filter(dst_filter, src_filter);
 		else
-- 
2.43.0

Re: [PATCH 2/2] mm/damon/core : fix commit_ops_filters by using correct nth function

[SeongJae Park]

On Sat,  9 Aug 2025 04:55:18 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> damos_commit_ops_filters() incorrectly uses damos_nth_filter() which
> iterates core_filters. As a result, performing a commit unintentionally
> corrupts ops_filters.
> 
> Add damos_nth_ops_filter() which iterates ops_filters. Use this function
> to fix issues caused by wrong iteration.

Thank you for finding and fixing this bug!  Since this bug is better to be
fixed as soon as possible, could you please send the next version of this patch
as an individual one, rather than as a part of this patchset?

> 
> Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>

Let's add below for stable kernel maintainers[1].

Fixes: 3607cc590f18 ("mm/damon/core: support committing ops_filters") # 6.15.x
Cc: stable@vger.kernel.org

And the fix looks good to me, so

Reviewed-by: SeongJae Park <sj@kernel.org>

[...]

[1] https://docs.kernel.org/process/stable-kernel-rules.html


Thanks,
SJ

Re: [PATCH 2/2] mm/damon/core : fix commit_ops_filters by using correct nth function

[SeongJae Park]

Forgot saying this, sorry.  Please update the subject to not have a space
before ':', for consistency.


Thanks,
SJ

[...]