DAMON development mailing list
 help / color / mirror / Atom feed
* [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage
@ 2026-08-07  9:35 Liew Rui Yan
  2026-08-07  9:35 ` [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag Liew Rui Yan
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Liew Rui Yan @ 2026-08-07  9:35 UTC (permalink / raw)
  To: SJ Park; +Cc: damon, linux-mm, Liew Rui Yan

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag
  2026-08-07  9:35 [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage Liew Rui Yan
@ 2026-08-07  9:35 ` Liew Rui Yan
  2026-08-07  9:48   ` sashiko-bot
  2026-08-07 13:52   ` SJ Park
  2026-08-07  9:35 ` [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep Liew Rui Yan
  2026-08-07 14:12 ` [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage SJ Park
  2 siblings, 2 replies; 14+ messages in thread
From: Liew Rui Yan @ 2026-08-07  9:35 UTC (permalink / raw)
  To: SJ Park; +Cc: damon, linux-mm, Liew Rui Yan

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.

Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
 include/linux/damon.h |  2 ++
 mm/damon/core.c       | 12 ++++--------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 0c8b7ddef9ab..01e899a2150c 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -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.
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 644daf5a1656..7230483e771f 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep
  2026-08-07  9:35 [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage Liew Rui Yan
  2026-08-07  9:35 ` [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag Liew Rui Yan
@ 2026-08-07  9:35 ` Liew Rui Yan
  2026-08-07  9:47   ` sashiko-bot
  2026-08-07 14:07   ` SJ Park
  2026-08-07 14:12 ` [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage SJ Park
  2 siblings, 2 replies; 14+ messages in thread
From: Liew Rui Yan @ 2026-08-07  9:35 UTC (permalink / raw)
  To: SJ Park; +Cc: damon, linux-mm, Liew Rui Yan

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.

Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
 mm/damon/core.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 7230483e771f..57d1a21a8a0a 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep
  2026-08-07  9:35 ` [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep Liew Rui Yan
@ 2026-08-07  9:47   ` sashiko-bot
  2026-08-07 14:07   ` SJ Park
  1 sibling, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-08-07  9:47 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: damon

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kdamond_wait_activation() traps kdamond in an infinite sleep loop when no schemes are registered, completely breaking pure access monitoring.
--

--- Patch [2]: [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep ---
commit 7f4aa250c9a91aca4f1a7a02df1c14e1ab990d4e
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon: skip deactivated schemes in watermark checks and add fallback sleep

This commit modifies scheme evaluation to skip those deactivated either by
watermarks or max_nr_snapshots. It also changes kdamond_wait_activation()
to sleep for the sample interval as a fallback when all schemes are
deactivated.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -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);

[Severity: High]
Does this change cause a regression where kdamond becomes trapped in an
infinite sleep loop if no schemes are configured?

When a user starts DAMON for pure access monitoring without configuring any
schemes, the damon_for_each_scheme() loop is bypassed. This leaves
init_wait_time as false.

Because init_wait_time is false, the condition
if (!min_wait_time && init_wait_time) will fail. The function then falls
through to the new fallback logic:

    kdamond_usleep(ctx->attrs.sample_interval);

Since init_wait_time remains false on every iteration of the while loop,
kdamond_wait_activation() will never return 0. Could this permanently hang
the monitoring thread and prevent it from collecting any access metrics?

>  
>  		kdamond_call(ctx, false);
>  		if (ctx->maybe_corrupted)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807093526.183009-1-aethernet65535@gmail.com?part=2

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag
  2026-08-07  9:35 ` [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag Liew Rui Yan
@ 2026-08-07  9:48   ` sashiko-bot
  2026-08-07 13:52   ` SJ Park
  1 sibling, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-08-07  9:48 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: damon

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Logic error: `tried_applied` is set unconditionally, ignoring the access pattern guard check.
- [Medium] Uninitialized boolean `tried_applied` in `struct damos` leads to spurious `nr_snapshots` increment.
- [Low] Build failure due to removed function `damon_is_last_region()` still being referenced in KUnit tests.
--

--- Patch [1]: [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag ---
commit b2745ffce878c131daca165cb2a6674f9ded0bc7
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon: fix nr_snapshots counting using tried_applied flag

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.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a16560..7230483e771f7 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -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);
> -}
> -

[Severity: Low]
Does removing this function break the KUnit tests?

Looking at mm/damon/tests/core-kunit.h:damon_test_is_last_region(), it
still calls this function:

    KUNIT_EXPECT_TRUE(test, damon_is_last_region(r, t));

Since the test header is included at the end of core.c, compiling with
CONFIG_DAMON_KUNIT_TEST=y would result in an implicit declaration or
undefined symbol error.

[ ... ]

> @@ -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;
>  	}
>  }

[Severity: High]
Does this bypass the access pattern guard check?

The commit message states the scheme should pass all guard checks including
the access pattern before being considered tried. However, s->tried_applied
is set to true unconditionally here, even if damos_valid_target() returns
false. This would cause the max_nr_snapshots limit to be reached
prematurely.

[ ... ]

> @@ -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;

[Severity: Medium]
Could this result in a spurious increment of nr_snapshots due to
uninitialized memory?

When struct damos is allocated in mm/damon/core.c:damon_new_scheme() using:

    scheme = kmalloc_obj(*scheme);

the memory is not zero-initialized. During the first execution here, if a
scheme was skipped before damon_do_apply_schemes() explicitly sets the flag
(for example, due to zero valid targets or fully consumed quotas), the
uninitialized value might evaluate to true.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807093526.183009-1-aethernet65535@gmail.com?part=1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag
  2026-08-07  9:35 ` [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag Liew Rui Yan
  2026-08-07  9:48   ` sashiko-bot
@ 2026-08-07 13:52   ` SJ Park
  1 sibling, 0 replies; 14+ messages in thread
From: SJ Park @ 2026-08-07 13:52 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, damon, linux-mm

On Fri,  7 Aug 2026 17:35:25 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> 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.

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

[...]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep
  2026-08-07  9:35 ` [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep Liew Rui Yan
  2026-08-07  9:47   ` sashiko-bot
@ 2026-08-07 14:07   ` SJ Park
  2026-08-07 16:18     ` Liew Rui Yan
  2026-08-08 21:55     ` Liew Rui Yan
  1 sibling, 2 replies; 14+ messages in thread
From: SJ Park @ 2026-08-07 14:07 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, damon, linux-mm

Hello Liew,

On Fri,  7 Aug 2026 17:35:26 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> 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().

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

[...]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage
  2026-08-07  9:35 [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage Liew Rui Yan
  2026-08-07  9:35 ` [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag Liew Rui Yan
  2026-08-07  9:35 ` [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep Liew Rui Yan
@ 2026-08-07 14:12 ` SJ Park
  2026-08-07 16:19   ` Liew Rui Yan
  2 siblings, 1 reply; 14+ messages in thread
From: SJ Park @ 2026-08-07 14:12 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, damon, linux-mm

Hello Liew,

On Fri,  7 Aug 2026 17:35:24 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> 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.

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

[...]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep
  2026-08-07 14:07   ` SJ Park
@ 2026-08-07 16:18     ` Liew Rui Yan
  2026-08-08  0:07       ` SJ Park
  2026-08-08 21:55     ` Liew Rui Yan
  1 sibling, 1 reply; 14+ messages in thread
From: Liew Rui Yan @ 2026-08-07 16:18 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, damon, linux-mm

Hi SJ,

On Fri,  7 Aug 2026 07:07:39 -0700 SJ Park <sj@kernel.org> wrote:

> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage
  2026-08-07 14:12 ` [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage SJ Park
@ 2026-08-07 16:19   ` Liew Rui Yan
  2026-08-07 23:59     ` SJ Park
  0 siblings, 1 reply; 14+ messages in thread
From: Liew Rui Yan @ 2026-08-07 16:19 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, damon, linux-mm

Hi SJ,

On Fri,  7 Aug 2026 07:12:13 -0700 SJ Park <sj@kernel.org> wrote:

> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage
  2026-08-07 16:19   ` Liew Rui Yan
@ 2026-08-07 23:59     ` SJ Park
  0 siblings, 0 replies; 14+ messages in thread
From: SJ Park @ 2026-08-07 23:59 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, damon, linux-mm

On Sat,  8 Aug 2026 00:19:00 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SJ,
> 
> On Fri,  7 Aug 2026 07:12:13 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > 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.
> 
> I will update those documentations! Thank you again for your detailed
> explanations.

Thank you, looking forward to it!


Thanks,
SJ

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep
  2026-08-07 16:18     ` Liew Rui Yan
@ 2026-08-08  0:07       ` SJ Park
  0 siblings, 0 replies; 14+ messages in thread
From: SJ Park @ 2026-08-08  0:07 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, damon, linux-mm

On Sat,  8 Aug 2026 00:18:40 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SJ,
> 
> On Fri,  7 Aug 2026 07:07:39 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > 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.
> 
> 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.

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

[...]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep
  2026-08-07 14:07   ` SJ Park
  2026-08-07 16:18     ` Liew Rui Yan
@ 2026-08-08 21:55     ` Liew Rui Yan
  2026-08-08 22:50       ` SJ Park
  1 sibling, 1 reply; 14+ messages in thread
From: Liew Rui Yan @ 2026-08-08 21:55 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, damon, linux-mm

Hi SJ,

On Fri,  7 Aug 2026 07:07:39 -0700 SJ Park <sj@kernel.org> wrote:

> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep
  2026-08-08 21:55     ` Liew Rui Yan
@ 2026-08-08 22:50       ` SJ Park
  0 siblings, 0 replies; 14+ messages in thread
From: SJ Park @ 2026-08-08 22:50 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, damon, linux-mm

On Sun,  9 Aug 2026 05:55:54 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SJ,
> 
> On Fri,  7 Aug 2026 07:07:39 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > 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.
> 
> Wait, according to the documentation and the code, DAMON will still stop
> monitoring when all schemes is deactivated.

That's correct.

[...]
> Should this be changed to monitoring will not stop?

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

> 
> 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.

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.

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

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


Thanks,
SJ

[...]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2026-08-08 22:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-07  9:35 [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage Liew Rui Yan
2026-08-07  9:35 ` [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag Liew Rui Yan
2026-08-07  9:48   ` sashiko-bot
2026-08-07 13:52   ` SJ Park
2026-08-07  9:35 ` [PATCH 2/2] mm/damon: skip deactivated schemes in watermark checks and add fallback sleep Liew Rui Yan
2026-08-07  9:47   ` sashiko-bot
2026-08-07 14:07   ` SJ Park
2026-08-07 16:18     ` Liew Rui Yan
2026-08-08  0:07       ` SJ Park
2026-08-08 21:55     ` Liew Rui Yan
2026-08-08 22:50       ` SJ Park
2026-08-07 14:12 ` [RFC PATCH 0/2] mm/damon: fix nr_snapshots semantics and max_nr_snapshots check coverage SJ Park
2026-08-07 16:19   ` Liew Rui Yan
2026-08-07 23:59     ` SJ Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox