mm/damon: fix zero quota for PSI goals under the temporal tuner

A newly created or committed PSI quota goal has no previous sample. Commit a68878f83ae6 ("mm/damon/core: handle uninitialized damos_quota_goal->last_psi_total") scores the first tuning round as achieved to preserve the consist tuner's quota, since the feedback loop returns its input unchanged at that score.

The temporal tuner sets the quota to zero for an achieved goal. This skips the first charge window and the window after each goal commit. Committing the PSI goal before every tuning round keeps the scheme idle indefinitely.

Score an unmeasured PSI goal as not achieved for the temporal tuner, so a new scheme can run in its first window. Keep the consist tuner's behaviour unchanged.

Fixes a68878f83ae6 ("mm/damon/core: handle uninitialized damos_quota_goal->last_psi_total")

Cc <[email protected]> # 7.1.x

Assisted-by LLM

Signed-off-by Karl Mehltretter <[email protected]>

---
 mm/damon/core.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

mm/damon/core.c +0 -3

index 06cf2ab7e97d..e6d87fd3992e 100644
@@ -3161,12 +3161,18 @@ static void damos_set_quota_goal_current_value(struct damon_ctx *c,
 		break;
 	case DAMOS_QUOTA_SOME_MEM_PSI_US:
 		now_psi_total = damos_get_some_mem_psi_total();
-		/* uninitialized last_psi_total; make no effect this round */
-		if (goal->last_psi_total == U64_MAX)
-			goal->current_value = goal->target_value;

The intention is to make no effect this round. Setting current_value to zero to temporal tuner means it will now have highest quota it could have. If the effective quota before this was zero, it gets an effect.

Ideally, we should somehow remember what was the last esz and keep it. I have no good idea for doing that with minimum change. This is a corner case in my opinion (correct me if I'm wrong) so I want to keep the change as simple as possible.

If there is not easy way to do that, I think just keeping the behavior but making it explicitly explained might be better.

Quoted 10 lines · SJ Park

The intention is to make no effect this round. Setting current_value to zero to temporal tuner means it will now have highest quota it could have. If the effective quota before this was zero, it gets an effect.

Ideally, we should somehow remember what was the last esz and keep it. I have no good idea for doing that with minimum change. This is a corner case in my opinion (correct me if I'm wrong) so I want to keep the change as simple as possible.

Quoted 3 lines

If there is not easy way to do that, I think just keeping the behavior but making it explicitly explained might be better.

Hello SJ,

I believe I have a version that addresses your concern without adding too much complexity.

In short, it leaves the tuners as they are and instead keeps last_psi_total across a commit of an existing PSI goal, as the code did before a68878f83ae6. The U64_MAX round then happens only once per new goal.

So I'd still prefer to just making no effect this round. And my commit was failed at doing that, because it didn't aware of temporal tuner. Maybe we could show whether the goal was achieved or not, using esz and let the tuner show same achieveness? What about something like below?

Quoted 5 lines · SJ Park

So I'd still prefer to just making no effect this round. And my commit was failed at doing that, because it didn't aware of temporal tuner. Maybe we could show whether the goal was achieved or not, using esz and let the tuner show same achieveness? What about something like below?

Thanks for explaining the two concerns. I am still new to the DAMON code, so I may be missing something, but your suggestion looks good to me and it is simpler than what I had. I have prepared a v2 this way and will wait for your comments before I send it.

One note on the first concern: as far as I can see last_psi_total is updated in every tuning round, so commits in between would not make the measured time longer than the reset interval. But I am fine with your approach either way.

I would also move the last_psi_total update into the new helper, so that the helper is the only place that touches the field and the unit test can check it.

While testing this with damo I found a second problem. A full "damo tune" still gave a zero quota for one window, on the unpatched kernel too. damon_new_scheme() runs damos_quota_init() on the quota of the caller, and since b90408ef1163 damon_commit_ctx() passes the quota of each running scheme to it for the test context. So every full commit zeroes esz, esz_bp and the charge state of the running schemes. Please correct me if I read this wrong. I can send a separate patch for that.

In a quick QEMU test with your suggestion and that change, the quota stays at 1 MiB over "damo tune" and "damo tune --quota_goals_only".

And the apply interval (or, next_apply_sis) is extended for each commit, in damon_set_attrs(). Hence, the problem can still happen.

Quoted 3 lines

full commit zeroes esz, esz_bp and the charge state of the running schemes. Please correct me if I read this wrong. I can send a separate patch for that.

I don't really understand the problem. Could you please further elaborate?

Exercise a PSI quota goal under the temporal tuner on its first round, its next measured round, and after a goal commit. Use ULONG_MAX as the target so the goal is not reached during the test. Then check that a measured round which does reach the target sets the quota to zero, and that the consist tuner keeps its quota over an unmeasured round.

Without the previous patch, the first and the post-commit checks see a zero quota.

Assisted-by LLM

Signed-off-by Karl Mehltretter <[email protected]>

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

mm/damon/tests/core-kunit.h +31 -0

index c01e6a75cadc..2b9b24ce7dad 100644
@@ -889,6 +889,74 @@ static void damos_test_commit_quota_goal(struct kunit *test)
                        });
 }
 
