Re: [PATCH] mm/damon/sysfs: preventing duplicated list_add_tail() at the damon_call()

[SeongJae Park <sj@kernel.org>]

On Fri, 26 Dec 2025 10:48:31 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:

> On Fri, 26 Dec 2025 at 04:50, SeongJae Park <sj@kernel.org> wrote:
> >
> > On Thu, 25 Dec 2025 11:35:33 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:
> >
> > > On Thu, 25 Dec 2025 at 09:32, SeongJae Park <sj@kernel.org> wrote:
> > > >
> > > > Hello JaeJoon,
> > > >
> > > > On Wed, 24 Dec 2025 18:43:58 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:
[...]
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index babad37719b6..2ead0bb3c462 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > > @@ -1462,6 +1462,9 @@ bool damon_is_running(struct damon_ctx *ctx)
> > >   */
> > >  int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
> > >  {
> > > +       if (!damon_is_running(ctx))
> > > +               return -EINVAL;
> > > +
> > >         if (!control->repeat)
> > >                 init_completion(&control->completion);
> > >         control->canceled = false;
> > > @@ -1470,8 +1473,6 @@ int damon_call(struct damon_ctx *ctx, struct
> > > damon_call_control *control)
> > >         mutex_lock(&ctx->call_controls_lock);
> > >         list_add_tail(&control->list, &ctx->call_controls);
> > >         mutex_unlock(&ctx->call_controls_lock);
> > > -       if (!damon_is_running(ctx))
> > > -               return -EINVAL;
> > >         if (control->repeat)
> > >                 return 0;
> > >         wait_for_completion(&control->completion);
> >
> > Let's assume DAMON is terminated between the damon_is_running() and
> > list_add_tail().  In the case, the control->fn() will never be called back.  If
> > control->repeat is false, this function will even inifnitely wait.
> 
> As you said, there are cases where kdamond is terminated(stopped) in
> damon_is_running() and list_add_tail().  It may be a very rare case, but
> I missed this case.
> 
> >
> > I think we should keep the damon_is_running() as is, but further check if it
> > was terminated without handling the control object, and remove it from the list
> > in the case.  Like below.
[...]
> However, the damon_call_handle_inactive_ctx() function is to post-process
> the duplicate addition of control->list.  Rather, it is more efficient to
> prevent duplicate additions in advance, as follows:
> I have tested the following and it works fine.
> 
> @@ -1467,11 +1496,14 @@ int damon_call(struct damon_ctx *ctx, struct
> damon_call_control *control)
>         control->canceled = false;
>         INIT_LIST_HEAD(&control->list);
> 
> -       mutex_lock(&ctx->call_controls_lock);
> -       list_add_tail(&control->list, &ctx->call_controls);
> -       mutex_unlock(&ctx->call_controls_lock);
> -       if (!damon_is_running(ctx))
> +       if (damon_is_running(ctx)) {
> +               mutex_lock(&ctx->call_controls_lock);
> +               list_add_tail(&control->list, &ctx->call_controls);
> +               mutex_unlock(&ctx->call_controls_lock);
> +       } else {
> +               /* return damon_call_handle_inactive_ctx(ctx, control); */
>                 return -EINVAL;
> +       }
>         if (control->repeat)
>                 return 0;
>         wait_for_completion(&control->completion);

I think this is not differnt from your previous suggestion, and thus it has the
same issue.  What if DAMON is terminated between damon_is_running() and
list_add_tail() call?  Please let me know if I'm missing something.

> 
> > If you don't mind, I'll post the above diff as a patch, adding a 'Reported-by:'
> > tag for you.
> 
> 'Reported-by:' is OK.  However, please check the above again.

Thank you!


Thanks,
SJ

[...]