mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage

Both patches address the DAMOS nr_snapshots/max_nr_snapshots deactivation mechanism.

- Patch 1: fixes the nr_snapshots counting semantics.
- Patch 2: unifies deactivation checks to prevent busy-loop.

Note: Patch 1 should be applied before Patch 2, as damos_is_deactivated() relies on correct nr_snapshots counting.

Liew Rui Yan (2):
mm/damon: fix nr_snapshots counting using tried_applied flag mm/damon: skip deactivated schemes in watermark checks and add
fallback sleep

 include/linux/damon.h |  2 ++
 mm/damon/core.c       | 42 ++++++++++++++++++++++++++----------------
 2 files changed, 28 insertions(+), 16 deletions(-)

2.55.0

Currently, nr_snapshots is only incremented when damon_is_last_region() returns true. This is semantically incorrect because nr_snapshots should count the number of DAMON snapshots (aggregation intervals) where the scheme was tried to be applied, as documented in design.rst.

The "tried to be applied" means the scheme passed all guard checks including access pattern, quotas, watermarks, and max_nr_snapshots filters. The old damon_is_last_region() check does not accurately reflect this semantic.

To fix this, add a tried_applied flag to 'struct damos' that is set whenever a scheme passes all guard checks and is considered for application in damon_do_apply_schemes(). Then, increment nr_snapshots based on this flag in kdamond_apply_schemes() after the walk completes. This ensures nr_snapshots accurately reflects the number of snapshots where the scheme was actually tried.

---
 include/linux/damon.h |  2 ++
 mm/damon/core.c       | 12 ++++--------
 2 files changed, 6 insertions(+), 8 deletions(-)

