[RFC PATCH 00/11] mm/damon: refactor damon_{start,stop,commit}() for simple error handling

[RFC PATCH 00/11] mm/damon: refactor damon_{start,stop,commit}() for simple error handling

[SJ Park]

damon_start() and damon_stop() could leave a few of requested DAMON
contexts running when they return an error.  damon_commit() failure
stops the DAMON context, but in an asynchronous way.  Callers should
stop the left-over DAMON contexts.  It is easy to mistake, and indeed a
few bugs from such mistakes were found and fixed.

Refactor the core API functions to stop all contexts to avoid the
mistakes.  Also remove the unnecessary error handlings from callers.

Patches Sequence
================

Patch 1 refactors damon_start() to ensure all contexts are stopped for
failures.  Patch 2 updates unnecessary damon_start() error handling from
mtier sample module.

Patch 3 refactors damon_stop() to always success.  Patches 4-6 updates
callers (damon_{sysfs,reclaim lru_sort}) to ignore the return value.
Patch 7 update damon_stop() return value to void.  Patch 8 simplifies
damon_stop() error handling in mtier sample module.

Patch 9 refactors damon_call() to stop the context before returning the
error.  Patches 10 and 11 remove unnecessary error handlings from
callers (wsse and prcl sample modules).

SJ Park (11):
  mm/damon/core: stop ctxs in damon_start() before returning an error
  samples/damon/mtier: do not stop first context for damon_start()
    failure
  mm/damon/core: make damon_stop() never fails
  mm/damon/sysfs: ignore damon_stop() return value
  mm/damon/reclaaim: ignore damon_stop() return value
  mm/damon/lru_sort: ignore damon_stop() return value
  mm/damon/core: change damon_stop() return type to void
  samples/damon/mtier: stop all contexts with single damon_stop() call
  mm/damon/core: stop ctx in damon_call() before reruning an error
  samples/damon/wsse: do not stop ctx for damon_call() failure
  samples/damon/prcl: do not stop DAMON for damon_call() failure

 include/linux/damon.h |  2 +-
 mm/damon/core.c       | 37 ++++++++++++++++++-------------------
 mm/damon/lru_sort.c   |  6 ++++--
 mm/damon/reclaim.c    |  6 ++++--
 mm/damon/sysfs.c      |  3 ++-
 samples/damon/mtier.c |  5 +----
 samples/damon/prcl.c  |  4 +---
 samples/damon/wsse.c  |  4 +---
 8 files changed, 32 insertions(+), 35 deletions(-)


base-commit: 39f9f608b1858e65db6d2766d692a7a700b1e0fb
-- 
2.47.3

[RFC PATCH 01/11] mm/damon/core: stop ctxs in damon_start() before returning an error

[SJ Park]

When multiple contexts are passed to damon_start(), the function starts
the contexts one by one.  If any of the operations fails, it immediately
returns an error.  Contexts that successfully started before the failure
keep running.  The caller should catch this and stop the contexts.  It
is complicated and easy to make mistakes.  Stop all contexts in
damon_start() under the failures.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 3c5a46e1c079a..16d9223c33a72 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1909,8 +1909,10 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
 
 	for (i = 0; i < nr_ctxs; i++) {
 		err = __damon_start(ctxs[i]);
-		if (err)
+		if (err) {
+			damon_stop(ctxs, i);
 			break;
+		}
 		nr_running_ctxs++;
 	}
 	if (exclusive && nr_running_ctxs)
-- 
2.47.3

[RFC PATCH 02/11] samples/damon/mtier: do not stop first context for damon_start() failure

[SJ Park]

damon_start() was able to fail while keeping the first context running.
mtier hence stops the first context in the case.  damon_start() is
refactored to avoid that.  The error handling is no longer necessary.
Remove it.

Signed-off-by: SJ Park <sj@kernel.org>
---
 samples/damon/mtier.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index e567f4edd80ea..90881e8bd441f 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -193,8 +193,6 @@ static int damon_sample_mtier_start(void)
 	if (!err)
 		return 0;
 
-	if (damon_is_running(ctxs[0]))
-		damon_stop(ctxs, 1);
 	damon_destroy_ctx(ctxs[0]);
 	damon_destroy_ctx(ctxs[1]);
 	return err;
-- 
2.47.3

[RFC PATCH 03/11] mm/damon/core: make damon_stop() never fails

[SJ Park]

damon_stop() has no reason to fail.  It returns an error code only for
possible future changes that can make it fail.  Such a change has not
been made yet, and this only makes the error handling complicated and
confusing.  Ensure it returns no error.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 16d9223c33a72..ef40aa44850ff 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1925,10 +1925,8 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
 /*
  * __damon_stop() - Stops monitoring of a given context.
  * @ctx:	monitoring context
- *
- * Return: 0 on success, negative error code otherwise.
  */
-static int __damon_stop(struct damon_ctx *ctx)
+static void __damon_stop(struct damon_ctx *ctx)
 {
 	struct task_struct *tsk;
 
@@ -1938,31 +1936,24 @@ static int __damon_stop(struct damon_ctx *ctx)
 		get_task_struct(tsk);
 		mutex_unlock(&ctx->kdamond_lock);
 		kthread_stop_put(tsk);
-		return 0;
+		return;
 	}
 	mutex_unlock(&ctx->kdamond_lock);
