[PATCH 0/2] mm/damon: preserve quota state when constructing schemes

[PATCH 0/2] mm/damon: preserve quota state when constructing schemes

[Karl Mehltretter]

damon_new_scheme() initializes the caller's quota in place before copying
it. When damon_commit_ctx() clones the running context for validation,
the quota pointer belongs to a running scheme. Constructing the temporary
scheme therefore clears the running scheme's quota state, even if the
proposed update is later rejected with -EINVAL.

Initialize the new scheme's copy instead, and add KUnit tests for the
constructor and for accepted and rejected context updates.

In a live test with damo, a scheme with a plain 64 KiB size quota and a
60-second reset interval uses its quota, and a full "damo tune" with
unchanged parameters then lets it try another 64 KiB within the same
window. With the fix, sz_tried stays at 64 KiB.

With only patch 2 applied, the two new tests fail on x86-64 and i386.
With the fix, all 43 DAMON KUnit tests pass on both, and the DAMON
selftests show no new failures (QEMU TCG guest; the wss_estimation test
misses its accuracy bounds with and without the fix).

Karl Mehltretter (2):
  mm/damon/core: preserve the caller's quota in damon_new_scheme()
  mm/damon/tests/core-kunit: test preservation of caller quota state

 mm/damon/core.c             |   6 +-
 mm/damon/tests/core-kunit.h | 107 ++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 3 deletions(-)


base-commit: 185111f116aabf202d12ce440c0f6e9bae073514
-- 
2.53.0

[PATCH 1/2] mm/damon/core: preserve the caller's quota in damon_new_scheme()

[Karl Mehltretter]

damon_new_scheme() calls damos_quota_init() on the caller's quota before
copying it to the new scheme. This clears the caller's effective quota,
feedback input and charging state as a side effect.

damon_commit_ctx() first copies the running context into a temporary
context for validating the proposed parameters. When
damon_commit_schemes() creates the temporary schemes, it passes the quota
of each running scheme to damon_new_scheme(). The quota pointer therefore
refers to the running scheme, and damos_quota_init() clears that scheme's
state before it is copied to the temporary scheme. Even an update
rejected with -EINVAL loses the running quota state.

For a size quota, this discards the bytes already charged and allows the
scheme to use a fresh quota before the reset interval has elapsed. For a
goal-driven quota, the consist tuner loses its accumulated input and
restarts from its minimum input. A time quota loses its throughput
estimate and falls back to the initial estimate.

The constructor side effect was introduced by commit 70e0c1d1bf94
("mm/damon/core: factor out 'damos_quota' private fileds initialization").
Commit 60bd24f272d0 ("mm/damon/sysfs: test commit input against realistic
destination"), merged in v6.19, exposed it when
validating sysfs updates against a copy of the running context. Commit
b90408ef1163 ("mm/damon/core: safely validate src on damon_commit_ctx()")
later moved that validation into the core API.

Sashiko reported the same side effect [1] on the RFC of the core API
change.

Copy the quota to the new scheme first, then initialize that copy. Make
damos_quota_init() return void, since its return value is no longer needed.

Fixes: 70e0c1d1bf94 ("mm/damon/core: factor out 'damos_quota' private fileds initialization")
Cc: <stable@vger.kernel.org> # 6.19.x
Link: https://lore.kernel.org/damon/20260702212143.0CB6D1F00A3D@smtp.kernel.org/ [1]
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 mm/damon/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 2258b72da7a78..e655863d33d9c 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -734,7 +734,7 @@ static bool damos_quota_goals_empty(struct damos_quota *q)
 }
 
 /* initialize fields of @quota that normally API users wouldn't set */
-static struct damos_quota *damos_quota_init(struct damos_quota *quota)
+static void damos_quota_init(struct damos_quota *quota)
 {
 	quota->esz = 0;
 	quota->total_charged_sz = 0;
@@ -744,7 +744,6 @@ static struct damos_quota *damos_quota_init(struct damos_quota *quota)
 	quota->charge_target_from = NULL;
 	quota->charge_addr_from = 0;
 	quota->esz_bp = 0;
-	return quota;
 }
 
 struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
