Re: [PATCH 6.1.y] mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch fails to fix the state machine deadlock that prevents kdamond from restarting, leaving the bug described in the commit message unresolved.
- [Low] The patch introduces calls to undefined functions `damon_is_running()` and `damon_kdamond_pid()`, causing a build failure.
--

commit de09cefa1bba6b569eff518a49dbc5fb07b59452
Author: SeongJae Park <sj@kernel.org>

mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

This patch updates DAMON_RECLAIM to dynamically detect the status of the
enabled and kdamond_pid parameters. It aims to prevent users from seeing
stale status values and being unable to restart the kdamond thread when
it stops unintentionally.

Link: https://lore.kernel.org/20260419161003.79176-1-sj@kernel.org
Link: https://lore.kernel.org/20260419161003.79176-2-sj@kernel.org

>     For example, the issue can be reproduced via below steps.
[ ... ]
>     # # even after fixing the wrong parameter,
>     # # kdamond cannot be restarted.
>     # echo 1 > addr_unit
>     # echo Y > enabled
>     # ps -ef | grep kdamond
>     root         815     803  0 17:54 pts/4    00:00:00 grep kdamond

Does this patch actually resolve the restart issue described here? 

Looking at damon_reclaim_timer_fn(), the state machine still appears to
deadlock if kdamond stops unintentionally:

	now_enabled = enabled;
	if (last_enabled != now_enabled) {
		if (!damon_reclaim_turn(now_enabled))
			last_enabled = now_enabled;
		else
			enabled = last_enabled;
	}

When kdamond dies, the global enabled and static last_enabled variables
remain true.

If a user writes 'Y' to enabled, the timer sees last_enabled == now_enabled
(true == true) and exits without restarting kdamond.

If a user writes 'N' to enabled, the timer calls damon_reclaim_turn(false),
which invokes damon_stop(). Since the thread is already dead, damon_stop()
returns -EPERM. The timer misinterprets this as a failure to stop and
executes enabled = last_enabled, reverting enabled back to true. 

Could this leave the state machine permanently deadlocked?

> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 7952a0b7f409d..dc435aefd44f5 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c
[ ... ]
> @@ -220,6 +203,13 @@ static DECLARE_DELAYED_WORK(damon_reclaim_timer, damon_reclaim_timer_fn);
>  
>  static bool damon_reclaim_initialized;
>  
> +static bool damon_reclaim_enabled(void)
> +{
> +	if (!ctx)
> +		return false;
> +	return damon_is_running(ctx);

Are damon_is_running() and damon_kdamond_pid() defined anywhere in the tree?

They don't appear to be declared in include/linux/damon.h or defined in
mm/damon/core.c.

Could this patch depend on prerequisite patches that might be missing?
Without them, this causes implicit function declaration errors and
unresolved symbol linker errors.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513050501.216835-1-sj@kernel.org?part=1