Re: [PATCH 2/2] selftests/damon: validate memcg filter sysfs readback

[SeongJae Park <sj@kernel.org>]

Hello niecheng,


Could we use 'selftests/damn/sysfs.py:' as the prefix of the subject?  That
will make it consistent with other commits and let us easily know to what test
this change is made.

On Thu, 28 May 2026 16:10:39 +0800 niecheng <niecheng1@uniontech.com> wrote:

> Add memcg filter validation to the DAMON sysfs selftest by checking the
> memcg_path sysfs readback.
> 
> Validate the readback path rather than a derived memcg_id so that the
> test stays focused on DAMON sysfs behavior and avoids depending on the
> local userspace cgroup mount layout.

Nice, thank you!

> 
> Also compare the readback path while stripping only the trailing
> newline.
> 
> Signed-off-by: niecheng <niecheng1@uniontech.com>
> ---
>  tools/testing/selftests/damon/sysfs.py | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/damon/sysfs.py b/tools/testing/selftests/damon/sysfs.py
> index 3aa5c91548a5..b595da24f698 100755
> --- a/tools/testing/selftests/damon/sysfs.py
> +++ b/tools/testing/selftests/damon/sysfs.py
> @@ -33,6 +33,7 @@ def assert_true(condition, expectation, status):
>      if condition is not True:
>          fail(expectation, status)
>  
> +
>  def assert_watermarks_committed(watermarks, dump):
>      wmark_metric_val = {
>              'none': 0,
> @@ -90,8 +91,14 @@ def assert_filter_committed(filter_, dump):
>      assert_true(filter_.type_ == dump['type'], 'type', dump)
>      assert_true(filter_.matching == dump['matching'], 'matching', dump)
>      assert_true(filter_.allow == dump['allow'], 'allow', dump)
> -    # TODO: check memcg_path and memcg_id if type is memcg
> -    if filter_.type_ == 'addr':
> +    if filter_.type_ == 'memcg':
> +        shown, rd_err = _damon_sysfs.read_file(
> +                os.path.join(filter_.sysfs_dir(), 'memcg_path'))
> +        if rd_err is not None:
> +            fail('memcg_path sysfs read', {'error': rd_err})
> +        assert_true(shown.rstrip('\n') == ('%s' % filter_.memcg_path),
> +                    'memcg_path_sysfs', dump)
> +    elif filter_.type_ == 'addr':
>          assert_true([filter_.addr_start, filter_.addr_end] ==
>                      dump['addr_range'], 'addr_range', dump)
>      elif filter_.type_ == 'target':

But this is in the middle of assert_filter_committed().  As the name says, this
is for validating if the user inputs are passed to DAMON internal.  I think
this is not the right place to test its 'staging' functionality.

> @@ -258,6 +265,8 @@ def main():
>                  ops_filters=[
>                      _damon_sysfs.DamosFilter(type_='anon', matching=True,
>                                               allow=True),
> +                    _damon_sysfs.DamosFilter(type_='memcg', matching=True,
> +                                             allow=True, memcg_path='/'),
>                      ],
>                  )])
>      context.idx = 0

What about adding another test case at the end of the main() function for the
validation of memcg string staging functionality?


Thanks,
SJ

[...]