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

[SeongJae Park <sj@kernel.org>]

On Thu, 14 May 2026 02:17:24 +0000 sashiko-bot@kernel.org wrote:

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

Correct.  The enabled_store() part porting was incomplete, similar to the
matching lru_sort patch.  I will post a complete porting.

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

I informed this and where the dependent patches can be found, on the commentary
area of the patch.

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


Thanks,
SJ