On Fri, 20 Mar 2026 08:41:33 -0700 SeongJae Park <[email protected]> wrote:
> On Fri, 20 Mar 2026 08:29:39 -0700 SeongJae Park <[email protected]> wrote: > > > Adding Sashiko comment > > (https://lore.kernel.org/https://sashiko.dev/#/patchset/[email protected]) > > with ': ' line prefix, and my replies in line. > > > > On Wed, 18 Mar 2026 22:21:53 -0700 SeongJae Park <[email protected]> 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 <[email protected]> > > > --- > > > 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
