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

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

Hi SeongJae

On Sun, Aug 10, 2025 at 2:48 AM SeongJae Park <sj@kernel.org> wrote:
>
> On Sat,  9 Aug 2025 22:07:56 +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.
> >
> > Also, add test to verify that modification is right way.
>
> As I mentioned on v1 thread, please drop the test part, for ease of
> backporting.

As I mentioned on the v1 thread as well, I'll separate them back.

> >
> > Fixes: 3607cc590f18 ("mm/damon/core: support committing ops_filters") # 6.15.x
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> > ---
> > Changes from v1 [1]:
> > 1. Fix code and commit message style.
> > 2. Merge patch set into one patch.
> > 3. Add fixes and cc section for backporting.
> >
> > [1] https://lore.kernel.org/damon/20250808195518.563053-1-ekffu200098@gmail.com/
> >
> > ---
> > I tried to fix your all comments, but maybe i miss something. Then
> > please let me know; I'll fix it as soon as possible.
> >
> > ---
> >  mm/damon/core.c                               | 14 +++-
> >  tools/testing/selftests/damon/Makefile        |  1 +
> >  .../damon/sysfs_no_op_commit_break.py         | 72 +++++++++++++++++++
> >  3 files changed, 86 insertions(+), 1 deletion(-)
> >  create mode 100755 tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> >
> > 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
> > 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..fbefb1c83045
> > --- /dev/null
> > +++ b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
> > @@ -0,0 +1,72 @@
> > +#!/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():
> > +    kdamonds = _damon_sysfs.Kdamonds(
> > +        [_damon_sysfs.Kdamond(
> > +            contexts=[_damon_sysfs.DamonCtx(
> > +                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)
>
> Adding more context would be nice, e.g.,
>
>     print('before-commit status dump failed: %s' % err)

Sure, I'll add that to v3 patch.

> > +        exit(1)
> > +
> > +    kdamonds.kdamonds[0].commit()
> > +
> > +    after_commit_status, err = \
> > +        dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> > +    if err is not None:
> > +        print(err)
>
> Adding more context would be nice, e.g.,
>
>     print('after-commit status dump failed: %s' % err)

ditto.

> > +        exit(1)
> > +
> > +    if before_commit_status != after_commit_status:
> > +        print(f'before: {json.dump(before_commit_status, indent=2)}')
> > +        print(f'after: {json.dump(after_commit_status, indent=2)}')
> > +        exit(1)
> > +
> > +    kdamonds.stop()
> > +
> > +if __name__ == '__main__':
> > +    main()
> > --
> > 2.43.0
>
> Other than above two trivial comments, nothing stands out to me.
>

Sounds great!

> Thanks,
> SJ

Best Regards.
Sang-Heon Jeon.