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

[SeongJae Park <sj@kernel.org>]

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


Thanks,
SJ

[...]