Re: [PATCH] mm/damon/core: remove call_control in inactive contexts

[JaeJoon Jung <rgbi3307@gmail.com>]

On Thu, 1 Jan 2026 at 10:41, SeongJae Park <sj@kernel.org> wrote:
>
> On Thu, 1 Jan 2026 09:55:56 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:
>
> > On Thu, 1 Jan 2026 at 00:26, SeongJae Park <sj@kernel.org> wrote:
> > >
> > > On Wed, 31 Dec 2025 14:27:54 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:
> > >
> > > > On Wed, 31 Dec 2025 at 10:25, SeongJae Park <sj@kernel.org> wrote:
> > > > >
> > > > > On Mon, 29 Dec 2025 19:45:14 -0800 SeongJae Park <sj@kernel.org> wrote:
> > > > >
> > > > > > On Mon, 29 Dec 2025 18:41:28 -0800 SeongJae Park <sj@kernel.org> wrote:
> > > > > >
> > > > > > > On Mon, 29 Dec 2025 17:45:30 -0800 SeongJae Park <sj@kernel.org> wrote:
> > > > > > >
> > > > > > > > On Sun, 28 Dec 2025 10:31:01 -0800 SeongJae Park <sj@kernel.org> wrote:
> > > > > > > >
> > > > > > [...]
> > > > > > > > I will send a new version of this fix soon.
> > > > > > >
> > > > > > > So far, I got two fixup ideas.
> > > > > > >
> > > > > > > The first one is keeping the current code as is, and additionally modifying
> > > > > > > kdamond_call() to protect all call_control object accesses under
> > > > > > > ctx->call_controls_lock protection.
> > > > > > >
> > > > > > > The second one is reverting this patch, and doing the DAMON running status
> > > > > > > check before adding the damon_call_control object, but releasing the
> > > > > > > kdamond_lock after the object insertion is done.
> > > > > > >
> > > > > > > I'm in favor of the second one at the moment, as it seems more simple.
> > > > > >
> > > > > > I don't really like both approaches because those implicitly add locking rules.
> > > > > > If the first approach is taken, damon_call() callers should aware they should
> > > > > > not register callback functions that can hold call_controls_lock.  If the
> > > > > > second approach is taken, we should avoid holding kdamond_lock while holding
> > > > > > damon_call_control lock.  The second implicit rule seems easier to keep to me,
> > > > > > but I want to avoid that if possible.
> > > > > >
> > > > > > The third idea I just got is, keeping this patch as is, and moving the final
> > > > > > kdamond_call() invocation to be made _before_ the ctx->kdamond reset.  That
> > > > > > removes the race condition between the final kdamond_call() and
> > > > > > damon_call_handle_inactive_ctx(), without introducing new locking rules.
> > > > >
> > > > > I just posted the v2 [1] with the third idea.
> > > > >
> > > > > [1] https://lore.kernel.org/20251231012315.75835-1-sj@kernel.org
> > > >
> > > > I generally agree with what you've said so far.  However, it's inefficient
> > > > to continue executing damon_call_handle_inactive_ctx() while kdamond is
> > > > "off".  There's no need to add a new damon_call_handle_inactive_ctx()
> > > > function.
> > >
> > > As I mentioned before on other threads with you, we care not only efficiency
> > > but also maintainability of the code.  The inefficiency you are saying about
> > > happens only in corner cases because damon_call() is not usually called while
> > > kdamond is off.  So the gain of making this efficient is not that big.
> >
> > The overhead isn't that high, but I think it's better to keep things
> > simple.
> > I think it's better to use list_add_tail() when kdamond is "on".
> >
> > >
> > > Meanwhile, to avoid this, as I mentioned on the previous reply to the first and
> > > the second idea of the fix, we need to add locking rule, which makes the code
> > > bit difficult to maintain.
> >
> > I think it's better to solve it with the existing kdamond_call(ctx,
> > cancel=true) rather than adding the damon_call_handle_inactive_ctx().
>
> On my previous reply, I was saying I think your suggested approach is not
> making it simple but only more complicated in the next paragraph.  And I was
> again further explaining why I think so.  More specifically, the added locking
> rule, that I previously explained with the third fix idea.
>
> Your above reply is saying it is "better", without any reason.  It even not
> addressing my concern that I explained more than once.  As a result, I again
> fail at finding why you keep repeating the argument.
>
> So, let me again ask you.  Please explain.

