Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call()

[JaeJoon Jung <rgbi3307@gmail.com>]

On Sat, 27 Dec 2025 at 03:31, SeongJae Park <sj@kernel.org> wrote:
>
> On Fri, 26 Dec 2025 11:19:28 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:
>
> > On Fri, 26 Dec 2025 at 05:01, SeongJae Park <sj@kernel.org> wrote:
> > >
> > > On Thu, 25 Dec 2025 12:10:30 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:
> > >
> > > > On Thu, 25 Dec 2025 at 10:07, SeongJae Park <sj@kernel.org> wrote:
> > > > >
> > > > > On Wed, 24 Dec 2025 21:43:54 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote:
> > > > >
> > > > > > The kdamond_call() function is executed repeatedly in the kdamond_fn()
> > > > > > kernel thread.  The kdamond_call() function is implemented as a while loop.
> > > > > > Therefore, it is important to improve the list processing logic here to
> > > > > > ensure faster execution of control->fn().
> > > > >
> > > > > That depends on how critical the performance is, and how much complexity the
> > > > > optimization introduces.  I have no idea about if the performance of
> > > > > kdamond_call() is really important.  If you have a realistic use case that
> > > > > shows it, sharing it would be nice.
> > > >
> > > > This is because kdamond_call() is called repeatedly in kdamond_fn().
> > >
> > > Yes, it is repeatedly called.  But, my question is, does it impose overhead
> > > that great enough to make a negative impact to the real world.
> >
> > I agree that the overhead is not that much since there are only a few lists
> > added to ctx->call_controls(CTX.head).
> [...]
> > > > > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > > > > index 824aa8f22db3..babad37719b6 100644
> > > > > > --- a/mm/damon/core.c
> > > > > > +++ b/mm/damon/core.c
> > > > > > @@ -2554,42 +2554,43 @@ static void kdamond_usleep(unsigned long usecs)
> > > > > >   */
> > > > > >  static void kdamond_call(struct damon_ctx *ctx, bool cancel)
> > > > > >  {
> > > > > > -     struct damon_call_control *control;
> > > > > > -     LIST_HEAD(repeat_controls);
> > > > > > -     int ret = 0;
> > > > > > +     struct damon_call_control *control, *first = NULL;
> > > > > > +     unsigned int idx = 0;
> > > > > >
> > > > > >       while (true) {
> > > > > >               mutex_lock(&ctx->call_controls_lock);
> > > > > >               control = list_first_entry_or_null(&ctx->call_controls,
> > > > > >                               struct damon_call_control, list);
> > > > > >               mutex_unlock(&ctx->call_controls_lock);
> > > > > > -             if (!control)
> > > > > > +
> > > > > > +             /* check control empty or 1st rotation */
> > > > > > +             if (!control || control == first)
> > > > > >                       break;
> > > > > > -             if (cancel) {
> > > > > > +
> > > > > > +             if (++idx == 1)
> > > > > > +                     first = control;
> > > > > > +
> > > > > > +             if (cancel)
> > > > > >                       control->canceled = true;
> > > > > > -             } else {
> > > > > > -                     ret = control->fn(control->data);
> > > > > > -                     control->return_code = ret;
> > > > > > -             }
> > > > > > +             else
> > > > > > +                     control->return_code = control->fn(control->data);
> > > > > > +
> > > > > >               mutex_lock(&ctx->call_controls_lock);
> > > > > >               list_del(&control->list);
> > > > > >               mutex_unlock(&ctx->call_controls_lock);
> > > > > > +
> > > > > >               if (!control->repeat) {
> > > > > > +                     /* run control->fn() one time */
> > > > > >                       complete(&control->completion);
> > > > > >               } else if (control->canceled && control->dealloc_on_cancel) {
> > > > > >                       kfree(control);
> > > > > > -                     continue;
> > > > > >               } else {
> > > > > > -                     list_add(&control->list, &repeat_controls);
> > > > > > +                     /* to repeat next time */
> > > > > > +                     mutex_lock(&ctx->call_controls_lock);
> > > > > > +                     list_add_tail(&control->list, &ctx->call_controls);
> > > > > > +                     mutex_unlock(&ctx->call_controls_lock);
> > > > > >               }
> > > > > >       }
> > > > >
> > > > > Let's suppose there are two damon_call_control objects on the
> > > > > ctx->call_controls.  The first one has ->repeat unset, while the second one
> > > > > has.  Then, it seems the 'break' condition will never met and therefore this
> > > > > loop will never finished.  Am I missing something?
> > > >
> > > > You misjudged.
> > > > If (!C.repeat), it will be removed with list_del() and disappear.
> > > > If (C.repeat) loops through the loop once, and when it returns to the
> > > > first, it breaks.
> > >
> > > Maybe my explanation was not enough.  Let me explain a bit in more detail.
> > >
> > > In the scenario I mentioned, at the first iteration of the loop, 'first' will
> > > be the first control object, which has ->repeat unset.  The object will be
> > > removed from the list.  In the second iteration of the loop, it handles the
> > > second object, which has ->repeat set.  The object is added to the list again.
> > > In the third iteration, the loop runs for the second object again.  Because it
> > > is not same to 'first', the 'break' statement is not reached.  The loop
> > > continues forever.
> > >
> > > Am I missing something?
> >
> > Thank you for your detailed review.
> > There may be cases where C->repeat=false is the first control.
> > This can also be solved simply as follows:
> >
> > @@ -2567,9 +2599,6 @@ static void kdamond_call(struct damon_ctx *ctx,
> > bool cancel)
> >                 if (!control || control == first)
> >                         break;
> >
> > -               if (++idx == 1)
> > -                       first = control;
> > -
> >                 if (cancel)
> >                         control->canceled = true;
> >                 else
> > @@ -2589,6 +2618,8 @@ static void kdamond_call(struct damon_ctx *ctx,
> > bool cancel)
> >                         mutex_lock(&ctx->call_controls_lock);
> >                         list_add_tail(&control->list, &ctx->call_controls);
> >                         mutex_unlock(&ctx->call_controls_lock);
> > +                       if (++idx == 1)
> > +                               first = control;
> >                 }
> >         }
> >  }
>
> Yes, that should fix the issue.
>
> And it seems 'idx' is being used for only this?  If I'm not wrong, I think it
> may be easier to read if you do something like 'first = first ? first :
> control' and drop 'idx'.

It's better removing idx.
Thanks.
JaeJoon

@@ -2587,7 +2587,6 @@ static void kdamond_usleep(unsigned long usecs)
 static void kdamond_call(struct damon_ctx *ctx, bool cancel)
 {
        struct damon_call_control *control, *first = NULL;
-       unsigned int idx = 0;

        while (true) {
                mutex_lock(&ctx->call_controls_lock);
@@ -2618,8 +2617,8 @@ static void kdamond_call(struct damon_ctx *ctx,
bool cancel)
                        mutex_lock(&ctx->call_controls_lock);
                        list_add_tail(&control->list,
&ctx->call_controls);
                        mutex_unlock(&ctx->call_controls_lock);
-                       if (++idx == 1)
-                               first = control;
+
+                       first = (first) ? first : control;
                }
        }
 }

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