-
-	return -EPERM;
 }
 
 /**
  * damon_stop() - Stops the monitorings for a given group of contexts.
  * @ctxs:	an array of the pointers for contexts to stop monitoring
  * @nr_ctxs:	size of @ctxs
- *
- * Return: 0 on success, negative error code otherwise.
  */
 int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
 {
-	int i, err = 0;
+	int i;
 
-	for (i = 0; i < nr_ctxs; i++) {
+	for (i = 0; i < nr_ctxs; i++)
 		/* nr_running_ctxs is decremented in kdamond_fn */
-		err = __damon_stop(ctxs[i]);
-		if (err)
-			break;
-	}
-	return err;
+		__damon_stop(ctxs[i]);
+	return 0;
 }
 
 /**
-- 
2.47.3

[RFC PATCH 04/11] mm/damon/sysfs: ignore damon_stop() return value

[SJ Park]

damon_stop() return value is guaranteed to be 0.  Ignore it.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/sysfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index b65651498e0d1..6d7e36c9e509d 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -2262,12 +2262,13 @@ static int damon_sysfs_turn_damon_off(struct damon_sysfs_kdamond *kdamond)
 {
 	if (!kdamond->damon_ctx)
 		return -EINVAL;
-	return damon_stop(&kdamond->damon_ctx, 1);
+	damon_stop(&kdamond->damon_ctx, 1);
 	/*
 	 * To allow users show final monitoring results of already turned-off
 	 * DAMON, we free kdamond->damon_ctx in next
 	 * damon_sysfs_turn_damon_on(), or kdamonds_nr_store()
 	 */
