mm/damon: cleanup, clarify, and add/improve test

damos_adjust_quota() is manually checking if the user set the DAMOS quota. There is a dedicated helper, damos_quota_is_set(), for the purpose. Use the helper.

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/damon/core.c +0 -0

index cf10f6d725c19..dd27068cb83e3 100644

mm/damon/core.c +0 -0

 		return;
 
 	/* First charge window */

Signature · 1 line

2.47.3

Quoted 14 lines · On Sat, 12 Sep 2026 13:08:05 -0700 SJ Park <[email protected]> wrote:

damos_adjust_quota() is manually checking if the user set the DAMOS quota. There is a dedicated helper, damos_quota_is_set(), for the purpose. Use the helper.

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/damon/core.c +0 -0

index cf10f6d725c19..dd27068cb83e3 100644

mm/damon/core.c static void damos_adjust_quota(struct damon_ctx *c, struct damos *s) · line 3347

Quoted 5 lines

mm/damon/core.c +1 -1

@@ -3344,7 +3344,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
 	unsigned long cumulated_sz, cached_esz;
 	unsigned int score, max_score = 0;
 
-	if (!quota->ms && !quota->sz && list_empty(&quota->goals))
+	if (!damos_quota_is_set(quota))

Hi SJ,
This is more than a cleanup, because damos_quota_is_set() also checks quota->esz, which the original condition did not.

The path that makes this matter: damos_commit_quota() copies ms and sz from sysfs but not esz. So if a scheme runs with a non-zero ms, then the user sets ms=0 and commits, the running scheme gets ms=0 while esz keeps its old value.

The old check would then return early, leaving esz stale. That stale esz is visible to damos_valid_target() and damos_quota_is_full(), which also use damos_quota_is_set().

With the helper, damos_adjust_quota() proceeds into damos_set_effective_quota(), where the first branch resets esz from sz when ms and goals are unset.

The code change is correct, but worth noting the behavioral change in the commit message.

Quoted 39 lines · On Mon, 14 Sep 2026 23:01:48 +0800 Kunwu Chan <[email protected]> wrote:

Quoted 14 lines · On Sat, 12 Sep 2026 13:08:05 -0700 SJ Park <[email protected]> wrote:

damos_adjust_quota() is manually checking if the user set the DAMOS quota. There is a dedicated helper, damos_quota_is_set(), for the purpose. Use the helper.

Signed-off-by SJ Park <[email protected]>

---
 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/damon/core.c +0 -0

index cf10f6d725c19..dd27068cb83e3 100644

mm/damon/core.c static void damos_adjust_quota(struct damon_ctx *c, struct damos *s) · line 3347

Quoted 5 lines

mm/damon/core.c +1 -1

@@ -3344,7 +3344,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
    unsigned long cumulated_sz, cached_esz;
    unsigned int score, max_score = 0;
 
-   if (!quota->ms && !quota->sz && list_empty(&quota->goals))
+   if (!damos_quota_is_set(quota))

Hi SJ,
This is more than a cleanup, because damos_quota_is_set() also checks quota->esz, which the original condition did not.

The path that makes this matter: damos_commit_quota() copies ms and sz from sysfs but not esz. So if a scheme runs with a non-zero ms, then the user sets ms=0 and commits, the running scheme gets ms=0 while esz keeps its old value.

The old check would then return early, leaving esz stale. That stale esz is visible to damos_valid_target() and damos_quota_is_full(), which also use damos_quota_is_set().

With the helper, damos_adjust_quota() proceeds into damos_set_effective_quota(), where the first branch resets esz from sz when ms and goals are unset.

Quoted 2 lines

The code change is correct, but worth noting the behavioral change in the commit message.

So, if user sets only time quota, and then commits the quota to zero while the esz is non-zero, esz is never reset to zero. As a result, DAMOS keeps applying the action with the esz forever. Correct?

If so, this may deserve Cc-ing stable@. Kunwu, what do you think?

Thanks,
SJ

[...]

Quoted 4 lines · SJ Park

Quoted 1 line · On Mon, 14 Sep 2026 23:01:48 +0800 Kunwu Chan <[email protected]> wrote:

