* [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call()
@ 2025-12-24 12:43 JaeJoon Jung
2025-12-25 1:07 ` SeongJae Park
0 siblings, 1 reply; 12+ messages in thread
From: JaeJoon Jung @ 2025-12-24 12:43 UTC (permalink / raw)
To: SeongJae Park; +Cc: JaeJoon Jung, damon, linux-mm, rgbi3307
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(). 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.
The process of adding C.list to repeat_controls and then restoring it back
to CTX.head is complex and inefficient. Furthermore, there's the problem
of restoring only a single C.list to CTX.head.
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);
}
}
- 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);
}
/* Returns negative error code if it's not activated but should return */
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-24 12:43 [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() JaeJoon Jung @ 2025-12-25 1:07 ` SeongJae Park 2025-12-25 3:10 ` JaeJoon Jung 0 siblings, 1 reply; 12+ messages in thread From: SeongJae Park @ 2025-12-25 1:07 UTC (permalink / raw) To: JaeJoon Jung; +Cc: SeongJae Park, damon, linux-mm, rgbi3307 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. > 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'? > 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. > > 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? > - 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. > } > > /* Returns negative error code if it's not activated but should return */ > -- > 2.43.0 Thanks, SJ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-25 1:07 ` SeongJae Park @ 2025-12-25 3:10 ` JaeJoon Jung 2025-12-25 20:00 ` SeongJae Park 0 siblings, 1 reply; 12+ messages in thread From: JaeJoon Jung @ 2025-12-25 3:10 UTC (permalink / raw) To: SeongJae Park; +Cc: damon, linux-mm, rgbi3307 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-25 3:10 ` JaeJoon Jung @ 2025-12-25 20:00 ` SeongJae Park 2025-12-26 2:19 ` JaeJoon Jung 0 siblings, 1 reply; 12+ messages in thread From: SeongJae Park @ 2025-12-25 20:00 UTC (permalink / raw) To: JaeJoon Jung; +Cc: SeongJae Park, damon, linux-mm, rgbi3307 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. > > > > > > 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? Thanks, SJ [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-25 20:00 ` SeongJae Park @ 2025-12-26 2:19 ` JaeJoon Jung 2025-12-26 18:31 ` SeongJae Park 0 siblings, 1 reply; 12+ messages in thread From: JaeJoon Jung @ 2025-12-26 2:19 UTC (permalink / raw) To: SeongJae Park; +Cc: damon, linux-mm, rgbi3307 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 > > [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-26 2:19 ` JaeJoon Jung @ 2025-12-26 18:31 ` SeongJae Park 2025-12-26 23:42 ` JaeJoon Jung 0 siblings, 1 reply; 12+ messages in thread From: SeongJae Park @ 2025-12-26 18:31 UTC (permalink / raw) To: JaeJoon Jung; +Cc: SeongJae Park, damon, linux-mm, rgbi3307 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 [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-26 18:31 ` SeongJae Park @ 2025-12-26 23:42 ` JaeJoon Jung 2025-12-30 0:14 ` JaeJoon Jung 0 siblings, 1 reply; 12+ messages in thread From: JaeJoon Jung @ 2025-12-26 23:42 UTC (permalink / raw) To: SeongJae Park; +Cc: damon, linux-mm, rgbi3307 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 > > [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-26 23:42 ` JaeJoon Jung @ 2025-12-30 0:14 ` JaeJoon Jung 2025-12-30 0:57 ` SeongJae Park 0 siblings, 1 reply; 12+ messages in thread From: JaeJoon Jung @ 2025-12-30 0:14 UTC (permalink / raw) To: SeongJae Park; +Cc: damon, linux-mm, rgbi3307 I will reflect the above in patch v2. Would you like me to resend patch v2 ? On Sat, 27 Dec 2025 at 08:42, JaeJoon Jung <rgbi3307@gmail.com> wrote: > > 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 > > > > [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-30 0:14 ` JaeJoon Jung @ 2025-12-30 0:57 ` SeongJae Park 2025-12-31 1:28 ` SeongJae Park 0 siblings, 1 reply; 12+ messages in thread From: SeongJae Park @ 2025-12-30 0:57 UTC (permalink / raw) To: JaeJoon Jung; +Cc: SeongJae Park, damon, linux-mm, rgbi3307 On Tue, 30 Dec 2025 09:14:37 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote: > I will reflect the above in patch v2. > Would you like me to resend patch v2 ? I just found my fix [1] of the bug that you reported has a race condition. And to fix that, I think kdamond_call() will anyway need to be modified. I will shortly send more details as a reply to the fix [1]. Can you please hold this refactoring until all discussions about the bug fix is done? Sorry for your inconvenience. [1] https://lore.kernel.org/20251228183105.289441-1-sj@kernel.org Thanks, SJ [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-30 0:57 ` SeongJae Park @ 2025-12-31 1:28 ` SeongJae Park 2025-12-31 6:23 ` JaeJoon Jung 0 siblings, 1 reply; 12+ messages in thread From: SeongJae Park @ 2025-12-31 1:28 UTC (permalink / raw) To: SeongJae Park; +Cc: JaeJoon Jung, damon, linux-mm, rgbi3307 On Mon, 29 Dec 2025 16:57:15 -0800 SeongJae Park <sj@kernel.org> wrote: > On Tue, 30 Dec 2025 09:14:37 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote: > > > I will reflect the above in patch v2. > > Would you like me to resend patch v2 ? > > I just found my fix [1] of the bug that you reported has a race condition. And > to fix that, I think kdamond_call() will anyway need to be modified. I will > shortly send more details as a reply to the fix [1]. Can you please hold this > refactoring until all discussions about the bug fix is done? Sorry for your > inconvenience. > > [1] https://lore.kernel.org/20251228183105.289441-1-sj@kernel.org I ended up sending the new version of the fix [1] without changes on kdamond_call(). Unless you have concerns on the fix, please feel free to proceed to v2 of this patch. Thank you for patiently waiting. [1] https://lore.kernel.org/20251231012315.75835-1-sj@kernel.org Thanks, SJ [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-31 1:28 ` SeongJae Park @ 2025-12-31 6:23 ` JaeJoon Jung 2025-12-31 15:29 ` SeongJae Park 0 siblings, 1 reply; 12+ messages in thread From: JaeJoon Jung @ 2025-12-31 6:23 UTC (permalink / raw) To: SeongJae Park; +Cc: damon, linux-mm, rgbi3307 On Wed, 31 Dec 2025 at 10:28, SeongJae Park <sj@kernel.org> wrote: > > On Mon, 29 Dec 2025 16:57:15 -0800 SeongJae Park <sj@kernel.org> wrote: > > > On Tue, 30 Dec 2025 09:14:37 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote: > > > > > I will reflect the above in patch v2. > > > Would you like me to resend patch v2 ? > > > > I just found my fix [1] of the bug that you reported has a race condition. And > > to fix that, I think kdamond_call() will anyway need to be modified. I will > > shortly send more details as a reply to the fix [1]. Can you please hold this > > refactoring until all discussions about the bug fix is done? Sorry for your > > inconvenience. > > > > [1] https://lore.kernel.org/20251228183105.289441-1-sj@kernel.org > > I ended up sending the new version of the fix [1] without changes on > kdamond_call(). Unless you have concerns on the fix, please feel free to > proceed to v2 of this patch. Thank you for patiently waiting. > > [1] https://lore.kernel.org/20251231012315.75835-1-sj@kernel.org My patch for the kdamond_call() function further improves control->list to behave as a FIFO. This also improves stability against the control->list collision issue encountered when damon_call() is called. If there are no further objections, I'll send you Patch v2. Thanks, JaeJoon > > > Thanks, > SJ > > [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() 2025-12-31 6:23 ` JaeJoon Jung @ 2025-12-31 15:29 ` SeongJae Park 0 siblings, 0 replies; 12+ messages in thread From: SeongJae Park @ 2025-12-31 15:29 UTC (permalink / raw) To: JaeJoon Jung; +Cc: SeongJae Park, damon, linux-mm, rgbi3307 On Wed, 31 Dec 2025 15:23:49 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote: > On Wed, 31 Dec 2025 at 10:28, SeongJae Park <sj@kernel.org> wrote: > > > > On Mon, 29 Dec 2025 16:57:15 -0800 SeongJae Park <sj@kernel.org> wrote: > > > > > On Tue, 30 Dec 2025 09:14:37 +0900 JaeJoon Jung <rgbi3307@gmail.com> wrote: > > > > > > > I will reflect the above in patch v2. > > > > Would you like me to resend patch v2 ? > > > > > > I just found my fix [1] of the bug that you reported has a race condition. And > > > to fix that, I think kdamond_call() will anyway need to be modified. I will > > > shortly send more details as a reply to the fix [1]. Can you please hold this > > > refactoring until all discussions about the bug fix is done? Sorry for your > > > inconvenience. > > > > > > [1] https://lore.kernel.org/20251228183105.289441-1-sj@kernel.org > > > > I ended up sending the new version of the fix [1] without changes on > > kdamond_call(). Unless you have concerns on the fix, please feel free to > > proceed to v2 of this patch. Thank you for patiently waiting. > > > > [1] https://lore.kernel.org/20251231012315.75835-1-sj@kernel.org > > My patch for the kdamond_call() function further improves control->list > to behave as a FIFO. This also improves stability against the control->list > collision issue encountered when damon_call() is called. > If there are no further objections, I'll send you Patch v2. If you want to say something about damon_call() uaf issue, let's make a conclusion about it first, rather than mixing topics. Thanks, SJ [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-12-31 15:29 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-12-24 12:43 [PATCH] mm/damon/core: modified control->repeat loop at the kdamond_call() JaeJoon Jung 2025-12-25 1:07 ` SeongJae Park 2025-12-25 3:10 ` JaeJoon Jung 2025-12-25 20:00 ` SeongJae Park 2025-12-26 2:19 ` JaeJoon Jung 2025-12-26 18:31 ` SeongJae Park 2025-12-26 23:42 ` JaeJoon Jung 2025-12-30 0:14 ` JaeJoon Jung 2025-12-30 0:57 ` SeongJae Park 2025-12-31 1:28 ` SeongJae Park 2025-12-31 6:23 ` JaeJoon Jung 2025-12-31 15:29 ` SeongJae Park
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox