Re: [RFC PATCH] mm/damon: reset thread status parameters upon kdamond termination

[SeongJae Park <sj@kernel.org>]

On Wed,  1 Apr 2026 00:09:56 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SeongJae,
> 
> On Tue, 31 Mar 2026 14:58:36 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> 
> > [...]
> > Option 1: Introduce a generic termination callback
> > ==================================================
> > Add 'void *after_terminate_fn(void*)' and 'void *after_terminate_data'
> > to 'struct damon_ctx' or 'struct damon_operations'. While this extends
> > the Core API, it provides a clean notification mechanism. When kdamond
> > exits for any reason, the module can perform its own cleanup (e.g.,
> > resetting 'enabled' and 'kdamond_pid') within its own callback. This
> > keeps the core logic decoupled from module parameters.

Maybe this is the right long term direction.  But to my understanding the fix
should be backported to stable kernels.  Is that correct?  If so, I'd prefer
simple quick fix that can easily backported.

> > 
> > Option 2: On-demand state correction in the module
> > ==================================================
> > In damon_{lru_sort, reclaim}_enabled_store(), if damon_stop() fails, we
> > check is_kdamond_running(). If the kdamond is found to be terminated, we
> > forcibly reset 'enabled' and 'kdamond_pid'.

I think this can work, and simple.

> > 
> > My perspective
> > --------------
> > I personally prefer OPTION-1 because it ensures the sysfs state in
> > synchronized actively.
> > 
> > OPTION-2 is simpler and avoids API changes, but it's a "passive" fix
> > that only triggers when user atttempts a write operation. User might
> > still see inconsistent value until they try to interact with the module.

I agree your concern.

> > [...]
> 
> To avoid over-complicating the Core API (Option 1 or current approach),
> I've implemented a lightweight, on-demand synchronization mechanism in
> the module layer.
> 
> By overriding the '.get' operator of the 'enabled' parameter, we can
> detect if kdamond has terminated and reset the module-level state right
> before the user reads them.
> 
> Since sysfs parameter access is protected by 'kernel_param_lock', this
> approach is race-free and keeps the DAMON core decoupling intact.
> 
> Core Implementation:
> 
> 	if (enabled && !damon_is_running(ctx)) {
> 		enabled = false;
> 		kdamond_pid = -1;
> 	}
> 
> 	return param_get_bool(buffer, kp);
> 
> Test Log:
> 
>     # cd /sys/module/damon_lru_sort/parameters/
>     # echo Y > enabled 
>     # echo 3 > addr_unit 
>     # echo Y > commit_inputs 
>     # cat enabled 
>     N
>     # cat kdamond_pid 
>     -1
> 
> I think this approach is better than my previous OPTION-1 and OPTION-2.
> Does this looks good to you?

Looks not bad.  But, what if we read kdamond_pid before reading enabled?
Also, what if the user simply try writing N to enabled?  Wouldn't user still
see the partial-broken status?  So this doesn't seem gretly superior to the
option 2.

Based on your above reproducer, I understand this issue happens by the
damon_commit_ctx() failure.  If it is not wrong, can't we catch the
damon_commit_ctx() failure from the calling place and make appropriate updates?


Thanks,
SJ

[...]