mm/damon: introduce damos quota goal target metric complement flag

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: Integer underflow in DAMOS_QUOTA_SOME_MEM_PSI_US complement calculation --

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

mm/damon/core: introduce damos_quota_goal->complement

Introduce damos_quota_goal->complement for specifying whether to use a complemented value of the given goal target metric. Add the field to the data structure and implement essential core support. Handle the flag in the quota goal commit and current quota goal metric value retrieval.

Quoted 4 lines

mm/damon/core.c +0 -0

index 2258b72da7a78..9fd08cbfacd8c 100644

[ ... ]

Quoted 5 lines

mm/damon/core.c +1 -0

@@ -3238,6 +3239,23 @@ static void damos_set_quota_goal_current_value(struct damon_ctx *c,
 	default:
 		break;
 	}
+	if (!goal->complement)

mm/damon/core.c static void damos_set_quota_goal_current_value(struct damon_ctx *c, · line 3252

Quoted 10 lines

mm/damon/core.c +10 -0

+		return;
+
+	/* updte current_value to complemented value */
+
+	/* for user_input, users set complemented value on their own */
+	if (goal->metric == DAMOS_QUOTA_USER_INPUT)
+		return;
+	if (goal->metric == DAMOS_QUOTA_SOME_MEM_PSI_US) {
+		goal->current_value = s->quota.reset_interval * 1000 -
+			goal->current_value;

[Severity: High]
Does this unsigned subtraction underflow if goal->current_value exceeds s->quota.reset_interval * 1000?

If goal->current_value exceeds the nominal reset interval, which can happen on the first iteration when initialized to an arbitrarily large user-provided target_value, or on subsequent iterations if kernel scheduling delays cause the actual elapsed time between callbacks to exceed the nominal reset_interval, this subtraction will underflow and produce a massive current_value near ULONG_MAX.

This large value is then passed to damos_quota_score(), which computes a normalized score that maxes out the feedback loop, forcing the tuned quota limit to drop aggressively.

Quoted 9 lines

mm/damon/core.c +6 -0

+		return;
+	}
+	if (goal->current_value < 10000)
+		goal->current_value = 10000 - goal->current_value;
+	else
+		goal->current_value = 0;
 }
 

Quoted 1 line

mm/damon/core.c +0 -0

 /* Return the highest score since it makes schemes least aggressive */

Signature · 1 line

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] mm: damon: hardcoded false complement flag when cloning quota goals --

commit 9116ea6f94ac9ade4ef359ba8351ef835957bf80
Author: SJ Park <[email protected]>

mm/damon: add complement argument to damos_new_quota_goal()

This commit extends the damos_new_quota_goal() constructor to accept and set the 'complement' flag value. It also updates all API callers to use the new signature, which aims to prevent mistakes where the complement flag was previously set manually.

mm/damon/core.c int damos_commit_quota_goals(struct damos_quota *dst, struct damos_quota *src) · line 1267

Quoted 12 lines

mm/damon/core.c +2 -2