@@ -776,7 +775,8 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
 	scheme->last_applied = NULL;
 	INIT_LIST_HEAD(&scheme->list);
 
-	scheme->quota = *(damos_quota_init(quota));
+	scheme->quota = *quota;
+	damos_quota_init(&scheme->quota);
 	/* quota.goals should be separately set by caller */
 	INIT_LIST_HEAD(&scheme->quota.goals);
 
-- 
2.53.0

[PATCH 2/2] mm/damon/tests/core-kunit: test preservation of caller quota state

[Karl Mehltretter]

Check that damon_new_scheme() initializes the new scheme's quota without
changing the caller's quota. Cover all eight fields initialized by
damos_quota_init().

Also check that damon_commit_ctx() preserves those fields in the
destination scheme for both accepted and rejected parameter updates.
Use an invalid min_region_sz for the rejected update and confirm that
returning -EINVAL leaves the running quota state unchanged.

Without the preceding fix, all eight fields are cleared in the
constructor test and in both context update cases.

Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
Please feel free to take the fix on its own. I can also fold the
context-update checks into damon_test_commit_ctx() if that would fit
better.

 mm/damon/tests/core-kunit.h | 107 ++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 5ff0436c58441..811fd668e2482 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -812,6 +812,48 @@ static void damos_test_new_filter(struct kunit *test)
 	damos_destroy_filter(filter);
 }
 
+static void damos_test_new_scheme_keeps_src_quota(struct kunit *test)
+{
+	struct damos_access_pattern pattern = {};
+	struct damon_target target = {};
+	struct damos_quota quota = {
+		.sz = SZ_64K,
+		.esz = 123,
+		.esz_bp = 456,
+		.total_charged_sz = 789,
+		.total_charged_ns = 1011,
+		.charged_sz = 12,
+		.charged_from = 13,
+		.charge_target_from = &target,
+		.charge_addr_from = 14,
+	};
+	struct damos_watermarks wmarks = {};
+	struct damos *s;
+
+	s = damon_new_scheme(&pattern, DAMOS_STAT, 0, &quota, &wmarks,
+			NUMA_NO_NODE);
+	if (!s)
+		kunit_skip(test, "scheme alloc fail");
+	KUNIT_EXPECT_EQ(test, s->quota.sz, (unsigned long)SZ_64K);
+	KUNIT_EXPECT_EQ(test, s->quota.esz, 0ul);
+	KUNIT_EXPECT_EQ(test, s->quota.esz_bp, 0ul);
+	KUNIT_EXPECT_EQ(test, s->quota.total_charged_sz, 0ul);
+	KUNIT_EXPECT_EQ(test, s->quota.total_charged_ns, 0ul);
+	KUNIT_EXPECT_EQ(test, s->quota.charged_sz, 0ul);
+	KUNIT_EXPECT_EQ(test, s->quota.charged_from, 0ul);
+	KUNIT_EXPECT_PTR_EQ(test, s->quota.charge_target_from, NULL);
+	KUNIT_EXPECT_EQ(test, s->quota.charge_addr_from, 0ul);
+	KUNIT_EXPECT_EQ(test, quota.esz, 123ul);
+	KUNIT_EXPECT_EQ(test, quota.esz_bp, 456ul);
+	KUNIT_EXPECT_EQ(test, quota.total_charged_sz, 789ul);
+	KUNIT_EXPECT_EQ(test, quota.total_charged_ns, 1011ul);
+	KUNIT_EXPECT_EQ(test, quota.charged_sz, 12ul);
+	KUNIT_EXPECT_EQ(test, quota.charged_from, 13ul);
+	KUNIT_EXPECT_PTR_EQ(test, quota.charge_target_from, &target);
+	KUNIT_EXPECT_EQ(test, quota.charge_addr_from, 14ul);
+	damon_destroy_scheme(s);
+}
+
 static void damos_test_commit_quota_goal_for(struct kunit *test,
 		struct damos_quota_goal *dst,
 		struct damos_quota_goal *src)