+	return 0;
 }
 
 static int damon_sysfs_damon_call(int (*fn)(void *data),
-- 
2.47.3

[RFC PATCH 05/11] mm/damon/reclaaim: ignore damon_stop() return value

[SJ Park]

damon_stop() return value is guaranteed to be 0.  Ignore it.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/reclaim.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 6469b25cc34f9..09e941d75f67d 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -332,8 +332,10 @@ static int damon_reclaim_turn(bool on)
 {
 	int err;
 
-	if (!on)
-		return damon_stop(&ctx, 1);
+	if (!on) {
+		damon_stop(&ctx, 1);
+		return 0;
+	}
 
 	err = damon_reclaim_apply_parameters();
 	if (err)
-- 
2.47.3

[RFC PATCH 06/11] mm/damon/lru_sort: ignore damon_stop() return value

[SJ Park]

damon_stop() return value is guaranteed to be 0.  Ignore it.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/lru_sort.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 2dd0cd0d26273..e8c389ad3226f 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -409,8 +409,10 @@ static int damon_lru_sort_turn(bool on)
 {
 	int err;
 
-	if (!on)
-		return damon_stop(&ctx, 1);
+	if (!on) {
+		damon_stop(&ctx, 1);
+		return 0;
+	}
 
 	err = damon_lru_sort_apply_parameters();
 	if (err)
-- 
2.47.3

[RFC PATCH 07/11] mm/damon/core: change damon_stop() return type to void

[SJ Park]

damon_stop() always returns 0, and nobody cares.  Change the return type
to void.

Signed-off-by: SJ Park <sj@kernel.org>
---
 include/linux/damon.h | 2 +-
 mm/damon/core.c       | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 2661231c0ae82..19b7e839bde0b 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -1070,7 +1070,7 @@ static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs
 
 bool damon_initialized(void);
 int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive);
-int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
+void damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
 bool damon_is_running(struct damon_ctx *ctx);
 int damon_kdamond_pid(struct damon_ctx *ctx);
 
diff --git a/mm/damon/core.c b/mm/damon/core.c
index ef40aa44850ff..1a6c30ae91711 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1946,14 +1946,13 @@ static void __damon_stop(struct damon_ctx *ctx)
  * @ctxs:	an array of the pointers for contexts to stop monitoring
  * @nr_ctxs:	size of @ctxs
  */
-int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
+void damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
 {
 	int i;
 
 	for (i = 0; i < nr_ctxs; i++)
 		/* nr_running_ctxs is decremented in kdamond_fn */
 		__damon_stop(ctxs[i]);
-	return 0;
 }
 
 /**
-- 
2.47.3

[RFC PATCH 08/11] samples/damon/mtier: stop all contexts with single damon_stop() call

[SJ Park]

damon_stop() was theoretically able to return failure while keeping the
second context for mtier running.  mtier stops its contexts one by one
with two damon_stop() call for the reason.  damon_stop() is refactored
to always successfully stop all requested DAMON contexts.  The two calls
are unnecessary.  Use only single damon_stop() call for all contexts.

Signed-off-by: SJ Park <sj@kernel.org>
---
 samples/damon/mtier.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index 90881e8bd441f..ac9c24b92ead8 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -200,8 +200,7 @@ static int damon_sample_mtier_start(void)
 
 static void damon_sample_mtier_stop(void)
 {
-	damon_stop(ctxs, 1);
-	damon_stop(&ctxs[1], 1);
+	damon_stop(ctxs, 2);
 	damon_destroy_ctx(ctxs[0]);
 	damon_destroy_ctx(ctxs[1]);
 }
-- 
2.47.3

[RFC PATCH 09/11] mm/damon/core: stop ctx in damon_call() before reruning an error

[SJ Park]

damon_call() failure makes the DAMON context to stop if it was running.
The stop is asynchronously done in kdamond thread, though.  The caller's
error handling should handle the race, too.  It is complicated and easy
to make mistakes.

Update damon_call() to ensure the context is stopped in the case.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 1a6c30ae91711..5f16d2e13ebf7 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2009,6 +2009,8 @@ int damon_kdamond_pid(struct damon_ctx *ctx)
  * @ctx has succeeded.  Otherwise, this function could fall into an indefinite
  * wait.
  *
+ * When this function is failed, the @ctx is guaranteed to be stopped.
+ *
  * Return: 0 on success, negative error code otherwise.
  */
 int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
@@ -2021,7 +2023,7 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
 	mutex_lock(&ctx->call_controls_lock);
 	if (ctx->call_controls_obsolete) {
 		mutex_unlock(&ctx->call_controls_lock);
-		return -ECANCELED;
+		goto canceled;
 	}
 	list_add_tail(&control->list, &ctx->call_controls);
 	mutex_unlock(&ctx->call_controls_lock);
@@ -2029,8 +2031,13 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
 		return 0;
 	wait_for_completion(&control->completion);
 	if (control->canceled)
-		return -ECANCELED;
+		goto canceled;
 	return 0;
+
+canceled:
+	__damon_stop(ctx);
+	return -ECANCELED;
+
 }
 
 /**
-- 
2.47.3

[RFC PATCH 10/11] samples/damon/wsse: do not stop ctx for damon_call() failure

[SJ Park]

damon_call() failure was causing the context to be stopped, but
asynchronously by the kdamond thread.  To handle the race,   the caller
had to stop the context.  damon_call() is updated to do the stop on its
own.  Remove the damon_stop() call from the damon_call() error handling.

Signed-off-by: SJ Park <sj@kernel.org>
---
 samples/damon/wsse.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/samples/damon/wsse.c b/samples/damon/wsse.c
index ff5e8a890f448..37fd5da201588 100644
--- a/samples/damon/wsse.c
+++ b/samples/damon/wsse.c
@@ -93,10 +93,8 @@ static int damon_sample_wsse_start(void)
 	}
 	repeat_call_control.data = ctx;
 	err = damon_call(ctx, &repeat_call_control);
-	if (err) {
-		damon_stop(&ctx, 1);
+	if (err)
 		damon_destroy_ctx(ctx);
-	}
 	return err;
 }
 
-- 
2.47.3

[RFC PATCH 11/11] samples/damon/prcl: do not stop DAMON for damon_call() failure

[SJ Park]

damon_call() failure was causing the context to be stopped, but
asynchronously by the kdamond thread.  To handle the race,   the caller
had to stop the context.  damon_call() is updated to do the stop on its
own.  Remove the damon_stop() call from the damon_call() error handling.

Signed-off-by: SJ Park <sj@kernel.org>
---
 samples/damon/prcl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/samples/damon/prcl.c b/samples/damon/prcl.c
index edeae145c4a8a..842099bd62286 100644
--- a/samples/damon/prcl.c
+++ b/samples/damon/prcl.c
@@ -113,10 +113,8 @@ static int damon_sample_prcl_start(void)
 
 	repeat_call_control.data = ctx;
 	err = damon_call(ctx, &repeat_call_control);
-	if (err) {
-		damon_stop(&ctx, 1);
+	if (err)
 		damon_destroy_ctx(ctx);
-	}
 	return err;
 }
 
-- 
2.47.3

Re: [RFC PATCH 01/11] mm/damon/core: stop ctxs in damon_start() before returning an error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling damon_stop() from within damon_start() while holding damon_lock causes an AB-BA deadlock with kdamond_fn().
--

commit 1cec89c3cfde2c1ab34bcccb4767280aac126514
Author: SJ Park <sj@kernel.org>

mm/damon/core: stop ctxs in damon_start() before returning an error

When multiple contexts are passed to damon_start(), it starts them one
by one and returns an error immediately on failure. This commit adds logic
to stop any already-started contexts directly within damon_start() if a
subsequent context fails to start.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c6fc35be633b3..45aa9acef4e30 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1836,8 +1836,10 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
>  
>  	for (i = 0; i < nr_ctxs; i++) {
>  		err = __damon_start(ctxs[i]);
> -		if (err)
> +		if (err) {
> +			damon_stop(ctxs, i);

[Severity: High]
Will this cause a deadlock if a context fails to start?

damon_start() holds damon_lock while iterating through the contexts. When it
encounters an error, it calls damon_stop() which synchronously waits for the
previously started kthreads to stop.

However, the kdamond_fn thread attempts to acquire the same damon_lock
during its cleanup phase before it can exit:

kdamond_fn() {
        ...
        mutex_lock(&damon_lock);
        nr_running_ctxs--;
        ...
        mutex_unlock(&damon_lock);
}

Since damon_start() is blocking on the thread exit while holding damon_lock,
and the thread is blocking on damon_lock to exit, doesn't this result in a
circular wait?

>  			break;
> +		}
>  		nr_running_ctxs++;
>  	}
>  	if (exclusive && nr_running_ctxs)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=1

Re: [RFC PATCH 08/11] samples/damon/mtier: stop all contexts with single damon_stop() call

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] AB-BA deadlock in `damon_start()` error path with `kdamond_fn` over `damon_lock`.
--

commit 65a3f2350afc5895953b74d531916747653f71ed
Author: SJ Park <sj@kernel.org>

samples/damon/mtier: stop all contexts with single damon_stop() call

damon_stop() was theoretically able to return failure while keeping the
second context for mtier running.  mtier stops its contexts one by one
with two damon_stop() call for the reason.  damon_stop() is refactored
to always successfully stop all requested DAMON contexts.  The two calls
are unnecessary.  Use only single damon_stop() call for all contexts.

> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index 90881e8bd441f..ac9c24b92ead8 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
> @@ -200,8 +200,7 @@ static int damon_sample_mtier_start(void)
>  
>  static void damon_sample_mtier_stop(void)
>  {
> -	damon_stop(ctxs, 1);
> -	damon_stop(&ctxs[1], 1);
> +	damon_stop(ctxs, 2);

[Severity: High]
This bug was introduced in this series by commit "mm/damon/core: stop ctxs
in damon_start() before returning an error" and remains unfixed at the end
of the series.

Does the error path in damon_start() introduce an AB-BA deadlock with
kdamond_fn() over damon_lock?

If a caller attempts to start multiple DAMON contexts and damon_start()
successfully starts at least one context but fails on a subsequent one,
it calls damon_stop() while still holding damon_lock:

mm/damon/core.c:damon_start() {
	mutex_lock(&damon_lock);
	// ...
	for (i = 0; i < nr_ctxs; i++) {
		err = __damon_start(ctxs[i]);
		if (err) {
			damon_stop(ctxs, i);
			break;
		}
		nr_running_ctxs++;
	}
}

damon_stop() synchronously waits for the kdamond thread to exit. However,
the kdamond thread blocks forever waiting to acquire damon_lock to decrement
nr_running_ctxs:

mm/damon/core.c:kdamond_fn() {
	mutex_lock(&damon_lock);
	nr_running_ctxs--;
	if (!nr_running_ctxs && running_exclusive_ctxs)
		running_exclusive_ctxs = false;
	mutex_unlock(&damon_lock);
}

Could this permanent deadlock hang the kdamond kernel thread and any process
attempting to use DAMON?

>  	damon_destroy_ctx(ctxs[0]);
>  	damon_destroy_ctx(ctxs[1]);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=8

Re: [RFC PATCH 07/11] mm/damon/core: change damon_stop() return type to void

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ABBA deadlock in the error handling path of `damon_start()` when a context fails to start.
--

commit 48348b0ead7556cdf0c68b6bf17ef7a1991b89c8
Author: SJ Park <sj@kernel.org>

mm/damon/core: change damon_stop() return type to void

This commit changes the return type of damon_stop() from int to void,
as it always returned 0 and callers did not check the value.

[ ... ]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 0845d6b151f81..aa48bf44c2a0b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1873,14 +1873,13 @@ static void __damon_stop(struct damon_ctx *ctx)
>   * @ctxs:	an array of the pointers for contexts to stop monitoring
>   * @nr_ctxs:	size of @ctxs
>   */
> -int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
> +void damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
>  {

[Severity: High]
Can this cause an ABBA deadlock in the error handling path of damon_start()
when a context fails to start?

When damon_start() is called with multiple contexts, it acquires damon_lock.
If the first context starts successfully, it spawns a kdamond thread which
begins execution in kdamond_fn().

If the second context fails to start, the error path calls damon_stop() while
still holding damon_lock:

mm/damon/core.c:damon_start() {
    ...
		err = __damon_start(ctxs[i]);
		if (err) {
			damon_stop(ctxs, i);
			break;
		}
    ...
}

damon_stop() eventually calls kthread_stop_put() on the thread for the
first context, which blocks waiting for the thread to exit.

However, the first context's thread attempts to acquire damon_lock in
kdamond_fn() to decrement nr_running_ctxs:

mm/damon/core.c:kdamond_fn() {
    ...
	mutex_lock(&damon_lock);
	nr_running_ctxs--;
    ...
}

Since damon_start() holds damon_lock while waiting for the thread to exit,
and the thread waits for damon_lock to exit, won't this result in a permanent
deadlock?

>  	int i;
>  
>  	for (i = 0; i < nr_ctxs; i++)
>  		/* nr_running_ctxs is decremented in kdamond_fn */
>  		__damon_stop(ctxs[i]);
> -	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=7

Re: [RFC PATCH 02/11] samples/damon/mtier: do not stop first context for damon_start() failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ABBA deadlock in damon_start() during error recovery when rolling back successfully started contexts.
--

commit c411cd45182c74c483bd6bd5d58aea28123f720d
Author: SJ Park <sj@kernel.org>

samples/damon/mtier: do not stop first context for damon_start() failure

damon_start() was able to fail while keeping the first context running.
mtier hence stops the first context in the case. damon_start() is
refactored to avoid that. The error handling is no longer necessary.
Remove it.

> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index e567f4edd80ea..90881e8bd441f 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
> @@ -193,8 +193,6 @@ static int damon_sample_mtier_start(void)
>  	if (!err)
>  		return 0;
>  
> -	if (damon_is_running(ctxs[0]))
> -		damon_stop(ctxs, 1);

[Severity: High]
Does the refactored error handling in damon_start() introduce a deadlock?

Since damon_start() is called with nr_ctxs = 2 here, if the first context
starts successfully but the second one fails, the error recovery path in
damon_start() could trigger an ABBA deadlock.

If damon_start() calls damon_stop() on the error path while holding
damon_lock:

mm/damon/core.c:damon_start() {
    ...
		err = __damon_start(ctxs[i]);
		if (err) {
			damon_stop(ctxs, i);
			break;
		}
    ...
}

damon_stop() blocks until the kdamond_fn() thread exits. However, kdamond_fn()
attempts to acquire damon_lock at its exit path:

mm/damon/core.c:kdamond_fn() {
    ...
	mutex_lock(&damon_lock);
	nr_running_ctxs--;
	if (!nr_running_ctxs && running_exclusive_ctxs)
		running_exclusive_ctxs = false;
	mutex_unlock(&damon_lock);
    ...
}

Could this cause kdamond_fn() to hang waiting for damon_lock, which is still
held by damon_start(), resulting in a deadlock?

>  	damon_destroy_ctx(ctxs[0]);
>  	damon_destroy_ctx(ctxs[1]);
>  	return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=2

Re: [RFC PATCH 09/11] mm/damon/core: stop ctx in damon_call() before reruning an error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent invocations of `__damon_stop()` triggered by canceled `damon_call()`s lead to a double `kthread_stop()` deadlock.
--

commit 5b208204f87a1b80335c35ce1b66a42e8f74a1ba
Author: SJ Park <sj@kernel.org>

mm/damon/core: stop ctx in damon_call() before reruning an error

This commit updates damon_call() to ensure the DAMON context is explicitly
stopped when a call fails or is canceled.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index aa48bf44c2a0b..9c15d44084af4 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -1956,8 +1958,13 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
>  		return 0;
>  	wait_for_completion(&control->completion);
>  	if (control->canceled)
> -		return -ECANCELED;
> +		goto canceled;
>  	return 0;
> +
> +canceled:
> +	__damon_stop(ctx);

[Severity: High]
Could this lead to a deadlock if multiple threads invoke damon_call()
concurrently on the same context?

If multiple threads are waiting in damon_call() and the kthread exits, it
cancels all pending controls and wakes the waiters. These threads will jump
to the canceled label and call __damon_stop(ctx) concurrently.

Looking at __damon_stop(), it reads ctx->kdamond without clearing it:

static void __damon_stop(struct damon_ctx *ctx)
{
	struct task_struct *tsk;

	mutex_lock(&ctx->kdamond_lock);
	tsk = ctx->kdamond;
	if (tsk) {
		get_task_struct(tsk);
		mutex_unlock(&ctx->kdamond_lock);
		kthread_stop_put(tsk);
		return;
	}
	mutex_unlock(&ctx->kdamond_lock);
}

Does this allow multiple threads to fetch the same task struct and call
kthread_stop_put() concurrently? 

Since kthread_stop() waits on the kthread's exit completion which is
signaled via complete() (waking only one waiter), would the subsequent
threads block forever in kthread_stop(), causing a deadlock?

> +	return -ECANCELED;
> +
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=9

Re: [RFC PATCH 03/11] mm/damon/core: make damon_stop() never fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] AB-BA deadlock in damon_start() error path when failing to start multiple contexts.
--

commit c787674782fb673ed8c1babcfb03eca187f10a1d
Author: SJ Park <sj@kernel.org>
Subject: mm/damon/core: make damon_stop() never fails

This commit modifies damon_stop() and __damon_stop() to return void instead
of an error code. This simplifies the error handling in damon_stop() since
it no longer needs to break out of the loop on failure.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 45aa9acef4e30..0845d6b151f81 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -1865,31 +1863,24 @@ static int __damon_stop(struct damon_ctx *ctx)
[ ... ]
>  /**
>   * damon_stop() - Stops the monitorings for a given group of contexts.
>   * @ctxs:	an array of the pointers for contexts to stop monitoring
>   * @nr_ctxs:	size of @ctxs
> - *
> - * Return: 0 on success, negative error code otherwise.
>   */
>  int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
>  {
> -	int i, err = 0;
> +	int i;
>  
> -	for (i = 0; i < nr_ctxs; i++) {
> +	for (i = 0; i < nr_ctxs; i++)
>  		/* nr_running_ctxs is decremented in kdamond_fn */
> -		err = __damon_stop(ctxs[i]);
> -		if (err)
> -			break;
> -	}
> -	return err;
> +		__damon_stop(ctxs[i]);

[Severity: High]
Does calling damon_stop() here result in an AB-BA deadlock during a partial
startup failure?

While this issue was introduced by an earlier patch in the series, it remains
present here. If damon_start() successfully starts the first context but fails
to start a subsequent one, it calls damon_stop() while still holding
damon_lock:

mm/damon/core.c:damon_start() {
    ...
    mutex_lock(&damon_lock);
    ...
    for (i = 0; i < nr_ctxs; i++) {
        err = __damon_start(ctxs[i]);
        if (err) {
            damon_stop(ctxs, i);
            break;
        }
        nr_running_ctxs++;
    }
    ...
    mutex_unlock(&damon_lock);
}

Inside damon_stop(), __damon_stop() calls kthread_stop_put(), which blocks
waiting for the running kdamond thread to exit. However, kdamond_fn() must
acquire damon_lock to decrement nr_running_ctxs before it can complete its
exit:

mm/damon/core.c:kdamond_fn() {
    ...
done:
    ...
    mutex_lock(&damon_lock);
    nr_running_ctxs--;
    ...
    mutex_unlock(&damon_lock);
    return 0;
}

Can this cause damon_start() and the kdamond thread to deadlock waiting for
each other?

> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=3

Re: [RFC PATCH 10/11] samples/damon/wsse: do not stop ctx for damon_call() failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A self-deadlock occurs in `damon_start()` if a subsequent context fails to start.
--

commit 2f9c1f0eae64d458e95f8baff7b56262e634bb2f
Author: SJ Park <sj@kernel.org>

samples/damon/wsse: do not stop ctx for damon_call() failure

This commit modifies the error handling path in damon_sample_wsse_start()
when damon_call() fails. It removes the explicit call to damon_stop()
because damon_call() is updated to stop the context on its own.

> diff --git a/samples/damon/wsse.c b/samples/damon/wsse.c
> index ff5e8a890f448..37fd5da201588 100644
> --- a/samples/damon/wsse.c
> +++ b/samples/damon/wsse.c
> @@ -93,10 +93,8 @@ static int damon_sample_wsse_start(void)
>  	}
>  	repeat_call_control.data = ctx;
>  	err = damon_call(ctx, &repeat_call_control);
> -	if (err) {
> -		damon_stop(&ctx, 1);
> +	if (err)
>  		damon_destroy_ctx(ctx);
> -	}
>  	return err;
>  }

