[PATCH] mm/damon/core: delete damon_target when detected invalid

[PATCH] mm/damon/core: delete damon_target when detected invalid

[Enze Li]

Currently, DAMON does not proactively clean up invalid monitoring
targets during its runtime.  When some monitored targets exit, DAMON
still makes the following unnecessary function calls,

  --damon_for_each_target--
  --damon_for_each_region--
      damon_do_apply_schemes
        damos_apply_scheme
          damon_va_apply_scheme
            damos_madvise
              damon_get_mm

and it is only in the damon_get_mm() that it may finally discover that
the monitoring target no longer exists.

To avoid wasting CPU resources, this patch modifies the
kdamond_need_stop() logic to proactively clean up monitoring targets
when they are found to be invalid.

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 mm/damon/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index f9fc0375890a..eb5612bfd6bf 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2462,7 +2462,8 @@ static void kdamond_split_regions(struct damon_ctx *ctx)
  */
 static bool kdamond_need_stop(struct damon_ctx *ctx)
 {
-	struct damon_target *t;
+	struct damon_target *t, *next;
+	bool valid_target_exist = false;
 
 	if (kthread_should_stop())
 		return true;
@@ -2470,11 +2471,16 @@ static bool kdamond_need_stop(struct damon_ctx *ctx)
 	if (!ctx->ops.target_valid)
 		return false;
 
-	damon_for_each_target(t, ctx) {
+	damon_for_each_target_safe(t, next, ctx) {
 		if (ctx->ops.target_valid(t))
-			return false;
+			valid_target_exist = true;
+		else
+			damon_destroy_target(t, ctx);
 	}
 
+	if (valid_target_exist)
+		return false;
+
 	return true;
 }
 
-- 
2.52.0

Re: [PATCH] mm/damon/core: delete damon_target when detected invalid

[SeongJae Park]

On Tue,  9 Dec 2025 13:57:13 +0800 Enze Li <lienze@kylinos.cn> wrote:

> Currently, DAMON does not proactively clean up invalid monitoring
> targets during its runtime.  When some monitored targets exit, DAMON
> still makes the following unnecessary function calls,
> 
>   --damon_for_each_target--
>   --damon_for_each_region--
>       damon_do_apply_schemes
>         damos_apply_scheme
>           damon_va_apply_scheme
>             damos_madvise
>               damon_get_mm
> 
> and it is only in the damon_get_mm() that it may finally discover that
> the monitoring target no longer exists.

Thank you for finding this inefficiency, Enze!

> 
> To avoid wasting CPU resources, this patch modifies the
> kdamond_need_stop() logic to proactively clean up monitoring targets
> when they are found to be invalid.

However, this could introduce an unexpected behavior change to DAMON sysfs
users.  Let's consider a DAMON context having three target processes (a, b, c)
is running.  The second process is terminated.  And the user updates the target
with two existing processes and a new process (a, d, c) using the online commit
feature.

Before this change, 'a' and 'c' targets keep running with the old monitoring
results.  The target for 'b' is replaced for 'd'.  After this change, the
target for 'b' is removed from the list, and 'c' becomes the second target of
the list.  Because the online commit feature works based on the indices of
targets on the list, target for 'c' is updated for 'b', lose previous
monitoring results.

Please let me know if I'm missing something.

> 
> Signed-off-by: Enze Li <lienze@kylinos.cn>
> ---
>  mm/damon/core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index f9fc0375890a..eb5612bfd6bf 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2462,7 +2462,8 @@ static void kdamond_split_regions(struct damon_ctx *ctx)
>   */
>  static bool kdamond_need_stop(struct damon_ctx *ctx)
>  {
> -	struct damon_target *t;
> +	struct damon_target *t, *next;
> +	bool valid_target_exist = false;
>  
>  	if (kthread_should_stop())
>  		return true;
> @@ -2470,11 +2471,16 @@ static bool kdamond_need_stop(struct damon_ctx *ctx)
>  	if (!ctx->ops.target_valid)
>  		return false;
>  
> -	damon_for_each_target(t, ctx) {
> +	damon_for_each_target_safe(t, next, ctx) {
>  		if (ctx->ops.target_valid(t))
> -			return false;
> +			valid_target_exist = true;
> +		else
> +			damon_destroy_target(t, ctx);
>  	}
>  
> +	if (valid_target_exist)
> +		return false;
> +
>  	return true;
>  }

So, from the beginning part of the patch description, I understand your concern
is the unnecessary function calls in kdamond_apply_schemes().  What about
checking the target validness using ctx->ops.target_valid() and avoid going
deeper if it is invalid?  Seems the damon_for_each_target() loop is a good
point to do this.

What do you think?


Thanks,
SJ

[...]

Re: [PATCH] mm/damon/core: delete damon_target when detected invalid

[Enze Li]

Hi SJ,

On Mon, Dec 08 2025 at 11:21:45 PM -0800, SeongJae Park wrote:

> On Tue,  9 Dec 2025 13:57:13 +0800 Enze Li <lienze@kylinos.cn> wrote:
>
>> Currently, DAMON does not proactively clean up invalid monitoring
>> targets during its runtime.  When some monitored targets exit, DAMON
>> still makes the following unnecessary function calls,
>> 
>>   --damon_for_each_target--
>>   --damon_for_each_region--
>>       damon_do_apply_schemes
>>         damos_apply_scheme
>>           damon_va_apply_scheme
>>             damos_madvise
>>               damon_get_mm
>> 
>> and it is only in the damon_get_mm() that it may finally discover that
>> the monitoring target no longer exists.
>
> Thank you for finding this inefficiency, Enze!
>
>> 
>> To avoid wasting CPU resources, this patch modifies the
>> kdamond_need_stop() logic to proactively clean up monitoring targets
>> when they are found to be invalid.
>
> However, this could introduce an unexpected behavior change to DAMON sysfs
> users.  Let's consider a DAMON context having three target processes (a, b, c)
> is running.  The second process is terminated.  And the user updates the target
> with two existing processes and a new process (a, d, c) using the online commit
> feature.
>
> Before this change, 'a' and 'c' targets keep running with the old monitoring
> results.  The target for 'b' is replaced for 'd'.  After this change, the
> target for 'b' is removed from the list, and 'c' becomes the second target of
> the list.  Because the online commit feature works based on the indices of
> targets on the list, target for 'c' is updated for 'b', lose previous
> monitoring results.

Ah, I see.  I hadn't thought of that situation.

>
> Please let me know if I'm missing something.
>
>> 
>> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> ---
>>  mm/damon/core.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>> 
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index f9fc0375890a..eb5612bfd6bf 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -2462,7 +2462,8 @@ static void kdamond_split_regions(struct damon_ctx *ctx)
>>   */
>>  static bool kdamond_need_stop(struct damon_ctx *ctx)
>>  {
>> -	struct damon_target *t;
>> +	struct damon_target *t, *next;
>> +	bool valid_target_exist = false;
>>  
>>  	if (kthread_should_stop())
>>  		return true;
>> @@ -2470,11 +2471,16 @@ static bool kdamond_need_stop(struct damon_ctx *ctx)
>>  	if (!ctx->ops.target_valid)
>>  		return false;
>>  
>> -	damon_for_each_target(t, ctx) {
>> +	damon_for_each_target_safe(t, next, ctx) {
>>  		if (ctx->ops.target_valid(t))
>> -			return false;
>> +			valid_target_exist = true;
>> +		else
>> +			damon_destroy_target(t, ctx);
>>  	}
>>  
>> +	if (valid_target_exist)
>> +		return false;
>> +
>>  	return true;
>>  }
>
> So, from the beginning part of the patch description, I understand your concern
> is the unnecessary function calls in kdamond_apply_schemes().  What about
> checking the target validness using ctx->ops.target_valid() and avoid going
> deeper if it is invalid?  Seems the damon_for_each_target() loop is a good
> point to do this.
>
> What do you think?

Excellent suggestion!  Moving the validity check to the
damon_for_each_target() loop via ctx->ops.target_valid() would indeed
prevent all unnecessary function calls, which is more efficient and
architecturally cleaner than my original approach. :)

I will implement this change shortly.

By the way, I'll add a "Suggested-by" tag to the commit message, if
that's okay with you.

Best Regards,
Enze

<...>

Re: [PATCH] mm/damon/core: delete damon_target when detected invalid

[SeongJae Park]

On Tue, 09 Dec 2025 17:24:49 +0800 Enze Li <lienze@kylinos.cn> wrote:

> Hi SJ,
> 
> On Mon, Dec 08 2025 at 11:21:45 PM -0800, SeongJae Park wrote:
> 
> > On Tue,  9 Dec 2025 13:57:13 +0800 Enze Li <lienze@kylinos.cn> wrote:
> >
> >> Currently, DAMON does not proactively clean up invalid monitoring
> >> targets during its runtime.  When some monitored targets exit, DAMON
> >> still makes the following unnecessary function calls,
> >> 
> >>   --damon_for_each_target--
> >>   --damon_for_each_region--
> >>       damon_do_apply_schemes
> >>         damos_apply_scheme
> >>           damon_va_apply_scheme
> >>             damos_madvise
> >>               damon_get_mm
> >> 
> >> and it is only in the damon_get_mm() that it may finally discover that
> >> the monitoring target no longer exists.
[...]
> > So, from the beginning part of the patch description, I understand your concern
> > is the unnecessary function calls in kdamond_apply_schemes().  What about
> > checking the target validness using ctx->ops.target_valid() and avoid going
> > deeper if it is invalid?  Seems the damon_for_each_target() loop is a good
> > point to do this.
> >
> > What do you think?
> 
> Excellent suggestion!  Moving the validity check to the
> damon_for_each_target() loop via ctx->ops.target_valid() would indeed
> prevent all unnecessary function calls, which is more efficient and
> architecturally cleaner than my original approach. :)
> 
> I will implement this change shortly.
> 
> By the way, I'll add a "Suggested-by" tag to the commit message, if
> that's okay with you.

Surely that's okay and my honor :)


Thanks,
SJ

[...]