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

[JaeJoon Jung <rgbi3307@gmail.com>]

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

>
> > For ease of explanation,
> > the data structure names will be abbreviated as follows:
> >
> > damon_call_control.list: C.list
> > ctx->call_controls: CTX.head
> > repeat_controls: R.head
> >
> > the execution flow of the while loop of the kdamond_call() function,
> >
> > Before modification:
> > Old while loop:
> >
> >         CTX.head <-----> C.list <-----> C.list <----> C.list
> >         ^                   |                           |
> >         |               if (C.repeat)            if (!C.repeat)
> >    restore: only one        |                           |
> >    list_add_tail()        list_del()                  list_del()
> >         |                   |                           |
> >         +                   |                       complete()
> >         R.head <------ list_add()
> >
> > To process C.repeat above, we use an additional list, repeat_controls.
>
> Your above abbreviation didn't explain what C.repeat is.  Maybe you mean
> 'damon_call_control.repeat'?

Yes, that's right.

>
> > The process of adding C.list to repeat_controls and then restoring it back
> > to CTX.head is complex and inefficient.
>
> I agree.
>
> > Furthermore, there's the problem
> > of restoring only a single C.list to CTX.head.
>
> I had to take some time on understanding what this mean.  And it seems you are
> working on an old version of the tree, and therefore saying about an issue that
> already fixed by commit 592e5c5f8ec6 ("mm/damon/core: fix memory leak of repeat
> mode damon_call_control objects").
>
> Please use mm-new as a baseline of DAMON patches, unless there are special
> reasons.  If there are special reasons, please explicitly specify.

This patch is based on v6.19-rc2.
I will continue to refer to mm-new and damon-new.

>
> >
> > Below, repeat_controls is removed and the existing CTX.head is modified to
> > loop once(1st rotation).  This simplifies list processing and creates a
> > more efficient structure.
> >
> > Modified while loop:
> > Not used repeat_controls:
> >
> >         CTX.head <-----> C.list <-----> C.list <----> C.list <-------+
> >                             |                           |            |
> >                         if (C.repeat)            if (!C.repeat)      |
> >                             |                           |            |
> >                           list_del()                  list_del()     |
> >                             |                           |            |
> >                             |                       complete()       |
> >                             |                                        |
> >                           first --------> list_add_tail() -----------+
> >
> >         if (C.list == first) break;
> >
> > Signed-off-by: JaeJoon Jung <rgbi3307@gmail.com>
> > ---
> >  mm/damon/core.c | 37 +++++++++++++++++++------------------
> >  1 file changed, 19 insertions(+), 18 deletions(-)
> >
> > 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.

>
> > -     control = list_first_entry_or_null(&repeat_controls,
> > -                     struct damon_call_control, list);
> > -     if (!control || cancel)
> > -             return;
> > -     mutex_lock(&ctx->call_controls_lock);
> > -     list_add_tail(&control->list, &ctx->call_controls);
> > -     mutex_unlock(&ctx->call_controls_lock);
>
> As I mentioned above, apparently you are using an old version of the tree that
> not having commit 592e5c5f8ec6 ("mm/damon/core: fix memory leak of repeat mode
> damon_call_control objects") that modified this part.  Please use mm-new as a
> baseline, or specify reasons why you cannot do so.

I'll look into what you said.
I'll also continue to look at the mm-new version.

Thanks,
JaeJoon

>
> >  }
> >
> >  /* Returns negative error code if it's not activated but should return */
> > --
> > 2.43.0
>
>
> Thanks,
> SJ