[Severity: High]
Does this code expose a self-deadlock in damon_start() if a context fails to
start?

When damon_sample_wsse_start() calls damon_start() earlier in the execution,
it iterates over the contexts and calls __damon_start() to spawn a kdamond
thread for each, all while holding damon_lock. If __damon_start() fails, the
error handling path calls damon_stop() to tear down the already-started
contexts:

mm/damon/core.c:damon_start() {
    ...
    mutex_lock(&damon_lock);
    ...
        err = __damon_start(ctxs[i]);
        if (err) {
            damon_stop(ctxs, i);
            break;
        }
    ...
}

damon_stop() internally calls __damon_stop(), which invokes kthread_stop_put()
to synchronously wait for the running kdamond thread to exit.

However, when the exiting kdamond thread finishes its main loop and cleans up,
it attempts to acquire damon_lock to decrement nr_running_ctxs:

mm/damon/core.c:kdamond_fn() {
    ...
    mutex_lock(&damon_lock);
    nr_running_ctxs--;
    if (!nr_running_ctxs && running_exclusive_ctxs)
        running_exclusive_ctxs = false;
    mutex_unlock(&damon_lock);
    ...
}

Because the original thread executing damon_start() is still holding
damon_lock while waiting for the kdamond thread to exit, can this result
in a permanent ABBA deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=10

