mm/damon: refactor damon_{start,stop,commit}() for simple error handling

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 make mistakes, and indeed a few bugs from such mistakes were found and fixed.

Refactor the core API functions to guarantee contexts are completely stopped under failures. Remove the caller side error handlers that are no longer needed due to the refactoring.

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 return errors only after the context is completely stopped. Patches 10 and 11 remove unnecessary error handlings from callers (wsse and prcl sample modules).

Changelog
=========

Changes from RFC v1.1
- RFC v1.1: https://lore.kernel.org/[email protected]
- Drop RFC tag.
- Rebase to latest mm-new.
Changes from RFC v1
- RFC v1: https://lore.kernel.org/[email protected]
- Fix ABBA deadlock in damon_start().
- Wait instead of doing racy-stop in damon_call().

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: wait ctx stop 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, 33 insertions(+), 34 deletions(-)

base-commit: 3fc9ce1db037091d433cb95dff0ce1cb6815fffe

2.47.3

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.

---
 mm/damon/core.c | 3 +++
 1 file changed, 3 insertions(+)

index 3c5a46e1c079a..d89a7f2faf350 100644
@@ -1917,6 +1917,9 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
 		running_exclusive_ctxs = true;
 	mutex_unlock(&damon_lock);
 
+	if (i != nr_ctxs)
+		damon_stop(ctxs, i);
+
 	return err;
 }
 

2.47.3

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.

---
 samples/damon/mtier.c | 2 --
 1 file changed, 2 deletions(-)

index e567f4edd80ea..90881e8bd441f 100644
@@ -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

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.

---
 mm/damon/core.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

index d89a7f2faf350..84e400aa4e823 100644
@@ -1926,10 +1926,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;
 

@@ -1939,31 +1937,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

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

---
 mm/damon/sysfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index b65651498e0d1..6d7e36c9e509d 100644
@@ -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

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

---
 mm/damon/reclaim.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 6469b25cc34f9..09e941d75f67d 100644
@@ -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

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

---
 mm/damon/lru_sort.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 2dd0cd0d26273..e8c389ad3226f 100644
@@ -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

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

---
 include/linux/damon.h | 2 +-
 mm/damon/core.c       | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

index 2661231c0ae82..19b7e839bde0b 100644
@@ -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);
 

index 84e400aa4e823..08510bf9b38da 100644
@@ -1947,14 +1947,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

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.

---
 samples/damon/mtier.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 90881e8bd441f..ac9c24b92ead8 100644
@@ -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

Wrong. mtier is not a loadable module, so this race cannot happen.

Thanks,
SJ

[...]

damon_call() failure means the DAMON context started its termination. The termination is asynchronously done in kdamond thread. 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, by waiting until the completion is confirmed.

---
 mm/damon/core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

index 08510bf9b38da..390e00b3685ef 100644
@@ -2010,6 +2010,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)

@@ -2022,7 +2024,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);

@@ -2030,8 +2032,14 @@ 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:
+	while (damon_is_running(ctx))
+		schedule_timeout_idle(msecs_to_jiffies(100));
+	return -ECANCELED;
+
 }
 
 /**

2.47.3

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.

---
 samples/damon/wsse.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index ff5e8a890f448..37fd5da201588 100644
@@ -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

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.

---
 samples/damon/prcl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index edeae145c4a8a..842099bd62286 100644
@@ -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