[v2] mm/damon/core: Optimize kdamod.%d thread creation code

Commit Message

haoxin Oct. 16, 2021, 4:59 p.m. UTC

When the ctx->adaptive_targets list is empty,
i did some test on monitor_on interface like this.

echo on > /sys/kernel/debug/damon/monitor_on
[  851.988307] damon: kdamond (5390) starts

Though the ctx->adaptive_targets list is empty, but the
kthread_run still be called, and the kdamond.x thread still
be created, this is meaningless.

So there adds a judgment. only if the ctx->adaptive_targets
list is not empty, and ctx->kdamond pointer is NULL, then call
the __damon_start function.

Signed-off-by: Xin Hao <[e-mail]>
---
 mm/damon/core.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

--
2.31.0

Comments

SeongJae Park Oct. 19, 2021, 10:03 a.m. UTC | #1

Hello Xin,

On Sun, 17 Oct 2021 00:59:14 +0800 Xin Hao <[e-mail]> wrote:

> When the ctx->adaptive_targets list is empty,
> i did some test on monitor_on interface like this.
> 
> echo on > /sys/kernel/debug/damon/monitor_on
> [  851.988307] damon: kdamond (5390) starts
> 
> Though the ctx->adaptive_targets list is empty, but the
> kthread_run still be called, and the kdamond.x thread still
> be created, this is meaningless.

Good finding, thank you!

> 
> So there adds a judgment. only if the ctx->adaptive_targets
> list is not empty, and ctx->kdamond pointer is NULL, then call
> the __damon_start function.

I think we cannot believe user-space users, but kernel-space API users.  How
about adding the check in 'dbgfs_monitor_on_write()' rather than
'__damon_start()'?

Also, apparently this patch is based on latest 'damon/next' branch of my tree.
Could you please rebase this on latest -mm tree?


Thanks,
SJ

[...]

Patch

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 30e9211f494a..998c707fdca2 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -107,6 +107,11 @@  void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
 	list_add_tail(&t->list, &ctx->adaptive_targets);
 }

+static int damon_target_empty(struct damon_ctx *ctx)
+{
+	return list_empty(&ctx->adaptive_targets);
+}
+
 static void damon_del_target(struct damon_target *t)
 {
 	list_del(&t->list);
@@ -307,15 +312,14 @@  static int __damon_start(struct damon_ctx *ctx)
 	int err = -EBUSY;

 	mutex_lock(&ctx->kdamond_lock);
-	if (!ctx->kdamond) {
+	ctx->kdamond_stop = false;
+	ctx->kdamond = kthread_run(kdamond_fn, ctx, "kdamond.%d",
+				   nr_running_ctxs);
+	if (IS_ERR(ctx->kdamond)) {
+		err = PTR_ERR(ctx->kdamond);
+		ctx->kdamond = 0;
+	} else {
 		err = 0;
-		ctx->kdamond_stop = false;
-		ctx->kdamond = kthread_run(kdamond_fn, ctx, "kdamond.%d",
-				nr_running_ctxs);
-		if (IS_ERR(ctx->kdamond)) {
-			err = PTR_ERR(ctx->kdamond);
-			ctx->kdamond = 0;
-		}
 	}
 	mutex_unlock(&ctx->kdamond_lock);

@@ -347,10 +351,12 @@  int damon_start(struct damon_ctx **ctxs, int nr_ctxs)
 	}

 	for (i = 0; i < nr_ctxs; i++) {
-		err = __damon_start(ctxs[i]);
-		if (err)
-			break;
-		nr_running_ctxs++;
+		if (!damon_target_empty(ctxs[i]) && !ctxs[i]->kdamond) {
+			err = __damon_start(ctxs[i]);
+			if (err)
+				break;
+			nr_running_ctxs++;
+		}
 	}
 	mutex_unlock(&damon_lock);