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

[JaeJoon Jung]

On Thu, 1 Jan 2026 at 00:26, SeongJae Park <sj@xxxxxxxxxx> wrote:
>
> On Wed, 31 Dec 2025 14:27:54 +0900 JaeJoon Jung <rgbi3307@xxxxxxxxx> wrote:
>
> > On Wed, 31 Dec 2025 at 10:25, SeongJae Park <sj@xxxxxxxxxx> wrote:
> > >
> > > On Mon, 29 Dec 2025 19:45:14 -0800 SeongJae Park <sj@xxxxxxxxxx> wrote:
> > >
> > > > On Mon, 29 Dec 2025 18:41:28 -0800 SeongJae Park <sj@xxxxxxxxxx> wrote:
> > > >
> > > > > On Mon, 29 Dec 2025 17:45:30 -0800 SeongJae Park <sj@xxxxxxxxxx> wrote:
> > > > >
> > > > > > On Sun, 28 Dec 2025 10:31:01 -0800 SeongJae Park <sj@xxxxxxxxxx> 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@xxxxxxxxxx
> >
> > 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().

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

>
> >                 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);


Thanks,
JaeJoon

>
> Thanks,
> SJ
>
> [...]