On Sun, 15 Mar 2026 14:00:09 -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 | 27 ++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/tools/testing/selftests/damon/sysfs.py > b/tools/testing/selftests/damon/sysfs.py > index e6d34ba05893f..a1a29f1a7c27b 100755 > --- a/tools/testing/selftests/damon/sysfs.py > +++ b/tools/testing/selftests/damon/sysfs.py > @@ -193,18 +193,44 @@ def assert_ctx_committed(ctx, dump): > assert_true(dump['pause'] == ctx.pause, 'pause', dump) > > def assert_ctxs_committed(kdamonds): > + paused_for_dump = False > + if kdamonds.kdamonds[0].contexts[0].pause is False: > + kdamonds.kdamonds[0].contexts[0].pause = True Quoting sashiko.dev comments [1] with ': ' prefix below. : Does this code only pause the first context? The validation loop below : iterates over all contexts in the kdamond, so if there are multiple : contexts, will the others remain unpaused and vulnerable to the race : condition during the state dump? There is no real caller of this function that uses multiple contexts. So there is no real problem. That said, I think this code will be better to take care of such case that might happen in the future. I will therefore add below change to the next version of this patch. ''' --- a/tools/testing/selftests/damon/sysfs.py +++ b/tools/testing/selftests/damon/sysfs.py @@ -196,15 +196,17 @@ def assert_ctx_committed(ctx, dump): assert_true(dump['pause'] == ctx.pause, 'pause', dump) def assert_ctxs_committed(kdamonds): - paused_for_dump = False - if kdamonds.kdamonds[0].contexts[0].pause is False: - kdamonds.kdamonds[0].contexts[0].pause = True - err = kdamonds.kdamonds[0].commit() - if err is not None: - print('pause fail (%s)' % err) - kdamonds.stop() - exit(1) - paused_for_dump = True + 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: @@ -212,17 +214,17 @@ def assert_ctxs_committed(kdamonds): kdamonds.stop() exit(1) - if paused_for_dump: - # resume - kdamonds.kdamonds[0].contexts[0].pause = False - err = kdamonds.kdamonds[0].commit() - if err is not None: - print('resume fail (%s)' % err) - kdamonds.stop() - exit(1) - - # restore for comparison - kdamonds.kdamonds[0].contexts[0].pause = True + 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 ctxs = kdamonds.kdamonds[0].contexts dump = status['contexts'] @@ -230,9 +232,11 @@ def assert_ctxs_committed(kdamonds): for idx, ctx in enumerate(ctxs): assert_ctx_committed(ctx, dump[idx]) - if paused_for_dump: - # restore for the caller - kdamonds.kdamonds[0].contexts[0].pause = False + # restore for the caller + for kd in kdamonds.kdamonds: + for ctx in kd.contexts: + if ctx in ctxs_paused_for_dump: + ctx.pause = False def main(): global kdamonds ''' > + err = kdamonds.kdamonds[0].commit() : Does calling commit() here inadvertently mask test failures by forcing the : entire Python object state to the kernel right before reading the status? : : For example, if a test marks a target obsolete, commits it, and deletes : the target from the Python list to verify if the kernel autonomously : removed it, this commit() would explicitly push the target's deletion to : the kernel, potentially bypassing the test's purpose. No. Callers that could have such problem should call this function after pausing the context on their own. That's what the target obsolete test case is doing. [1] https://sashiko.dev/#/patchset/[email protected] Thanks, SJ [...]
