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

[Sang-Heon Jeon <ekffu200098@gmail.com>]

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