Re: [RFC PATCH v2 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status

[SeongJae Park <sj@kernel.org>]

On Fri, 20 Mar 2026 08:41:33 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Fri, 20 Mar 2026 08:29:39 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > Adding Sashiko comment
> > (https://lore.kernel.org/https://sashiko.dev/#/patchset/20260319052157.99433-11-sj@kernel.org)
> > with ': ' line prefix, and my replies in line.
> > 
> > On Wed, 18 Mar 2026 22:21:53 -0700 SeongJae Park <sj@kernel.org> wrote:
> > 
> > > The sysfs.py test commits DAMON parameters, dump the internal DAMON
> > > state, and show if the parameters are committed as expected using the
> > > dumped state.  While the dumping is ongoing, DAMON is alive.  It can
> > > make internal changes including addition and removal of regions.  It can
> > > therefore make a race that can result in false test results.  Pause
> > > DAMON execution during the state dumping to avoid such races.
> > > 
> > > Signed-off-by: SeongJae Park <sj@kernel.org>
> > > ---
> > >  tools/testing/selftests/damon/sysfs.py | 31 ++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/damon/sysfs.py b/tools/testing/selftests/damon/sysfs.py
> > > index e6d34ba05893f..704729c7a318e 100755
> > > --- a/tools/testing/selftests/damon/sysfs.py
> > > +++ b/tools/testing/selftests/damon/sysfs.py
> > > @@ -193,18 +193,48 @@ def assert_ctx_committed(ctx, dump):
> > >      assert_true(dump['pause'] == ctx.pause, 'pause', dump)
> > >  
> > >  def assert_ctxs_committed(kdamonds):
> > > +    ctxs_paused_for_dump = []
> > > +    for kd in kdamonds.kdamonds:
> > > +        for ctx in kd.contexts:
> > > +            if ctx.pause is False:
> > > +                ctx.pause = True
> > > +                err = kd.commit()
> > > +                if err is not None:
> > > +                    print('pause fail (%s)' % err)
> > > +                    kdamonds.stop()
> > > +                    exit(1)
> > > +                ctxs_paused_for_dump.append(ctx)
> > > +
> > >      status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
> > >      if err is not None:
> > >          print(err)
> > >          kdamonds.stop()
> > >          exit(1)
> > >  
> > > +    for kd in kdamonds.kdamonds:
> > > +        for ctx in kd.contexts:
> > > +            if ctx in ctxs_paused_for_dump:
> > > +                ctx.pause = False
> > > +                err = kd.commit()
> > > +                if err is not None:
> > > +                    print('resume fail (%s)' % err)
> > > +                    kdamonds.stop()
> > > +                    exit(1)
> > > +                # restore for comparison
> > > +                ctx.pause = True
> > 
> > : If a kdamond contains multiple contexts, does this loop leave earlier contexts
> > : paused in the kernel?
> > : 
> > : Since kd.commit() stages and commits the state of all contexts associated with
> > : the kdamond, when kd.commit() is called for the second context, the first
> > : context's local pause attribute is already back to True.
> > : 
> > : This would cause kd.commit() to write to sysfs and instruct the kernel to
> > : pause the first context again. By the end of this loop, only the last context
> > : in the kdamond would remain unpaused in the kernel.
> > 
> > No.  The pause field of the earlier context is set to False, so later
> > kd.commit() will commit the False 'pause' again.  But this finds a good point.
> > There is no reason to call kd.commit() for each context.  It is more efficient
> > to be called for each kdamond., thouth currently we support only one context
> > per kdamond.  I will update the code so, in the next spin.
> 
> I'm wrong, Sashiko is correct.  I missed the last 'ctx.pause = True' in the
> loop.  I will fix this together, like below:
> 
> '''
> --- a/tools/testing/selftests/damon/sysfs.py
> +++ b/tools/testing/selftests/damon/sysfs.py
> @@ -201,16 +201,17 @@ def assert_ctx_committed(ctx, dump):
> 
>  def assert_ctxs_committed(kdamonds):
>      ctxs_paused_for_dump = []
> +    # pause for safe state dumping
>      for kd in kdamonds.kdamonds:
>          for ctx in kd.contexts:
>              if ctx.pause is False:
>                  ctx.pause = True
> -                err = kd.commit()
> -                if err is not None:
> -                    print('pause fail (%s)' % err)
> -                    kdamonds.stop()
> -                    exit(1)
>                  ctxs_paused_for_dump.append(ctx)
> +        err = kd.commit()
> +        if err is not None:
> +            print('pause fail (%s)' % err)
> +            kdamonds.stop()
> +            exit(1)
> 
>      status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
>      if err is not None:
> @@ -218,17 +219,19 @@ def assert_ctxs_committed(kdamonds):
>          kdamonds.stop()
>          exit(1)
> 
> +    # resume contexts paused for safe state dumping
>      for kd in kdamonds.kdamonds:
> -        for ctx in kd.contexts:
> -            if ctx in ctxs_paused_for_dump:
> -                ctx.pause = False
> -                err = kd.commit()
> -                if err is not None:
> -                    print('resume fail (%s)' % err)
> -                    kdamonds.stop()
> -                    exit(1)
> -                # restore for comparison
> -                ctx.pause = True
> +        for ctx in ctxs_paused_for_dump:
> +            ctx.pause = False
> +        err = kd.commit()
> +        if err is not None:
> +            print('resume fail (%s)' % err)
> +            kdamonds.stop()
> +            exit(1)
> +
> +    # restore for comparison
> +    for ctx in ctxs_paused_for_dump:
> +        ctx.pause = True
> 
>      ctxs = kdamonds.kdamonds[0].contexts
>      dump = status['contexts']
> '''

The above fixup makes the test fails, for the obsolete targets.  The fixup
needs another fixup, like below.  I will add that to the next spin.

'''
--- a/tools/testing/selftests/damon/sysfs.py
+++ b/tools/testing/selftests/damon/sysfs.py
@@ -200,11 +200,12 @@ def assert_ctxs_committed(kdamonds):
             if ctx.pause is False:
                 ctx.pause = True
                 ctxs_paused_for_dump.append(ctx)
-        err = kd.commit()
-        if err is not None:
-            print('pause fail (%s)' % err)
-            kdamonds.stop()
-            exit(1)
+        if len(ctxs_paused_for_dump) > 0:
+            err = kd.commit()
+            if err is not None:
+                print('pause fail (%s)' % err)
+                kdamonds.stop()
+                exit(1)

     status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
     if err is not None:
@@ -216,11 +217,12 @@ def assert_ctxs_committed(kdamonds):
     for kd in kdamonds.kdamonds:
         for ctx in ctxs_paused_for_dump:
             ctx.pause = False
-        err = kd.commit()
-        if err is not None:
-            print('resume fail (%s)' % err)
-            kdamonds.stop()
-            exit(1)
+        if len(ctxs_paused_for_dump) > 0:
+            err = kd.commit()
+            if err is not None:
+                print('resume fail (%s)' % err)
+                kdamonds.stop()
+                exit(1)

     # restore for comparison
     for ctx in ctxs_paused_for_dump:
'''


Thanks,
SJ