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

[JaeJoon Jung <rgbi3307@gmail.com>]

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

>
> >
> > >
> > > > 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.
>
> Thank you for confirming.
>
> >
> > >
> > > > 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.
>
> 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;
                }
        }
 }

Thanks,
JaeJoon

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