+/*
+ * Unmeasured PSI goals must not disable the temporal quota.
+ * Keep sz * 10000 within a 32-bit unsigned long.
+ */
+static void damos_test_set_effective_quota_temporal_psi(struct kunit *test)
+{
+       struct damon_ctx *c = damon_new_ctx();
+       struct damos_access_pattern pattern = {};
+       struct damos_quota quota = {
+               .sz = SZ_64K,
+               .goal_tuner = DAMOS_QUOTA_GOAL_TUNER_TEMPORAL,
+       };
+       struct damos_watermarks wmarks = {};
+       struct damos_quota_goal src = {
+               .metric = DAMOS_QUOTA_SOME_MEM_PSI_US,
+               .target_value = ULONG_MAX,
+       };
+       struct damos_quota_goal *goal;
+       struct damos *s;
+
+       if (!c)
+               kunit_skip(test, "ctx alloc fail");
+       s = damon_new_scheme(&pattern, DAMOS_STAT, 0, &quota, &wmarks,
+                       NUMA_NO_NODE);
+       if (!s) {
+               damon_destroy_ctx(c);
+               kunit_skip(test, "scheme alloc fail");
+       }
+       damon_add_scheme(c, s);
+       goal = damos_new_quota_goal(DAMOS_QUOTA_SOME_MEM_PSI_US, ULONG_MAX);
+       if (!goal) {

mm/damon/tests/core-kunit.h +28 -1

+       damos_set_effective_quota(c, s);
+       KUNIT_EXPECT_EQ(test, s->quota.esz, (unsigned long)SZ_64K);
+
+       /* commit a PSI goal onto the initialised PSI goal */
+       damos_commit_quota_goal(goal, &src);
+       damos_set_effective_quota(c, s);
+       KUNIT_EXPECT_EQ(test, s->quota.esz, (unsigned long)SZ_64K);
+
+       /* a measured round that reaches the target disables the quota */
+       goal->target_value = 10;
+       goal->last_psi_total = damos_get_some_mem_psi_total() - 10;
+       /* U64_MAX marks an unmeasured goal, keep the sample away from it */
+       if (goal->last_psi_total == U64_MAX)
+               goal->last_psi_total--;
+       damos_set_effective_quota(c, s);
+       KUNIT_EXPECT_EQ(test, s->quota.esz, 0ul);
+
+       /* the consist tuner keeps its quota over an unmeasured round */
+       s->quota.goal_tuner = DAMOS_QUOTA_GOAL_TUNER_CONSIST;
+       s->quota.esz_bp = SZ_32K * 10000;
+       damos_commit_quota_goal(goal, &src);
+       damos_set_effective_quota(c, s);
+       KUNIT_EXPECT_EQ(test, s->quota.esz, (unsigned long)SZ_32K);
+
+       damon_destroy_ctx(c);
+}
+
 static void damos_test_commit_quota_goals_for(struct kunit *test,
                struct damos_quota_goal *dst_goals, int nr_dst_goals,
                struct damos_quota_goal *src_goals, int nr_src_goals)

@@ -1887,6 +1955,7 @@ static struct kunit_case damon_test_cases[] = {
        KUNIT_CASE(damos_test_new_filter),
        KUNIT_CASE(damos_test_commit_quota_goal),
        KUNIT_CASE(damos_test_commit_quota_goals),
+       KUNIT_CASE(damos_test_set_effective_quota_temporal_psi),
        KUNIT_CASE(damos_test_commit_quota),
        KUNIT_CASE(damos_test_commit_dests),
        KUNIT_CASE(damos_test_commit_filter),
--

2.53.0

Could we also check goal->last_psi_total here?

The first tuning round should consume the U64_MAX sentinel and establish the measured state.

Testing this explicitly would lock in that state transition.

Exercise a PSI quota goal under the temporal tuner on its first round, its next measured round, and after a goal commit. Use ULONG_MAX as the target so the goal is not reached during the test. Then check that a measured round which does reach the target sets the quota to zero, and that the consist tuner keeps its quota over an unmeasured round.

Without the previous patch, the first and the post-commit checks see a zero quota.

Assisted-by LLM

Signed-off-by Karl Mehltretter <[email protected]>

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

mm/damon/tests/core-kunit.h +0 -0

index c01e6a75cadc..2b9b24ce7dad 100644

So, this test depends on the memory pressure level of the testing system? I concern if that could make the test result not very reliable. Maybe we could split out the corner case handling logic to explicitly receive the current memory PSI total time. For example, like what I suggested in the reply to the first patch of this series. With it, we could build a kunit test for only the target logic, without depending on the test system's status.

Quoted 7 lines · SJ Park

So, this test depends on the memory pressure level of the testing system? I concern if that could make the test result not very reliable. Maybe we could split out the corner case handling logic to explicitly receive the current memory PSI total time. For example, like what I suggested in the reply to the first patch of this series. With it, we could build a kunit test for only the target logic, without depending on the test system's status.

Yes, the second round reads the real PSI total. I used ULONG_MAX as the target to make it unreachable, but I agree that a test which does not read system state is better.

For v2 I can test only the helper from your reply to patch 1, with explicit PSI totals. It would cover both tuners, zero and non-zero esz for the temporal one, and the last_psi_total value after each call as Kunwu asked.