public inbox for linux-kselftest@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] mm/damon: let DAMON be paused and resumed
@ 2026-03-21 18:13 SeongJae Park
  2026-03-21 18:13 ` [PATCH 06/10] mm/damon/tests/core-kunit: test pause commitment SeongJae Park
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 18:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: SeongJae Park, Liam R. Howlett, Brendan Higgins, David Gow,
	David Hildenbrand, Jonathan Corbet, Lorenzo Stoakes, Michal Hocko,
	Mike Rapoport, Shuah Khan, Shuah Khan, Suren Baghdasaryan,
	Vlastimil Babka, damon, kunit-dev, linux-doc, linux-kernel,
	linux-kselftest, linux-mm

DAMON utilizes a few mechanisms that enhance itself over time. Adaptive
regions adjustment, goal-based DAMOS quota auto-tuning and monitoring
intervals auto-tuning like self-training mechanisms are such examples.
It also adds access frequency stability information (age) to the
monitoring results, which makes it enhanced over time.

Sometimes users have to stop DAMON.  In this case, DAMON internal state
that enhanced over the time of the last execution simply goes away.
Restarted DAMON have to train itself and enhance its output from the
scratch.  This makes DAMON less useful in such cases.  Introducing three
such use cases below.

Investigation of DAMON.  It is best to do the investigation online,
especially when it is a production environment.  DAMON therefore
provides features for such online investigations, including DAMOS stats,
monitoring result snapshot exposure, and multiple tracepoints.  When
those are insufficient, and there are additional clues that could be
interfered by DAMON, users have to temporarily stop DAMON to collect the
additional clues.  It is not very useful since many of DAMON internal
clues are gone when DAMON is stopped.  The loss of the monitoring
results that improved over time is also problematic, especially in
production environments.

Monitoring of workloads that have different user-known phases.  For
example, in Android, applications are known to have very different
access patterns and behaviors when they are running on the foreground
and the background.  It can therefore be useful to separate monitoring
of apps based on whether they are running on the foreground and on the
background.  Having two DAMON threads per application that paused and
resumed for the apps foreground/background switches can be useful for
the purpose.  But such pause/resume of the execution is not supported.

Tests of DAMON.  A few DAMON selftests are using drgn to dump the
internal DAMON status.  The tests show if the dumped status is the same
as what the test code expected.  Because DAMON keeps running and
modifying its internal status, there are chances of data races that can
cause false test results.  Stopping DAMON can avoid the race.  But,
since the internal state of DAMON is dropped, the test coverage will be
limited.

Let DAMON execution be paused and resumed without loss of the internal
state, to overhaul the limitations.  For this, introduce a new DAMON
context parameter, namely 'pause'.  API callers can update it while the
context is running, using the online parameters update functions
(damon_commit_ctx() and damon_call()).  Once it is set, kdamond_fn()
main loop will do only limited works excluding the monitoring and DAMOS
works, while sleeping sampling intervals per the work.  The limited
works include handling of the online parameters update.  Hence users can
unset the 'pause' parameter again.  Once it is unset, kdamond_fn() main
loop will do all the work again (resumed).  Under the paused state, it
also does stop condition checks and handling of it, so that paused DAMON
can also be stopped if needed.  Expose the feature to the user space via
DAMON sysfs interface.  Also, update existing drgn-based tests to test
and use the feature.

Tests
=====

I confirmed the feature functionality using real time tracing ('perf
trace' or 'trace-cmd stream') of damon:damon_aggregated DAMON
tracepoint.  By pausing and resuming the DAMON execution, I was able to
see the trace stops and continued as expected.  Note that the pause
feature support is added to DAMON user-space tool (damo) after v3.1.9.
Users can use '--pause_ctx' command line option of damo for that, and I
actually used it for my test.  The extended drgn-based selftests are
also testing a part of the functionality.

Patches Sequence
================

Patch 1 introduces the new core API for the pause feature.  Patch 2
extend DAMON sysfs interface for the new parameter.  Patches 3-5 update
design, usage and ABI documents for the new sysfs file, respectively.
The following five patches are for tests.  Patch 6 implements a new
kunit test for the pause parameter online commitment.  Patches 7 and 8
extend DAMON selftest helpers to support the new feature.  Patch 9
extends selftest to test the commitment of the feature.  Finally, patch
10 updates existing selftest to be safe from the race condition using
the pause/resume feature.

Changelog
=========

Changes from RFC v2
(https://lore.kernel.org/20260319052157.99433-1-sj@kernel.org)
- Move damon_ctx->pause to public fields section.
- Wordsmith design doc change.
- Fix unintended resume of contexts in multiple contexts use case.
- Rebase to latest mm-new.
Changes from RFC v1
(https://lore.kernel.org/20260315210012.94846-1-sj@kernel.org)
- Continuously cancel new damos_walk() requests when paused.
- Initialize damon_sysfs_context->pause.
- Make sysfs.py dump-purpose pausing to work for all contexts.

SeongJae Park (10):
  mm/damon/core: introduce damon_ctx->paused
  mm/damon/sysfs: add pause file under context dir
  Docs/mm/damon/design: update for context pause/resume feature
  Docs/admin-guide/mm/damon/usage: update for pause file
  Docs/ABI/damon: update for pause sysfs file
  mm/damon/tests/core-kunit: test pause commitment
  selftests/damon/_damon_sysfs: support pause file staging
  selftests/damon/drgn_dump_damon_status: dump pause
  selftests/damon/sysfs.py: check pause on assert_ctx_committed()
  selftets/damon/sysfs.py: pause DAMON before dumping status

 .../ABI/testing/sysfs-kernel-mm-damon         |  7 ++++
 Documentation/admin-guide/mm/damon/usage.rst  | 12 ++++--
 Documentation/mm/damon/design.rst             |  7 ++++
 include/linux/damon.h                         |  2 +
 mm/damon/core.c                               |  9 +++++
 mm/damon/sysfs.c                              | 31 ++++++++++++++++
 mm/damon/tests/core-kunit.h                   |  4 ++
 tools/testing/selftests/damon/_damon_sysfs.py | 10 ++++-
 .../selftests/damon/drgn_dump_damon_status.py |  1 +
 tools/testing/selftests/damon/sysfs.py        | 37 +++++++++++++++++++
 10 files changed, 115 insertions(+), 5 deletions(-)


base-commit: 7580a23dc3d9af23b4911f9cc1d9da2f519d63a5
-- 
2.47.3

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 06/10] mm/damon/tests/core-kunit: test pause commitment
  2026-03-21 18:13 [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
@ 2026-03-21 18:13 ` SeongJae Park
  2026-03-21 18:13 ` [PATCH 07/10] selftests/damon/_damon_sysfs: support pause file staging SeongJae Park
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 18:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: SeongJae Park, Brendan Higgins, David Gow, damon, kunit-dev,
	linux-kernel, linux-kselftest, linux-mm

Add a kunit test for commitment of damon_ctx->pause parameter that can
be done using damon_commit_ctx().

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/tests/core-kunit.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 9e5904c2beeb2..0030f682b23b7 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -1077,6 +1077,10 @@ static void damon_test_commit_ctx(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, damon_commit_ctx(dst, src), 0);
 	src->min_region_sz = 4095;
 	KUNIT_EXPECT_EQ(test, damon_commit_ctx(dst, src), -EINVAL);
+	src->min_region_sz = 4096;
+	src->pause = true;
+	KUNIT_EXPECT_EQ(test, damon_commit_ctx(dst, src), 0);
+	KUNIT_EXPECT_TRUE(test, dst->pause);
 	damon_destroy_ctx(src);
 	damon_destroy_ctx(dst);
 }
-- 
2.47.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 07/10] selftests/damon/_damon_sysfs: support pause file staging
  2026-03-21 18:13 [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
  2026-03-21 18:13 ` [PATCH 06/10] mm/damon/tests/core-kunit: test pause commitment SeongJae Park