index 0c8b7ddef9ab..01e899a2150c 100644
@@ -573,6 +573,8 @@ struct damos {
 	unsigned long next_apply_sis;
 	/* informs if ongoing DAMOS walk for this scheme is finished */
 	bool walk_completed;
+	/* informs if damos is tried applied in this phase */
+	bool tried_applied;
 	/*
 	 * If the current region in the filtering stage is allowed by core
 	 * layer-handled filters.  If true, operations layer allows it, too.

index 644daf5a1656..7230483e771f 100644
@@ -398,12 +398,6 @@ static void damon_destroy_region(struct damon_region *r,
 	damon_free_region(r);
 }
 
-static bool damon_is_last_region(struct damon_region *r,
-		struct damon_target *t)
-{
-	return list_is_last(&r->list, &t->regions_list);
-}
-
 /**
  * damon_probe_hits_wsum() - Returns probe hits weighted sum of a region.
  * @r:		region to get the weighted sum of.

@@ -2668,8 +2662,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (damos_valid_target(c, r, s))
 			damos_apply_scheme(c, t, r, s);
 
-		if (damon_is_last_region(r, t))
-			s->stat.nr_snapshots++;
+		s->tried_applied = true;
 	}
 }
 

@@ -3249,6 +3242,9 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 	damon_for_each_scheme(s, c) {
 		if (time_before(c->passed_sample_intervals, s->next_apply_sis))
 			continue;
+		if (s->tried_applied)
+			s->stat.nr_snapshots++;
+		s->tried_applied = false;
 		damos_walk_complete(c, s);
 		damos_set_next_apply_sis(s, c);
 		s->last_applied = NULL;

2.55.0

I don't think the current implementation is wrong. Maybe the documentation is better to be updated to "...that the scheme has __completely__ tried". Let me know if I'm missing something, or this implementaion has some practical problems.

Thanks,
SJ

[...]

According to DAMOS design documentation, a scheme is deactivated when nr_snapshots reaches max_nr_snapshots. However, the previous kdamond_wait_activation() still checked the schemes which nr_snapshots reached max_nr_snapshots.

This caused an issue - when all schemes were deactivated due to max_nr_snapshots, but their watermarks were still satisfied, damos_wmark_wait_us() would return 0, causing kdamond_wait_activation() to return 0 (activated). The main loop would then continue without sleeping, leading to unnecessary overhead since all schemes would be skipped in damon_do_apply_schemes().

To fix this:
- Add a damos_is_deactivated() helper that checks both wmarks.activated and max_nr_snapshots conditions.
- Replace the wmarks.activated-only checks with damos_is_deactivated() in both damon_do_apply_schemes() and kdamond_apply_schemes().
- In kdamond_wait_activation(), skip deactivated schemes when calculating the minimum wait time.
- When no active schemes remain, sleep for sample_interval as a fallback (consistent with pause behavior) instead of returning 0.

---
 mm/damon/core.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

index 7230483e771f..57d1a21a8a0a 100644
@@ -2633,6 +2633,17 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 	damos_update_stat(s, sz, sz_applied, sz_ops_filter_passed);
 }
 
+static bool damos_is_deactivated(struct damos *s)
+{
+	if (!s->wmarks.activated)
+		return true;
+	if (s->max_nr_snapshots &&
+			s->max_nr_snapshots <= s->stat.nr_snapshots)
+		return true;
+
+	return false;
+}
+
 static void damon_do_apply_schemes(struct damon_ctx *c,
 				   struct damon_target *t,
 				   struct damon_region *r)

@@ -2645,7 +2656,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (time_before(c->passed_sample_intervals, s->next_apply_sis))
 			continue;
 
-		if (!s->wmarks.activated)
+		if (damos_is_deactivated(s))
 			continue;
 
 		/* Check the quota */

@@ -2655,10 +2666,6 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (damos_skip_charged_region(t, r, s, c->min_region_sz))
 			continue;
 
-		if (s->max_nr_snapshots &&
-				s->max_nr_snapshots <= s->stat.nr_snapshots)
-			continue;
-
 		if (damos_valid_target(c, r, s))
 			damos_apply_scheme(c, t, r, s);
 

@@ -3220,7 +3227,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 		if (time_before(c->passed_sample_intervals, s->next_apply_sis))
 			continue;
 
-		if (!s->wmarks.activated)
+		if (damos_is_deactivated(s))
 			continue;
 
 		has_schemes_to_apply = true;

@@ -3704,16 +3711,23 @@ static int kdamond_wait_activation(struct damon_ctx *ctx)
 
 	while (!kdamond_need_stop(ctx)) {
 		damon_for_each_scheme(s, ctx) {
+			if (s->max_nr_snapshots &&
+				s->max_nr_snapshots <= s->stat.nr_snapshots)
+				continue;
+
 			wait_time = damos_wmark_wait_us(s);
 			if (!init_wait_time || wait_time < min_wait_time) {
 				init_wait_time = true;
 				min_wait_time = wait_time;
 			}
 		}
-		if (!min_wait_time)
+		if (!min_wait_time && init_wait_time)
 			return 0;
 
-		kdamond_usleep(min_wait_time);
+		if (min_wait_time)
+			kdamond_usleep(min_wait_time);
+		else
+			kdamond_usleep(ctx->attrs.sample_interval);
 
 		kdamond_call(ctx, false);
 		if (ctx->maybe_corrupted)

2.55.0

Hello Liew,

This is an intended implementation.

When all schemes are deactivated by watermarks, DAMON stops monitoring. It was implemented in the way because we didn't want DAMON consumes system resource in the case. But, later it turned out DAMON's resource consumption is really negligible. Rather, it is turned out that it makes DAMON runs with stale history when it is activated again. Particularly, regions have 'age' and their start/end addresses that was emerged before the deactivation. Those are meaningless and could even cause wrong DAMOS decisions.

We don't want that anymore. For a case the user really want DAMON completely stops, we introduced 'pause'. For max_nr_snapshot-based deactivation, we don't intend to completely stop DAMON.

Maybe the documentation can be updated to further clarify this.

Thanks,
SJ

[...]

Hi SJ,

Thank you for your detailed explanation! I also glad to hear that DAMON's monitoring's resource consumption is negligible, since I plan to add a contexts/<N>/always_monitoring parameter later on, allowing users to decide for themselves whether to keep monitoring even all schemes is deactivated by watermarks.

Best regards,
Rui Yan

Unless it is for your real use case, I'd not encourage improving watermarks.

Here are contexts. Watermarks was initially developed for DAMON_RECLAIM. But I found it is difficult to be extended for other use cases. Also it has the aobve mentioned problem: losing the monitoring results that was converged over time.

I'm planning to develop an alternative superior feature and deprecate watermarks over time. The plan is not having a high priority because I don't show real requirement of that. If anyone wants it for their real use case, please let me know so that I can prioritize.

Thanks,
SJ

[...]

Hi SJ,

Wait, according to the documentation and the code, DAMON will still stop monitoring when all schemes is deactivated.

    static int kdamond_wait_activation(struct damon_ctx *ctx)
    {
        /* ... */
        while (!kdamond_need_stop(ctx)) {
            damon_for_each_scheme(s, ctx) {
                wait_time = damos_wmark_wait_us(s);
                if (!init_wait_time || wait_time < min_wait_time) {
                    init_wait_time = true;
                    min_wait_time = wait_time;
                }
            }
            if (!min_wait_time)
                return 0;

            kdamond_usleep(min_wait_time);
            /* ... */
        }
        return -EBUSY;
    }

    DAMOS allows users to offload such works using three watermarks.  It
    allows the users to configure the metric of their interest, and
    three watermark values, namely high, middle, and low.  If the value
    of the metric becomes above the high watermark or below the low
    watermark, the scheme is deactivated.  If the metric becomes below
    the mid watermark but above the low watermark, the scheme is
    activated.  __If all schemes are deactivated by the watermarks, the
    monitoring is also deactivated__.  In this case, the DAMON worker
    thread only periodically checks the watermarks and therefore incurs
    nearly zero overhead.

Should this be changed to monitoring will not stop?

Also this patch (0002) add a new function named damos_is_deactivated(), it checks both wmarks.activated and max_nr_snapshot conditions, and I replace the wmarks.activated-only with this new function.

I am sorry if my commit message has caused you any misunderstanding.

Best regards,
Rui Yan

That's correct.

[...]

No. The change you mentioning will be unexpected behavioral change.

Let's distinguish watermarks based deactivation and max_nr_snapshots based deactivation. They are completely different things and have different behaviors. The current documentation may confused you.

No worry. The current poor documentation is the one to blame in my opinion.

Thanks,
SJ

[...]

Hello Liew,

Thank you for sharing this patch series. However, I don't think the patches are fixing real problems. Rather, it seems the poor documentation has confused you. Sorry for writing the documentation in the confusing way. I commented to each patch for details. If you want, please feel free to send documentation update patches. I will also try to find a time to do that if nobody does.

Thanks,
SJ

[...]

Hi SJ,

I will update those documentations! Thank you again for your detailed explanations.

Thank you, looking forward to it!

Thanks,
SJ