Re: (sashiko review) [RFC PATCH 1/2] mm/damon/stat: add a parameter for reading kdamond pid

[SeongJae Park]

On Mon, 13 Apr 2026 22:37:41 -0700 SeongJae Park <sj@xxxxxxxxxx> wrote:

> TL; DR: Sashiko found a bug of this patch; I will fix it in the next revision.
> Sashiko also found one issue in the existing code; I will work on it
> separately.

No, I was wrong.

> 
> Forwarding full sashiko review in a reply format with my inline comments below,
> for sharing details of my view and doing followup discussions via mails if
> needed.
> 
> > # review url: https://sashiko.dev/#/patchset/20260414045002.88110-2-sj@xxxxxxxxxx
> > # start of sashiko.dev inline review
> > commit fc6c9ec12df0df57fcce5c90c7a0604cddb1cc78
> > Author: SeongJae Park <sj@xxxxxxxxxx>
> > 
> > mm/damon/stat: add a parameter for reading kdamond pid
> > 
> > This commit exposes the PID of the DAMON_STAT kdamond via a new read-only
> > module parameter, kdamond_pid. It intends to set this to -1 when DAMON_STAT
> > is disabled.
> > 
> > > diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> > > index 99ba346f9e325..21fa04d95eeac 100644
> > > --- a/mm/damon/stat.c
> > > +++ b/mm/damon/stat.c
> > [ ... ]
> > > @@ -260,6 +269,9 @@ static int damon_stat_start(void)
> > >  		damon_stat_context = NULL;
> > >  		return err;
> > >  	}
> > > +	kdamond_pid = damon_kdamond_pid(damon_stat_context);
> > > +	if (kdamond_pid < 0)
> > > +		return kdamond_pid;
> > 
> > If damon_kdamond_pid() returns an error code, doesn't this leave the global
> > kdamond_pid set to that negative error code instead of -1?
> > 
> > Also, if we take this early return path, does this code leak the
> > damon_stat_context allocation? The error path just above it correctly
> > calls damon_destroy_ctx(damon_stat_context) and sets it to NULL.
> 
> Nice catch.  I will fix this like below in the next revision.

The kdamond_pid stale value question is indeed a nice catch.  But the
damon_stat_context allocation leak question is not.  It will be cleaned up when
damon_stat_start() is called again, by the cleanup logic at the beginning of
this function.  This is not a bug but an intended behavior.

[...]
> 
> '''
> --- a/mm/damon/stat.c
> +++ b/mm/damon/stat.c
> @@ -223,8 +223,12 @@ static int damon_stat_start(void)
>                 return err;
>         }
>         kdamond_pid = damon_kdamond_pid(damon_stat_context);
> -       if (kdamond_pid < 0)
> -               return kdamond_pid;
> +       if (kdamond_pid < 0) {
> +               err = kdamond_pid;
> +               kdamond_pid = -1;
> +               damon_destroy_ctx(damon_stat_context);
> +               damon_stat_context = NULL;
> +               return err;
> 
>         damon_stat_last_refresh_jiffies = jiffies;
>         call_control.data = damon_stat_context;
> '''
> 
> > 
> > >  
> > >  	damon_stat_last_refresh_jiffies = jiffies;
> > >  	call_control.data = damon_stat_context;
> > >  	return damon_call(damon_stat_context, &call_control);
> > >  }
> > 
> > If damon_call() fails here and returns an error, the caller will handle it
> > by setting enabled to false. Does this leave kdamond_pid holding a stale
> > positive PID instead of being reset to -1?
> 
> Nice catch, but not this patch's fault.  I will separately work on it.

I'm wrong here, too.  This is this patch's fault.  But, there is actually a bug
that pre-existed before this patch.  I was somehow confused.  That is, the
'enabled' value can also be stale, and it can make DAMON_STAT cannot be
restarted before the system is reboot.

I will hold this patch and fix the bug first.

Refer to my comment [1] on the next version of this patch, for more detail.

[1] https://lore.kernel.org/20260416040602.88665-1-sj@xxxxxxxxxx


Thanks,
SJ

[...]