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

[SeongJae Park <sj@kernel.org>]

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

[...]