@ 2026-03-21 18:13 ` SeongJae Park
  2026-03-21 18:13 ` [PATCH 08/10] selftests/damon/drgn_dump_damon_status: dump pause SeongJae Park
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 18:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: SeongJae Park, Shuah Khan, damon, linux-kernel, linux-kselftest,
	linux-mm

DAMON test-purpose sysfs interface control Python module, _damon_sysfs,
is not supporting the newly added pause file.  Add the support of the
file, for future test and use of the feature.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 tools/testing/selftests/damon/_damon_sysfs.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/damon/_damon_sysfs.py b/tools/testing/selftests/damon/_damon_sysfs.py
index 2b4df655d9fd0..120b96ecbd741 100644
--- a/tools/testing/selftests/damon/_damon_sysfs.py
+++ b/tools/testing/selftests/damon/_damon_sysfs.py
@@ -604,10 +604,11 @@ class DamonCtx:
     targets = None
     schemes = None
     kdamond = None
+    pause = None
     idx = None
 
     def __init__(self, ops='paddr', monitoring_attrs=DamonAttrs(), targets=[],
-            schemes=[]):
+            schemes=[], pause=False):
         self.ops = ops
         self.monitoring_attrs = monitoring_attrs
         self.monitoring_attrs.context = self
@@ -622,6 +623,8 @@ class DamonCtx:
             scheme.idx = idx
             scheme.context = self
 