index 9fd08cbfacd8c..932ca3ad0e593 100644
@@ -1262,8 +1263,8 @@ int damos_commit_quota_goals(struct damos_quota *dst, struct damos_quota *src)
 	damos_for_each_quota_goal_safe(src_goal, next, src) {
 		if (j++ < i)
 			continue;
-		new_goal = damos_new_quota_goal(
-				src_goal->metric, src_goal->target_value);
+		new_goal = damos_new_quota_goal(src_goal->metric, false,
+				src_goal->target_value);

[Severity: Low]
Does this code discard the 'complement' flag from 'src_goal'?

By hardcoding 'false' here instead of passing 'src_goal->complement', the copied goal initially loses its complement state. While it appears damos_commit_quota_goal() immediately overwrites the flag with the correct value, passing 'false' defeats the purpose of the explicit constructor argument and could create a brittle pattern.

Quoted 4 lines

mm/damon/core.c +0 -0

 		if (!new_goal)
 			return -ENOMEM;
 		err = damos_commit_quota_goal(new_goal, src_goal);

[ ... ]

mm/damon/tests/core-kunit.h static void damos_test_commit_quota_goals_for(struct kunit *test, · line 933

Quoted 11 lines

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

index 5ff0436c58441..bba97ad449110 100644
@@ -929,7 +929,7 @@ static void damos_test_commit_quota_goals_for(struct kunit *test,
 		 * damos_commit_quota_goals() will kfree() the dst goals.
 		 * Make it kfree()-able.
 		 */
-		goal = damos_new_quota_goal(dst_goals[i].metric,
+		goal = damos_new_quota_goal(dst_goals[i].metric, false,
 				dst_goals[i].target_value);

[Severity: Low]
Does this discard the 'complement' flag from 'dst_goals[i]'?

Hardcoding 'false' instead of passing 'dst_goals[i].complement' might break future tests that attempt to verify complemented goals, as the destination goals in the test framework will lose their intended complement state.

Quoted 3 lines

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

 		if (!goal)
 			goto out;

Signature · 1 line

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/damon: integer underflow in DAMOS_QUOTA_SOME_MEM_PSI_US complement calculation --

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

mm/damon/sysfs-schemes: support quota goal complement flag

This patch adds a new sysfs file for the quota goal directory to allow setting and getting the quota goal metric complement flag value.

Quoted 4 lines

mm/damon/sysfs-schemes.c +0 -0

index 06af417bc9a2f..8f083611741fd 100644

[ ... ]

mm/damon/sysfs-schemes.c static int damos_sysfs_add_quota_score( · line 2905

Quoted 10 lines

mm/damon/sysfs-schemes.c +2 -1

@@ -2869,7 +2898,8 @@ static int damos_sysfs_add_quota_score(
 		if (!sysfs_goal->target_value)
 			continue;
 
-		goal = damos_new_quota_goal(sysfs_goal->metric, false,
+		goal = damos_new_quota_goal(sysfs_goal->metric,
+				sysfs_goal->complement,
 				sysfs_goal->target_value);
 		if (!goal)
 			return -ENOMEM;

[Severity: High]
Now that complement = true can be configured via sysfs, does this expose an integer underflow in damos_set_quota_goal_current_value()?

If a user configures a DAMOS quota goal using DAMOS_QUOTA_SOME_MEM_PSI_US with complement = true, the actual elapsed wall-clock time between evaluations can exceed the configured s->quota.reset_interval due to routine thread scheduling delays or high system load.

In mm/damon/core.c:damos_set_quota_goal_current_value(), the accumulated PSI time (goal->current_value) could then exceed reset_interval * 1000, underflowing the unsigned subtraction:

	if (goal->metric = DAMOS_QUOTA_SOME_MEM_PSI_US) {
		goal->current_value = s->quota.reset_interval * 1000 -
			goal->current_value;
		return;
	}

Unlike the basis-point metrics below it, this lacks a safety bounds check. The unsigned integer underflow results in an erroneously massive quota score.

This causes damon_feed_loop_next_input() to evaluate score >= goal * 2 and return the minimum possible input (min_input), severely throttling the quota size (esz) to a minimal value.

This effectively disables DAMOS scheme actions for that window, despite the system being under severe memory pressure, which is exactly when tuning is most important.

Signature · 2 lines

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: integer underflow in damos_set_quota_goal_current_value()
--

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

mm/damon/core: introduce damos_quota_goal->complement

Introduce damos_quota_goal->complement for specifying whether to use a complemented value of the given goal target metric. Add the field to the data structure and implement essential core support.

Quoted 4 lines

mm/damon/core.c +0 -0

index 2258b72da7a78..9fd08cbfacd8c 100644

[ ... ]

Quoted 5 lines

mm/damon/core.c +1 -0

@@ -3238,6 +3239,23 @@ static void damos_set_quota_goal_current_value(struct damon_ctx *c,
 	default:
 		break;
 	}
+	if (!goal->complement)

mm/damon/core.c static void damos_set_quota_goal_current_value(struct damon_ctx *c, · line 3252

Quoted 10 lines

mm/damon/core.c +10 -0

+		return;
+
+	/* updte current_value to complemented value */
+
+	/* for user_input, users set complemented value on their own */
+	if (goal->metric = DAMOS_QUOTA_USER_INPUT)
+		return;
+	if (goal->metric = DAMOS_QUOTA_SOME_MEM_PSI_US) {
+		goal->current_value = s->quota.reset_interval * 1000 -
+			goal->current_value;

[Severity: High]
Does this unprotected subtraction in damos_set_quota_goal_current_value() risk an integer underflow?

If a user configures DAMOS_QUOTA_SOME_MEM_PSI_US with complement enabled via sysfs and sets a target_value greater than reset_interval * 1000, the initial round (where current_value = target_value) will immediately underflow.

Furthermore, if the kdamond thread is delayed by the scheduler beyond the reset interval, the measured PSI time can naturally exceed reset_interval * 1000, which would also trigger the underflow.

This underflow would result in a huge current_value (near ULONG_MAX), which is then fed into damos_quota_score(), producing an astronomically high tuning score that aggressively drops the effective quota to zero, breaking the quota regulation mechanism.

Quoted 8 lines

mm/damon/core.c +6 -0

+		return;
+	}
+	if (goal->current_value < 10000)
+		goal->current_value = 10000 - goal->current_value;
+	else
+		goal->current_value = 0;
 }

Signature · 1 line

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