Re: [RFC PATCH 01/11] mm/damon/core: stop ctxs in damon_start() before returning an error

[SJ Park]

On Sat, 04 Jul 2026 18:20:49 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Calling damon_stop() from within damon_start() while holding damon_lock causes an AB-BA deadlock with kdamond_fn().
> --
> 
> commit 1cec89c3cfde2c1ab34bcccb4767280aac126514
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: stop ctxs in damon_start() before returning an error
> 
> When multiple contexts are passed to damon_start(), it starts them one
> by one and returns an error immediately on failure. This commit adds logic
> to stop any already-started contexts directly within damon_start() if a
> subsequent context fails to start.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index c6fc35be633b3..45aa9acef4e30 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1836,8 +1836,10 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
> >  
> >  	for (i = 0; i < nr_ctxs; i++) {
> >  		err = __damon_start(ctxs[i]);
> > -		if (err)
> > +		if (err) {
> > +			damon_stop(ctxs, i);
> 
> [Severity: High]
> Will this cause a deadlock if a context fails to start?
> 
> damon_start() holds damon_lock while iterating through the contexts. When it
> encounters an error, it calls damon_stop() which synchronously waits for the
> previously started kthreads to stop.
> 
> However, the kdamond_fn thread attempts to acquire the same damon_lock
> during its cleanup phase before it can exit:
> 
> kdamond_fn() {
>         ...
>         mutex_lock(&damon_lock);
>         nr_running_ctxs--;
>         ...
>         mutex_unlock(&damon_lock);
> }
> 
> Since damon_start() is blocking on the thread exit while holding damon_lock,
> and the thread is blocking on damon_lock to exit, doesn't this result in a
> circular wait?

Good finding.  I will fix this in the next revision by calling damon_stop()
after releasing the mutex, like below.

"""
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2182,16 +2182,17 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)

        for (i = 0; i < nr_ctxs; i++) {
                err = __damon_start(ctxs[i]);
-               if (err) {
-                       damon_stop(ctxs, i);
+               if (err)
                        break;
-               }
                nr_running_ctxs++;
        }
        if (exclusive && nr_running_ctxs)
                running_exclusive_ctxs = true;
        mutex_unlock(&damon_lock);

+       if (i != nr_ctxs)
+               damon_stop(ctxs, i);
+
        return err;
 }
"""


Thanks,
SJ

[...]

Re: [RFC PATCH 02/11] samples/damon/mtier: do not stop first context for damon_start() failure

[SJ Park]

On Sat, 04 Jul 2026 18:24:05 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ABBA deadlock in damon_start() during error recovery when rolling back successfully started contexts.
> --
> 
> commit c411cd45182c74c483bd6bd5d58aea28123f720d
> Author: SJ Park <sj@kernel.org>
> 
> samples/damon/mtier: do not stop first context for damon_start() failure
> 
> damon_start() was able to fail while keeping the first context running.
> mtier hence stops the first context in the case. damon_start() is
> refactored to avoid that. The error handling is no longer necessary.
> Remove it.
> 
> > diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> > index e567f4edd80ea..90881e8bd441f 100644
> > --- a/samples/damon/mtier.c
> > +++ b/samples/damon/mtier.c
> > @@ -193,8 +193,6 @@ static int damon_sample_mtier_start(void)
> >  	if (!err)
> >  		return 0;
> >  
> > -	if (damon_is_running(ctxs[0]))
> > -		damon_stop(ctxs, 1);
> 
> [Severity: High]
> Does the refactored error handling in damon_start() introduce a deadlock?
> 
> Since damon_start() is called with nr_ctxs = 2 here, if the first context
> starts successfully but the second one fails, the error recovery path in
> damon_start() could trigger an ABBA deadlock.
> 
> If damon_start() calls damon_stop() on the error path while holding
> damon_lock:
> 
> mm/damon/core.c:damon_start() {
>     ...
> 		err = __damon_start(ctxs[i]);
> 		if (err) {
> 			damon_stop(ctxs, i);
> 			break;
> 		}
>     ...
> }
> 
> damon_stop() blocks until the kdamond_fn() thread exits. However, kdamond_fn()
> attempts to acquire damon_lock at its exit path:
> 
> mm/damon/core.c:kdamond_fn() {
>     ...
> 	mutex_lock(&damon_lock);
> 	nr_running_ctxs--;
> 	if (!nr_running_ctxs && running_exclusive_ctxs)
> 		running_exclusive_ctxs = false;
> 	mutex_unlock(&damon_lock);
>     ...
> }
> 
> Could this cause kdamond_fn() to hang waiting for damon_lock, which is still
> held by damon_start(), resulting in a deadlock?

Yes, I will fix damon_start() it in the next revision.


Thanks,
SJ

[...]

Re: [RFC PATCH 03/11] mm/damon/core: make damon_stop() never fails

[SJ Park]

On Sat, 04 Jul 2026 18:30:02 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] AB-BA deadlock in damon_start() error path when failing to start multiple contexts.
> --
> 
> commit c787674782fb673ed8c1babcfb03eca187f10a1d
> Author: SJ Park <sj@kernel.org>
> Subject: mm/damon/core: make damon_stop() never fails
> 
> This commit modifies damon_stop() and __damon_stop() to return void instead
> of an error code. This simplifies the error handling in damon_stop() since
> it no longer needs to break out of the loop on failure.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 45aa9acef4e30..0845d6b151f81 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -1865,31 +1863,24 @@ static int __damon_stop(struct damon_ctx *ctx)
> [ ... ]
> >  /**
> >   * damon_stop() - Stops the monitorings for a given group of contexts.
> >   * @ctxs:	an array of the pointers for contexts to stop monitoring
> >   * @nr_ctxs:	size of @ctxs
> > - *
> > - * Return: 0 on success, negative error code otherwise.
> >   */
> >  int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
> >  {
> > -	int i, err = 0;
> > +	int i;
> >  
> > -	for (i = 0; i < nr_ctxs; i++) {
> > +	for (i = 0; i < nr_ctxs; i++)
> >  		/* nr_running_ctxs is decremented in kdamond_fn */
> > -		err = __damon_stop(ctxs[i]);
> > -		if (err)
> > -			break;
> > -	}
> > -	return err;
> > +		__damon_stop(ctxs[i]);
> 
> [Severity: High]
> Does calling damon_stop() here result in an AB-BA deadlock during a partial
> startup failure?
> 
> While this issue was introduced by an earlier patch in the series, it remains
> present here. If damon_start() successfully starts the first context but fails
> to start a subsequent one, it calls damon_stop() while still holding
> damon_lock:
> 
> mm/damon/core.c:damon_start() {
>     ...
>     mutex_lock(&damon_lock);
>     ...
>     for (i = 0; i < nr_ctxs; i++) {
>         err = __damon_start(ctxs[i]);
>         if (err) {
>             damon_stop(ctxs, i);
>             break;
>         }
>         nr_running_ctxs++;
>     }
>     ...
>     mutex_unlock(&damon_lock);
> }
> 
> Inside damon_stop(), __damon_stop() calls kthread_stop_put(), which blocks
> waiting for the running kdamond thread to exit. However, kdamond_fn() must
> acquire damon_lock to decrement nr_running_ctxs before it can complete its
> exit:
> 
> mm/damon/core.c:kdamond_fn() {
>     ...
> done:
>     ...
>     mutex_lock(&damon_lock);
>     nr_running_ctxs--;
>     ...
>     mutex_unlock(&damon_lock);
>     return 0;
> }
> 
> Can this cause damon_start() and the kdamond thread to deadlock waiting for
> each other?

Yes, I will fix damon_start() in the next revision.


Thanks,
SJ

[...]

Re: [RFC PATCH 09/11] mm/damon/core: stop ctx in damon_call() before reruning an error

[SJ Park]

On Sat, 04 Jul 2026 18:25:12 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Concurrent invocations of `__damon_stop()` triggered by canceled `damon_call()`s lead to a double `kthread_stop()` deadlock.
> --
> 
> commit 5b208204f87a1b80335c35ce1b66a42e8f74a1ba
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: stop ctx in damon_call() before reruning an error
> 
> This commit updates damon_call() to ensure the DAMON context is explicitly
> stopped when a call fails or is canceled.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index aa48bf44c2a0b..9c15d44084af4 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -1956,8 +1958,13 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
> >  		return 0;
> >  	wait_for_completion(&control->completion);
> >  	if (control->canceled)
> > -		return -ECANCELED;
> > +		goto canceled;
> >  	return 0;
> > +
> > +canceled:
> > +	__damon_stop(ctx);
> 
> [Severity: High]
> Could this lead to a deadlock if multiple threads invoke damon_call()
> concurrently on the same context?
> 
> If multiple threads are waiting in damon_call() and the kthread exits, it
> cancels all pending controls and wakes the waiters. These threads will jump
> to the canceled label and call __damon_stop(ctx) concurrently.
> 
> Looking at __damon_stop(), it reads ctx->kdamond without clearing it:
> 
> static void __damon_stop(struct damon_ctx *ctx)
> {
> 	struct task_struct *tsk;
> 
> 	mutex_lock(&ctx->kdamond_lock);
> 	tsk = ctx->kdamond;
> 	if (tsk) {
> 		get_task_struct(tsk);
> 		mutex_unlock(&ctx->kdamond_lock);
> 		kthread_stop_put(tsk);
> 		return;
> 	}
> 	mutex_unlock(&ctx->kdamond_lock);
> }
> 
> Does this allow multiple threads to fetch the same task struct and call
> kthread_stop_put() concurrently? 
> 
> Since kthread_stop() waits on the kthread's exit completion which is
> signaled via complete() (waking only one waiter), would the subsequent
> threads block forever in kthread_stop(), causing a deadlock?

Good finding.  But there is no such parallel usage of damon_call() to my
understanding.

DAMON_SYSFS calls damon_call() for state file writes.  Multiple threads can
write the file in parallel.  But the damon_call() from store file write
callback (state_store()) is protected by global damon_sysfs_lock.

DAMON modules can call damon_call() for parameters commit and initial start.
But param file writing is protected by the param_lock.

This may be better to be documented or make the code more robust.  But no real
issue.

> 
> > +	return -ECANCELED;
> > +
> >  }
> >  
> >  /**
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=9


Thanks,
SJ

Re: [RFC PATCH 07/11] mm/damon/core: change damon_stop() return type to void

[SJ Park]

On Sat, 04 Jul 2026 18:23:23 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ABBA deadlock in the error handling path of `damon_start()` when a context fails to start.

I will fix damon_start() patch in the next revision.


Thanks,
SJ

[...]

Re: [RFC PATCH 08/11] samples/damon/mtier: stop all contexts with single damon_stop() call

[SJ Park]

On Sat, 04 Jul 2026 18:20:55 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] AB-BA deadlock in `damon_start()` error path with `kdamond_fn` over `damon_lock`.

I will fix damon_start() patch in the next revision.


Thanks,
SJ

[...]

Re: [RFC PATCH 10/11] samples/damon/wsse: do not stop ctx for damon_call() failure

[SJ Park]

On Sat, 04 Jul 2026 18:31:59 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] A self-deadlock occurs in `damon_start()` if a subsequent context fails to start.

I will fix damon_start() patch in the next version.


Thanks,
SJ

[...]

Re: [RFC PATCH 09/11] mm/damon/core: stop ctx in damon_call() before reruning an error

[SJ Park]

On Sat, 04 Jul 2026 12:35:01 -0700 SJ Park <sj@kernel.org> wrote:

> On Sat, 04 Jul 2026 18:25:12 +0000 sashiko-bot@kernel.org wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Concurrent invocations of `__damon_stop()` triggered by canceled `damon_call()`s lead to a double `kthread_stop()` deadlock.
> > --
> > 
> > commit 5b208204f87a1b80335c35ce1b66a42e8f74a1ba
> > Author: SJ Park <sj@kernel.org>
> > 
> > mm/damon/core: stop ctx in damon_call() before reruning an error
> > 
> > This commit updates damon_call() to ensure the DAMON context is explicitly
> > stopped when a call fails or is canceled.
> > 
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index aa48bf44c2a0b..9c15d44084af4 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > [ ... ]
> > > @@ -1956,8 +1958,13 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
> > >  		return 0;
> > >  	wait_for_completion(&control->completion);
> > >  	if (control->canceled)
> > > -		return -ECANCELED;
> > > +		goto canceled;
> > >  	return 0;
> > > +
> > > +canceled:
> > > +	__damon_stop(ctx);
> > 
> > [Severity: High]
> > Could this lead to a deadlock if multiple threads invoke damon_call()
> > concurrently on the same context?
> > 
> > If multiple threads are waiting in damon_call() and the kthread exits, it
> > cancels all pending controls and wakes the waiters. These threads will jump
> > to the canceled label and call __damon_stop(ctx) concurrently.
> > 
> > Looking at __damon_stop(), it reads ctx->kdamond without clearing it:
> > 
> > static void __damon_stop(struct damon_ctx *ctx)
> > {
> > 	struct task_struct *tsk;
> > 
> > 	mutex_lock(&ctx->kdamond_lock);
> > 	tsk = ctx->kdamond;
> > 	if (tsk) {
> > 		get_task_struct(tsk);
> > 		mutex_unlock(&ctx->kdamond_lock);
> > 		kthread_stop_put(tsk);
> > 		return;
> > 	}
> > 	mutex_unlock(&ctx->kdamond_lock);
> > }
> > 
> > Does this allow multiple threads to fetch the same task struct and call
> > kthread_stop_put() concurrently? 
> > 
> > Since kthread_stop() waits on the kthread's exit completion which is
> > signaled via complete() (waking only one waiter), would the subsequent
> > threads block forever in kthread_stop(), causing a deadlock?
> 
> Good finding.  But there is no such parallel usage of damon_call() to my
> understanding.
> 
> DAMON_SYSFS calls damon_call() for state file writes.  Multiple threads can
> write the file in parallel.  But the damon_call() from store file write
> callback (state_store()) is protected by global damon_sysfs_lock.
> 
> DAMON modules can call damon_call() for parameters commit and initial start.
> But param file writing is protected by the param_lock.
> 
> This may be better to be documented or make the code more robust.

damon_call() is designed for multiple callers, so this is quite confusing.  I
will make it less confusing like below in the next revision.

"""
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2309,7 +2309,8 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
        return 0;

 canceled:
-       __damon_stop(ctx);
+       while (damon_is_running(ctx))
+               schedule_timeout_idle(msecs_to_jiffies(100));
        return -ECANCELED;

 }
"""


Thanks,
SJ