+        self.pause=pause
+
     def sysfs_dir(self):
         return os.path.join(self.kdamond.sysfs_dir(), 'contexts',
                 '%d' % self.idx)
@@ -662,6 +665,11 @@ class DamonCtx:
             err = scheme.stage()
             if err is not None:
                 return err
+
+        err = write_file(os.path.join(self.sysfs_dir(), 'pause'), self.pause)
+        if err is not None:
+            return err
+
         return None
 
 class Kdamond:
-- 
2.47.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 08/10] selftests/damon/drgn_dump_damon_status: dump pause
  2026-03-21 18:13 [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
  2026-03-21 18:13 ` [PATCH 06/10] mm/damon/tests/core-kunit: test pause commitment SeongJae Park
  2026-03-21 18:13 ` [PATCH 07/10] selftests/damon/_damon_sysfs: support pause file staging SeongJae Park
@ 2026-03-21 18:13 ` SeongJae Park
  2026-03-21 18:13 ` [PATCH 09/10] selftests/damon/sysfs.py: check pause on assert_ctx_committed() SeongJae Park
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 18:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: SeongJae Park, Shuah Khan, damon, linux-kernel, linux-kselftest,
	linux-mm

drgn_dump_damon_status is not dumping the damon_ctx->pause parameter
value, so it cannot be tested.  Dump it for future tests.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 tools/testing/selftests/damon/drgn_dump_damon_status.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/damon/drgn_dump_damon_status.py b/tools/testing/selftests/damon/drgn_dump_damon_status.py
index af99b07a4f565..5b90eb8e7ef88 100755
--- a/tools/testing/selftests/damon/drgn_dump_damon_status.py
+++ b/tools/testing/selftests/damon/drgn_dump_damon_status.py
@@ -200,6 +200,7 @@ def damon_ctx_to_dict(ctx):
         ['attrs', attrs_to_dict],
         ['adaptive_targets', targets_to_list],
         ['schemes', schemes_to_list],
+        ['pause', bool],
         ])
 
 def main():