@@ -1572,6 +1614,69 @@ static void damon_test_commit_ctx(struct kunit *test)
 	damon_destroy_ctx(dst);
 }
 
+static void damon_test_commit_ctx_keeps_quota_for(struct kunit *test,
+		unsigned long min_region_sz, int expected_err)
+{
+	struct damos_access_pattern pattern = {};
+	struct damos_quota quota = {.sz = SZ_64K};
+	struct damos_watermarks wmarks = {};
+	struct damon_ctx *src, *dst;
+	struct damon_target *target;
+	struct damos *s;
+
+	dst = damon_new_ctx();
+	if (!dst)
+		kunit_skip(test, "dst alloc fail");
+	target = damon_new_target();
+	if (!target) {
+		damon_destroy_ctx(dst);
+		kunit_skip(test, "target alloc fail");
+	}
+	damon_add_target(dst, target);
+	s = damon_new_scheme(&pattern, DAMOS_STAT, 0, &quota, &wmarks,
+			NUMA_NO_NODE);
+	if (!s) {
+		damon_destroy_ctx(dst);
+		kunit_skip(test, "scheme alloc fail");
+	}
+	damon_add_scheme(dst, s);
+
+	/* Copy the parameters before populating dst's runtime quota state. */
+	src = damon_new_test_ctx(dst);
+	if (!src) {
+		damon_destroy_ctx(dst);
+		kunit_skip(test, "src alloc fail");
+	}
+	src->min_region_sz = min_region_sz;
+	s->quota.esz = 123;
+	s->quota.esz_bp = 456;
+	s->quota.total_charged_sz = 789;
+	s->quota.total_charged_ns = 1011;
+	s->quota.charged_sz = 12;
+	s->quota.charged_from = 13;
+	s->quota.charge_target_from = target;
+	s->quota.charge_addr_from = 14;
+
+	KUNIT_EXPECT_EQ(test, damon_commit_ctx(dst, src), expected_err);
+	KUNIT_EXPECT_EQ(test, s->quota.esz, 123ul);
+	KUNIT_EXPECT_EQ(test, s->quota.esz_bp, 456ul);
+	KUNIT_EXPECT_EQ(test, s->quota.total_charged_sz, 789ul);
+	KUNIT_EXPECT_EQ(test, s->quota.total_charged_ns, 1011ul);
+	KUNIT_EXPECT_EQ(test, s->quota.charged_sz, 12ul);
+	KUNIT_EXPECT_EQ(test, s->quota.charged_from, 13ul);
+	KUNIT_EXPECT_PTR_EQ(test, s->quota.charge_target_from, target);
+	KUNIT_EXPECT_EQ(test, s->quota.charge_addr_from, 14ul);
+	damon_destroy_ctx(src);
+	damon_destroy_ctx(dst);
+}
+
+static void damon_test_commit_ctx_keeps_quota(struct kunit *test)
+{
+	/* Only power of two min_region_sz is allowed. */
+	damon_test_commit_ctx_keeps_quota_for(test, 4096, 0);
+	damon_test_commit_ctx_keeps_quota_for(test, 4095, -EINVAL);
+}
+
 static void damon_test_valid_probe_params(struct kunit *test)
 {
 	struct damon_ctx *ctx;
@@ -1946,6 +2051,7 @@ static struct kunit_case damon_test_cases[] = {
 	KUNIT_CASE(damon_test_mvsum),
 	KUNIT_CASE(damon_test_nr_accesses_mvsum),
 	KUNIT_CASE(damos_test_new_filter),
+	KUNIT_CASE(damos_test_new_scheme_keeps_src_quota),
 	KUNIT_CASE(damos_test_commit_quota_goal),
 	KUNIT_CASE(damos_test_commit_quota_goals),
 	KUNIT_CASE(damos_test_commit_quota),
@@ -1957,6 +2063,7 @@ static struct kunit_case damon_test_cases[] = {
 	KUNIT_CASE(damon_test_commit_filter),
 	KUNIT_CASE(damon_test_commit_probes),
 	KUNIT_CASE(damon_test_commit_ctx),
+	KUNIT_CASE(damon_test_commit_ctx_keeps_quota),
 	KUNIT_CASE(damon_test_valid_probe_params),
 	KUNIT_CASE(damos_test_filter_out),
 	KUNIT_CASE(damon_test_feed_loop_next_input),
-- 
2.53.0

Re: [PATCH 1/2] mm/damon/core: preserve the caller's quota in damon_new_scheme()

[SJ Park]

On Mon, 21 Sep 2026 02:30:46 +0200 Karl Mehltretter <kmehltretter@gmail.com> wrote:

> damon_new_scheme() calls damos_quota_init() on the caller's quota before
> copying it to the new scheme. This clears the caller's effective quota,
> feedback input and charging state as a side effect.

Apparently the above paragraph assumes it is called under damon_commit_ctx().
Lack of the context makes this quite confusing.  Could you please rewrite?

> 
> damon_commit_ctx() first copies the running context into a temporary
> context for validating the proposed parameters. When
> damon_commit_schemes() creates the temporary schemes, it passes the quota
> of each running scheme to damon_new_scheme(). The quota pointer therefore
> refers to the running scheme, and damos_quota_init() clears that scheme's
> state before it is copied to the temporary scheme. Even an update
> rejected with -EINVAL loses the running quota state.
> 
> For a size quota, this discards the bytes already charged and allows the
> scheme to use a fresh quota before the reset interval has elapsed. For a
> goal-driven quota, the consist tuner loses its accumulated input and
> restarts from its minimum input. A time quota loses its throughput
> estimate and falls back to the initial estimate.
> 
> The constructor side effect was introduced by commit 70e0c1d1bf94
> ("mm/damon/core: factor out 'damos_quota' private fileds initialization").
> Commit 60bd24f272d0 ("mm/damon/sysfs: test commit input against realistic
> destination"), merged in v6.19, exposed it when
> validating sysfs updates against a copy of the running context. Commit
> b90408ef1163 ("mm/damon/core: safely validate src on damon_commit_ctx()")
> later moved that validation into the core API.
> 
> Sashiko reported the same side effect [1] on the RFC of the core API
> change.
> 
> Copy the quota to the new scheme first, then initialize that copy. Make
> damos_quota_init() return void, since its return value is no longer needed.
> 
> Fixes: 70e0c1d1bf94 ("mm/damon/core: factor out 'damos_quota' private fileds initialization")
> Cc: <stable@vger.kernel.org> # 6.19.x

The Fixes commit was introduced in 6.1.  So the comment on Cc: stable@ line
should be fixed.  Also, at the time of the commit, validation purpose running
ctx committing didn't exist.  So, the issue you are explaining cannot happen on
the commit.  Or, am I missing something?  If I'm not incorrect, could you
please find the proper Fixes: commit and fix it?

Also, are you using LLM for Fixes...?  If so, the LLM seems not good at that.
Your previous patch also made a similar mistake.  Please manually work on
Fixes: tag or double check LLM's output.

> Link: https://lore.kernel.org/damon/20260702212143.0CB6D1F00A3D@smtp.kernel.org/ [1]
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
>  mm/damon/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

The code change looks correct to me.


Thanks,
SJ

[...]

Re: [PATCH 1/2] mm/damon/core: preserve the caller's quota in damon_new_scheme()

[SJ Park]

On Mon, 21 Sep 2026 10:11:54 -0700 SJ Park <sj@kernel.org> wrote:

> On Mon, 21 Sep 2026 02:30:46 +0200 Karl Mehltretter <kmehltretter@gmail.com> wrote:
> 
> > damon_new_scheme() calls damos_quota_init() on the caller's quota before
> > copying it to the new scheme. This clears the caller's effective quota,
> > feedback input and charging state as a side effect.
> 
> Apparently the above paragraph assumes it is called under damon_commit_ctx().
> Lack of the context makes this quite confusing.  Could you please rewrite?

To calrify my opinion more, "caller's quota" feels unclear to me.  I hope it to
be more clear that it means "the quota that is passed as a parameter to the
function".

> 
> > 
> > damon_commit_ctx() first copies the running context into a temporary
> > context for validating the proposed parameters.

I'd prefer using the term, 'commit' instead of 'copies' for clarity.

> > When
> > damon_commit_schemes() creates the temporary schemes, it passes the quota
> > of each running scheme to damon_new_scheme(). The quota pointer therefore
> > refers to the running scheme, and damos_quota_init() clears that scheme's
> > state before it is copied to the temporary scheme. Even an update
> > rejected with -EINVAL loses the running quota state.
> > 
> > For a size quota, this discards the bytes already charged and allows the
> > scheme to use a fresh quota before the reset interval has elapsed. For a
> > goal-driven quota, the consist tuner loses its accumulated input and
> > restarts from its minimum input. A time quota loses its throughput
> > estimate and falls back to the initial estimate.
> > 
> > The constructor side effect was introduced by commit 70e0c1d1bf94
> > ("mm/damon/core: factor out 'damos_quota' private fileds initialization").
> > Commit 60bd24f272d0 ("mm/damon/sysfs: test commit input against realistic
> > destination"), merged in v6.19, exposed it when
> > validating sysfs updates against a copy of the running context. Commit
> > b90408ef1163 ("mm/damon/core: safely validate src on damon_commit_ctx()")
> > later moved that validation into the core API.

I overlooked this part in the previous reply, sorry.  And thank you for adding
this detailed context.

> > 
> > Sashiko reported the same side effect [1] on the RFC of the core API
> > change.

Nice catch, I misunderstood Sashiko's point.  Thank you for catching this,
Karl.

> > 
> > Copy the quota to the new scheme first, then initialize that copy. Make
> > damos_quota_init() return void, since its return value is no longer needed.
> > 
> > Fixes: 70e0c1d1bf94 ("mm/damon/core: factor out 'damos_quota' private fileds initialization")
> > Cc: <stable@vger.kernel.org> # 6.19.x
> 
> The Fixes commit was introduced in 6.1.  So the comment on Cc: stable@ line
> should be fixed.  Also, at the time of the commit, validation purpose running
> ctx committing didn't exist.  So, the issue you are explaining cannot happen on
> the commit.  Or, am I missing something?  If I'm not incorrect, could you
> please find the proper Fixes: commit and fix it?
> 
> Also, are you using LLM for Fixes...?  If so, the LLM seems not good at that.
> Your previous patch also made a similar mistake.  Please manually work on
> Fixes: tag or double check LLM's output.

Now I understand you added the comment for commit 60bd24f272d0.  I think Fixes:
should also be 60bd24f272d0.


Thanks,
SJ

[...]

Re: [PATCH 2/2] mm/damon/tests/core-kunit: test preservation of caller quota state

[SJ Park]

On Mon, 21 Sep 2026 02:30:47 +0200 Karl Mehltretter <kmehltretter@gmail.com> wrote:

> Check that damon_new_scheme() initializes the new scheme's quota without
> changing the caller's quota. Cover all eight fields initialized by
> damos_quota_init().
> 
> Also check that damon_commit_ctx() preserves those fields in the
> destination scheme for both accepted and rejected parameter updates.
> Use an invalid min_region_sz for the rejected update and confirm that
> returning -EINVAL leaves the running quota state unchanged.
> 
> Without the preceding fix, all eight fields are cleared in the
> constructor test and in both context update cases.

Looks good to me.

> 
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>

Reviewed-by: SJ Park <sj@kernel.org>


Thanks,
SJ

[...]