On Sat, 12 Sep 2026 13:08:05 -0700 SJ Park <[email protected]> wrote:

Quoted 3 lines

So, if user sets only time quota, and then commits the quota to zero while the esz is non-zero, esz is never reset to zero. As a result, DAMOS keeps applying the action with the esz forever. Correct?

Yes, the stale esz path is confirmed.

One clarification, though: it does not keep applying the action forever. While charged_sz < stale_esz, damos_adjust_quota() still returns early, but damos_apply_target() continues to apply the scheme and damos_charge_quota() keeps increasing charged_sz.

Once charged_sz >= stale_esz, damos_quota_is_full() returns true, so damon_do_apply_schemes() skips the scheme. Since the early return also prevents the charge-window handling from running, charged_sz is never reset, and the scheme remains permanently inactive.

So clearing the quota can result in the scheme applying up to the stale esz, and then stopping permanently.

Quoted 3 lines

If so, this may deserve Cc-ing stable@. Kunwu, what do you think?

I think this is a functional bug in the existing code, and Cc'ing stable makes sense.

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Brittle cancellation polling loop in damon_call() can cause indefinite hangs on rapid context restarts.
- [High] Missing initialization of walk_control_obsolete in damon_new_ctx() causes unrecoverable hangs in damos_walk(). --

commit edbd3bc657324e35d324500107e2e30a1ffabc31
Author: SJ Park <[email protected]>

mm/damon/core: document damon_call()/damon_start() race hang issue

This commit documents a potential race condition and indefinite hang between damon_start() and damon_call() if they are executed in parallel for the same DAMON context. It adds a cautionary comment to damon_call() to warn API users about this behavior.

mm/damon/core.c int damon_kdamond_pid(struct damon_ctx *ctx) · line 2223

Quoted 11 lines

mm/damon/core.c +3 -0

index 6e4b0144c3dd1..f304813a08460 100644
@@ -2218,6 +2218,9 @@ int damon_kdamond_pid(struct damon_ctx *ctx)
  *
  * When this function is failed, the @ctx is guaranteed to be stopped.
  *
+ * This function should not be called in parallel to damon_start() for the
+ * @ctx.  In the case, this function could indefinitely hang.
+ *

[Severity: High]
This is a pre-existing issue, but does the cancellation polling loop in damon_call() cause indefinite hangs on rapid context restarts?

If Thread A is blocked in damon_call(), and Thread B stops the DAMON context (e.g., via module parameters), the monitoring thread cancels the call and exits.

If Thread A is preempted before evaluating the canceled polling loop, and Thread C concurrently restarts the context, Thread A resumes, evaluates damon_is_running(ctx) to true (due to the newly spawned thread), and hangs forever waiting for the new thread to stop:

mm/damon/core.c:damon_call() {
    ...
canceled:
        while (damon_is_running(ctx))
                schedule_timeout_idle(msecs_to_jiffies(100));
        return -ECANCELED;
}

This race can be triggered by root via concurrent sysfs or module parameter writes (e.g., in damon_reclaim).

[Severity: High]
This is a pre-existing issue, but does the missing initialization of walk_control_obsolete in damon_new_ctx() cause unrecoverable hangs in damos_walk()?

damon_new_ctx() explicitly initializes call_controls_obsolete = true to protect damon_call() from hanging if invoked before the context is started. However, it fails to initialize walk_control_obsolete:

mm/damon/core.c:damon_new_ctx() {
    ...
        ctx->call_controls_obsolete = true;
        prandom_seed_state(&ctx->rnd_state, get_random_u64());
    ...
}

If damos_walk() is called before damon_start(), it bypasses the safety check and blocks forever on wait_for_completion(), because no thread exists to signal it:

mm/damon/core.c:damos_walk() {
    ...
        mutex_lock(&ctx->walk_control_lock);
        if (ctx->walk_control_obsolete) {
                mutex_unlock(&ctx->walk_control_lock);
                return -ECANCELED;
        }
    ...
}

Even though it requires erroneous API usage, the API intends to gracefully reject this via the obsolete flag. The missing initialization breaks this boundary, leading to an unkillable hang.

