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

[Enze Li <lienze@kylinos.cn>]

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

<...>