Re: [PATCH v2 2/2] selftests/damon/sysfs.py: validate memcg_path staging readback

[SeongJae Park <sj@kernel.org>]

On Fri, 29 May 2026 09:43:26 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The newly added `test_memcg_filter_memcg_path_staging` test inadvertently depends on `CONFIG_MEMCG` and active cgroup configurations due to a call to `commit()`, causing the test to fail inappropriately on environments lacking them.
> --
> 
> commit 45ed1607d8ff8754a1a322fb6da3631cdc56f283
> Author: Cheng Nie <niecheng1@uniontech.com>
> 
> selftests/damon/sysfs.py: validate memcg_path staging readback
> 
> This commit adds a dedicated test to selftests/damon/sysfs.py that writes
> memcg_path to sysfs and verifies its readback. It aims to test the sysfs
> behavior independently of the local userspace cgroup mount layout.
> 
> > diff --git a/tools/testing/selftests/damon/sysfs.py b/tools/testing/selftests/damon/sysfs.py
> > index aa03a1187489f..0f20bc9c46b78 100755
> > --- a/tools/testing/selftests/damon/sysfs.py
> > +++ b/tools/testing/selftests/damon/sysfs.py
> > @@ -250,6 +250,43 @@ def assert_ctxs_committed(kdamonds):
> [ ... ]
> > +    context = _damon_sysfs.DamonCtx(
> > +            targets=[_damon_sysfs.DamonTarget(pid=-1)],
> > +            schemes=[_damon_sysfs.Damos(ops_filters=[memcg_filter])])
> > +    context.idx = 0
> > +    context.kdamond = kdamonds.kdamonds[0]
> > +    kdamonds.kdamonds[0].contexts = [context]
> > +
> > +    err = kdamonds.kdamonds[0].commit()
> 
> [Severity: Medium]
> Could this cause a regression where the test fails on environments lacking
> memcg support or active cgroups?
> 
> The stated goal of this test is to validate the sysfs staging readback
> without depending on local userspace cgroup mount layouts. However, when
> commit() is called, it instructs the kernel to apply the configuration.
> 
> This triggers damon_sysfs_memcg_path_to_id(), which traverses online memory
> cgroups and will return -EINVAL if memcg support is disabled or the root
> cgroup '/' cannot be resolved to an active memcg. This causes the commit()
> operation to fail, which makes the entire selftest report a regression.

Good finding, thanks.

> 
> Would it be better to only stage the values (e.g., via context.stage())
> and then verify the readback, without triggering the operational commit
> step?

We have no stage() method for Kdamonds, though.  I'm suggesting [1] to just do
'start()', but ignore the failure.  Cheng, let's keep discussion on the
original thread [1].

[1] https://lore.kernel.org/20260529150057.84753-1-sj@kernel.org


Thanks,
SJ

[...]