Quoted 4 lines

mm/damon/core.c +0 -0

  * Return: 0 on success, negative error code otherwise.
  */
 int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Use of the non-standard --quiet flag in grep can cause the test to silently mask real memory leaks on systems with strict POSIX grep implementations.
- [Low] Typo in the warning message: 'apparenty' instead of 'apparently'.
--

commit afc83fe4758b4c2e76cc3da97cb99e2a990b6b83
Author: SJ Park <[email protected]>

selftests/damon/sysfs_memcg_path_leak: fail only for real DAMON leak

The selftest can fail for any leak if it happens while the test is running. Remove the false positive test failures by further checking if the expected leaking function is called out on the report.

tools/testing/selftests/damon/sysfs_memcg_path_leak.sh if [ "$kmemleak_report" = "" ] · line 44

Quoted 9 lines

tools/testing/selftests/damon/sysfs_memcg_path_leak.sh +1 -0

index 33a7ff43ed6cc..6c2d8bb3fe570 100755
@@ -41,5 +41,12 @@ if [ "$kmemleak_report" = "" ]
 then
 	exit 0
 fi
+if ! echo "$kmemleak_report" | grep "memcg_path_store" --quiet

[Severity: Medium]
Could the --quiet flag cause the test to accidentally mask real memory leaks?

If the test runs in an environment with a strict POSIX grep implementation that does not support the GNU extension --quiet, grep will fail with an unrecognized option error.

Because of the negated condition, a non-zero exit code from grep causes the script to evaluate to true, print a warning, and exit 0. Should the standard POSIX -q flag be used instead to ensure compatibility?

tools/testing/selftests/damon/sysfs_memcg_path_leak.sh if [ "$kmemleak_report" = "" ] · line 46

Quoted 3 lines

tools/testing/selftests/damon/sysfs_memcg_path_leak.sh +2 -0

+then
+	echo "[WARN] memleak found; apparenty not from DAMON, though"

[Severity: Low]
This isn't a bug, but there is a typo in the warning message (apparenty instead of apparently).

Quoted 7 lines

tools/testing/selftests/damon/sysfs_memcg_path_leak.sh +4 -0

+	echo "$kmemleak_report"
+	exit 0
+fi
+
 echo "$kmemleak_report"
 exit 1

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

DAMON design document uses "bp" for "basis point" in multiple places. Because it is not clearly mentioned, it is difficult to understand what "bp" stands for. Add the clarification.

Reported-by Randy Dunlap <[email protected]>

Closes https://lore.kernel.org/[email protected] open here

Signed-off-by SJ Park <[email protected]>

---
 Documentation/mm/damon/design.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Documentation/mm/damon/design.rst +3 -2

index 0a86792f90a18..e82390e77a70a 100644
@@ -445,7 +445,7 @@ users to set the aimed amount of access events to observe via DAMON within
 given time interval.  The target can be specified by the user as a ratio of
 DAMON-observed access events to the theoretical maximum amount of the events
 (``access_bp``) that measured within a given number of aggregations
-(``aggrs``).
+(``aggrs``).  The ratio is in basis point (bp or 1/10,000).
 
 The DAMON-observed access events are calculated in byte granularity based on
 DAMON :ref:`region assumption <damon_design_region_based_sampling>`.  For

@@ -717,7 +717,8 @@ mechanism tries to make ``current_value`` of ``target_metric`` be same to
   in microseconds that measured from last quota reset to next quota reset.
   DAMOS does the measurement on its own, so only ``target_value`` need to be
   set by users at the initial time.  In other words, DAMOS does self-feedback.
-- ``node_mem_used_bp``: Specific NUMA node's used memory ratio in bp (1/10,000).
+- ``node_mem_used_bp``: Specific NUMA node's used memory ratio in basis point
+  (bp or 1/10,000).
 - ``node_mem_free_bp``: Specific NUMA node's free memory ratio in bp (1/10,000).
 - ``node_memcg_used_bp``: Specific cgroup's node used memory ratio for a
   specific NUMA node, in bp (1/10,000).