I've already answered your question.
Please understand my lack of expressiveness.

>
> >
> > >
> > > I therefore think the v2 is a good tradeoff.
> > >
> > > > As shown below, it's better to call list_add only when kdamond
> > > > is "on" (true), and then use the existing code to end with
> > > > kdamond_call(ctx, true) when kdamond is "off."
> > > >
> > > > +static void kdamond_call(struct damon_ctx *ctx, bool cancel);
> > > > +
> > > >  /**
> > > >   * damon_call() - Invoke a given function on DAMON worker thread (kdamond).
> > > >   * @ctx:       DAMON context to call the function for.
> > > > @@ -1496,14 +1475,17 @@ int damon_call(struct damon_ctx *ctx, struct
> > > > damon_call_control *control)
> > > >         control->canceled = false;
> > > >         INIT_LIST_HEAD(&control->list);
> > > >
> > > > -       if (damon_is_running(ctx)) {
> > > > -               mutex_lock(&ctx->call_controls_lock);
> > > > +       mutex_lock(&ctx->call_controls_lock);
> > > > +       if (ctx->kdamond) {
> > >
> > > This is wrong.  You shouldn't access ctx->kdamond without holding
> > > ctx->kdamond_lock.  Please read the comment about kdamond_lock field on damon.h
> > > file.
> >
> > That's right, I misjudged.
> > I've reorganized the code below.
>
> Your reply is not explaining how the reorganized code is solving it.  Let me
> ask you again.  Please explain.

I will explain this in the code I have summarized below.

>
> >
> > >
> > > >                 list_add_tail(&control->list, &ctx->call_controls);
> > > > -               mutex_unlock(&ctx->call_controls_lock);
> > > >         } else {
> > > > -               /* return damon_call_handle_inactive_ctx(ctx, control); */
> > > > +               mutex_unlock(&ctx->call_controls_lock);
> > > > +               if (!list_empty_careful(&ctx->call_controls))
> > > > +                       kdamond_call(ctx, true);
> > > >                 return -EINVAL;
> > > >         }
> > > > +       mutex_unlock(&ctx->call_controls_lock);
> > > > +
> > > >         if (control->repeat)
> > > >                 return 0;
> > > >         wait_for_completion(&control->completion);
> > >
> >
> > +static void kdamond_call(struct damon_ctx *ctx, bool cancel);
> > +
> >  /**
> >   * damon_call() - Invoke a given function on DAMON worker thread (kdamond).
> >   * @ctx:       DAMON context to call the function for.
> > @@ -1457,11 +1459,15 @@ 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 {
> > +               if (!list_empty_careful(&ctx->call_controls))
> > +                       kdamond_call(ctx, true);
> >                 return -EINVAL;
> > +       }
> >         if (control->repeat)
> >                 return 0;
> >         wait_for_completion(&control->completion);
>
> I think my previous concern that raised to your two previous approach [1,2] is
> again samely applied to the above diff.  And now you are suggesting a similar
> approach here.  Please explain and answer all questions before you move on, or
> explain why your new apprach is different from your previous one and how it
> addresses raised concerns..

The code above applies mutex and the locking method is the same.
The difference is that control->list does list_add_tail() in the "on" state in
damon_is_running(ctx).
Another difference is that when damon_is_running(ctx) is false("off"),
the existing call_controls list is cleared with kdamond_call(ctx, cancel=true).

Regarding spin_lock,
I'm sure you're familiar with spin_lock, It can apply like this:

spin_lock(&ctx->call_controls_lock);
list_add_tail(&control->list, &ctx->call_controls);
spin_unlock(&ctx->call_controls_lock);

I have expressed my opinion sufficiently,
and I believe you will make a better judgment than I.

Thanks,
JaeJoon

>
> [1] https://lore.kernel.org/20251225194959.937-1-sj@kernel.org
> [2] https://lore.kernel.org/20251226184111.254674-1-sj@kernel.org
> [3] https://lore.kernel.org/20251229151440.78818-1-sj@kernel.org
>
>
> Thanks,
> SJ
>
> [...]