Re: (sashiko) [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status

[SeongJae Park <sj@kernel.org>]

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/20260321181343.93971-11-sj@kernel.org
# start of sashiko.dev inline review
commit 019bf04e4448bbce4f6d092792a1b959f718f39a
Author: SeongJae Park <sj@kernel.org>

selftets/damon/sysfs.py: pause DAMON before dumping status

This isn't a bug, but there is a typo (selftets) in the subject line.

This commit updates the DAMON sysfs selftest to pause DAMON execution
during state dumping. This prevents potential races caused by internal
DAMON changes, such as the addition or removal of regions, while the
dump is ongoing.

> diff --git a/tools/testing/selftests/damon/sysfs.py b/tools/testing/selftests/damon/sysfs.py
> --- a/tools/testing/selftests/damon/sysfs.py
> +++ b/tools/testing/selftests/damon/sysfs.py
> @@ -193,18 +193,53 @@ def assert_ctx_committed(ctx, dump):
>      assert_true(dump['pause'] == ctx.pause, 'pause', 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
> +                ctxs_paused_for_dump.append(ctx)
> +        if len(ctxs_paused_for_dump) > 0:
> +            err = kd.commit()

Since ctxs_paused_for_dump is initialized outside the outer loop, if the
first kdamond appends a context to this list, won't len(ctxs_paused_for_dump)
> 0 evaluate to true for all subsequent kdamonds?

This seems like it would trigger kd.commit() on every subsequent kdamond
even if none of its contexts were actually modified.

[ ... ]

> +    # resume contexts paused for safe state dumping
> +    for kd in kdamonds.kdamonds:
> +        for ctx in ctxs_paused_for_dump:
> +            ctx.pause = False
> +        if len(ctxs_paused_for_dump) > 0:
> +            err = kd.commit()

Does this inner loop repeatedly modify contexts across all kdamonds on
every outer iteration?

Because ctxs_paused_for_dump is shared across all kdamonds, this appears
to redundantly modify and commit contexts for every kdamond instance.
Could ctxs_paused_for_dump be scoped per-kdamond instead to avoid this?

[ ... ]

> +    # restore for comparison
> +    for ctx in ctxs_paused_for_dump:
> +        ctx.pause = True

Since the context was forced to pause before the status dump, and the
python object is forced to pause right here before the comparison,
doesn't this mean the test only ever asserts that True == True?

It appears this entirely skips verifying whether the originally requested
unpaused state was successfully committed to the kernel before the dump
procedure began. Any kernel bug where the pause parameter is stuck as
True might silently pass this test now.

Should the original state be verified by reading the specific sysfs file
before overriding it?

[ ... ]


# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260321181343.93971-11-sj@kernel.org
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260321181343.93971-11-sj@kernel.org
#
# [1] https://github.com/sjp38/hackermail