-- 
2.47.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 09/10] selftests/damon/sysfs.py: check pause on assert_ctx_committed()
  2026-03-21 18:13 [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
                   ` (2 preceding siblings ...)
  2026-03-21 18:13 ` [PATCH 08/10] selftests/damon/drgn_dump_damon_status: dump pause SeongJae Park
@ 2026-03-21 18:13 ` SeongJae Park
  2026-03-21 18:13 ` [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status SeongJae Park
  2026-03-21 20:07 ` [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
  5 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 18:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: SeongJae Park, Shuah Khan, damon, linux-kernel, linux-kselftest,
	linux-mm

Extend sysfs.py tests to confirm damon_ctx->pause can be set using the
pause sysfs file.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 tools/testing/selftests/damon/sysfs.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/damon/sysfs.py b/tools/testing/selftests/damon/sysfs.py
index 3aa5c91548a53..e6d34ba05893f 100755
--- a/tools/testing/selftests/damon/sysfs.py
+++ b/tools/testing/selftests/damon/sysfs.py
@@ -190,6 +190,7 @@ def assert_ctx_committed(ctx, dump):
     assert_monitoring_attrs_committed(ctx.monitoring_attrs, dump['attrs'])
     assert_monitoring_targets_committed(ctx.targets, dump['adaptive_targets'])
     assert_schemes_committed(ctx.schemes, dump['schemes'])
+    assert_true(dump['pause'] == ctx.pause, 'pause', dump)
 
 def assert_ctxs_committed(kdamonds):
     status, err = dump_damon_status_dict(kdamonds.kdamonds[0].pid)
-- 
2.47.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status
  2026-03-21 18:13 [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
                   ` (3 preceding siblings ...)
  2026-03-21 18:13 ` [PATCH 09/10] selftests/damon/sysfs.py: check pause on assert_ctx_committed() SeongJae Park
@ 2026-03-21 18:13 ` SeongJae Park
  2026-03-21 20:12   ` (sashiko) " SeongJae Park
  2026-03-21 20:07 ` [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
  5 siblings, 1 reply; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 18:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: SeongJae Park, Shuah Khan, damon, linux-kernel, linux-kselftest,
	linux-mm

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 | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/testing/selftests/damon/sysfs.py b/tools/testing/selftests/damon/sysfs.py
index e6d34ba05893f..7a4cd915d5bc9 100755
--- 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()
+            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:
         print(err)
         kdamonds.stop()
         exit(1)
 
+    # 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()
+            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']
     assert_true(len(ctxs) == len(dump), 'ctxs length', dump)
     for idx, ctx in enumerate(ctxs):
         assert_ctx_committed(ctx, dump[idx])
 
+    # 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():
     kdamonds = _damon_sysfs.Kdamonds(
             [_damon_sysfs.Kdamond(
@@ -302,6 +337,7 @@ def main():
         print('kdamond start failed: %s' % err)
         exit(1)
     kdamonds.kdamonds[0].contexts[0].targets[1].obsolete = True
+    kdamonds.kdamonds[0].contexts[0].pause = True
     kdamonds.kdamonds[0].commit()
     del kdamonds.kdamonds[0].contexts[0].targets[1]
     assert_ctxs_committed(kdamonds)
-- 
2.47.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 00/10] mm/damon: let DAMON be paused and resumed
  2026-03-21 18:13 [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
                   ` (4 preceding siblings ...)
  2026-03-21 18:13 ` [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status SeongJae Park
@ 2026-03-21 20:07 ` SeongJae Park
  5 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 20:07 UTC (permalink / raw)
  To: SeongJae Park
  Cc: Andrew Morton, Liam R. Howlett, Brendan Higgins, David Gow,
	David Hildenbrand, Jonathan Corbet, Lorenzo Stoakes, Michal Hocko,
	Mike Rapoport, Shuah Khan, Shuah Khan, Suren Baghdasaryan,
	Vlastimil Babka, damon, kunit-dev, linux-doc, linux-kernel,
	linux-kselftest, linux-mm

Forwarding sashiko.dev review status for this thread.

# review url: https://sashiko.dev/#/patchset/20260321181343.93971-1-sj@kernel.org

- [PATCH 01/10] mm/damon/core: introduce damon_ctx->paused
  - status: Reviewed
- [PATCH 02/10] mm/damon/sysfs: add pause file under context dir
  - status: Reviewed
  - review: No issues found.
- [PATCH 03/10] Docs/mm/damon/design: update for context pause/resume feature
  - status: Reviewed
  - review: No issues found.
- [PATCH 04/10] Docs/admin-guide/mm/damon/usage: update for pause file
  - status: Reviewed
  - review: No issues found.
- [PATCH 05/10] Docs/ABI/damon: update for pause sysfs file
  - status: Reviewed
  - review: No issues found.
- [PATCH 06/10] mm/damon/tests/core-kunit: test pause commitment
  - status: Reviewed
  - review: No issues found.
- [PATCH 07/10] selftests/damon/_damon_sysfs: support pause file staging
  - status: Reviewed
  - review: No issues found.
- [PATCH 08/10] selftests/damon/drgn_dump_damon_status: dump pause
  - status: Reviewed
  - review: No issues found.
- [PATCH 09/10] selftests/damon/sysfs.py: check pause on assert_ctx_committed()
  - status: Reviewed
  - review: No issues found.
- [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status
  - status: Reviewed

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: (sashiko) [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status
  2026-03-21 18:13 ` [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status SeongJae Park
@ 2026-03-21 20:12   ` SeongJae Park
  2026-03-21 20:47     ` SeongJae Park
  0 siblings, 1 reply; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 20:12 UTC (permalink / raw)
  To: SeongJae Park
  Cc: Andrew Morton, Shuah Khan, damon, linux-kernel, linux-kselftest,
	linux-mm

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: (sashiko) [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status
  2026-03-21 20:12   ` (sashiko) " SeongJae Park
@ 2026-03-21 20:47     ` SeongJae Park
  0 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2026-03-21 20:47 UTC (permalink / raw)
  To: SeongJae Park
  Cc: Andrew Morton, Shuah Khan, damon, linux-kernel, linux-kselftest,
	linux-mm

TL; DR: need to add below attaching fixup.  Unless Andrew pick it together with
this, I will post v2 tomorrow.

On Sat, 21 Mar 2026 13:12:49 -0700 SeongJae Park <sj@kernel.org> wrote:

> 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.

Good eye!  Unless Andrew fixes this up, I will post v2 with the fix tomorrow.

> 
> 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.

Good eye.  There is no multiple kdamonds use case here, but why not make the
code more complete?  I will fix this, like below.

'''
@@ -201,13 +201,16 @@ def assert_ctx_committed(ctx, dump):

 def assert_ctxs_committed(kdamonds):
     ctxs_paused_for_dump = []
+    kdamonds_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:
+                if not kd in kdamonds_paused_for_dump:
+                    kdamonds_paused_for_dump.append(kd)
+        if kd in kdamonds_paused_for_dump:
             err = kd.commit()
             if err is not None:
                 print('pause fail (%s)' % err)
'''

Note that this is not a complete fix.  Similar change will be needed to be made
to later part of this file, too.


> 
> [ ... ]
> 
> > +    # 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?

Good eye!  I will fix this like below.

'''
@@ -221,15 +224,14 @@ def assert_ctxs_committed(kdamonds):
         exit(1)

     # 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()
-            if err is not None:
-                print('resume fail (%s)' % err)
-                kdamonds.stop()
-                exit(1)
+    for ctx in ctxs_paused_for_dump:
+        ctx.pause = False
+    if kd in kdamonds_paused_for_dump:
+        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:
'''

> 
> [ ... ]
> 
> > +    # 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?

I agree there is the room to improve.  But, there is no good way to test the
feature for making the test safe, at the moment.  We have kunit test for the
pause commit feature, though.  So, I'd not add change for this to this patch.

> 
> [ ... ]
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260321181343.93971-11-sj@kernel.org

So, below attaching fixup is needed for this patch.  Unless Andrew pick it
together with this patch, I will post v2 tomorrow.


Thanks,
SJ

=== >8 ===
From 90acf3dacea715790622c0dd6cf97c6ed97d1105 Mon Sep 17 00:00:00 2001
From: SeongJae Park <sj@kernel.org>
Date: Sat, 21 Mar 2026 13:43:50 -0700
Subject: [PATCH] selftests/damon/sysfs.py: fixup: avoid unnecessary commit()
 and resume setup

As Sashiko suggested and I agreed.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 tools/testing/selftests/damon/sysfs.py | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/damon/sysfs.py b/tools/testing/selftests/damon/sysfs.py
index 7a4cd915d5bc9..5f00e97f019f4 100755
--- a/tools/testing/selftests/damon/sysfs.py
+++ b/tools/testing/selftests/damon/sysfs.py
@@ -194,13 +194,16 @@ def assert_ctx_committed(ctx, dump):
 
 def assert_ctxs_committed(kdamonds):
     ctxs_paused_for_dump = []
+    kdamonds_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:
+                if not kd in kdamonds_paused_for_dump:
+                    kdamonds_paused_for_dump.append(kd)
+        if kd in kdamonds_paused_for_dump:
             err = kd.commit()
             if err is not None:
                 print('pause fail (%s)' % err)
@@ -214,15 +217,14 @@ def assert_ctxs_committed(kdamonds):
         exit(1)
 
     # 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()
-            if err is not None:
-                print('resume fail (%s)' % err)
-                kdamonds.stop()
-                exit(1)
+    for ctx in ctxs_paused_for_dump:
+        ctx.pause = False
+    if kd in kdamonds_paused_for_dump:
+        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:
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-03-21 20:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-21 18:13 [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park
2026-03-21 18:13 ` [PATCH 06/10] mm/damon/tests/core-kunit: test pause commitment SeongJae Park
2026-03-21 18:13 ` [PATCH 07/10] selftests/damon/_damon_sysfs: support pause file staging SeongJae Park
2026-03-21 18:13 ` [PATCH 08/10] selftests/damon/drgn_dump_damon_status: dump pause SeongJae Park
2026-03-21 18:13 ` [PATCH 09/10] selftests/damon/sysfs.py: check pause on assert_ctx_committed() SeongJae Park
2026-03-21 18:13 ` [PATCH 10/10] selftets/damon/sysfs.py: pause DAMON before dumping status SeongJae Park
2026-03-21 20:12   ` (sashiko) " SeongJae Park
2026-03-21 20:47     ` SeongJae Park
2026-03-21 20:07 ` [PATCH 00/10] mm/damon: let DAMON be paused and resumed SeongJae Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox