diff mbox series

[1/2] mm/damon/sysfs: remove requested targets when online-commit inputs

Message ID 20231022210735.46409-2-sj@kernel.org
State New
Headers
Series mm/damon/sysfs: fix unexpected targets adding bug |

Commit Message

SeongJae Park Oct. 22, 2023, 9:07 p.m. UTC 
  damon_sysfs_set_targets(), which updates the targets of the context for
online commitment, do not remove targets that removed from the
corresponding sysfs files.  As a result, more than intended targets of
the context can exist and hence consume memory and monitoring CPU
resource more than expected.

Fix it by removing all targets of the context and fill up again using
the user input.  This could cause unnecessary memory dealloc and realloc
operations, but this is not a hot code path.  Also, note that
damon_target is stateless, and hence no data is lost.

Fixes: da87878010e5 ("mm/damon/sysfs: support online inputs update")
Cc: <stable@vger.kernel.org> # 5.19.x
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/sysfs.c | 50 +++++++++---------------------------------------
 1 file changed, 9 insertions(+), 41 deletions(-)

Comments

SeongJae Park Oct. 28, 2023, 9:09 p.m. UTC | #1 
On Sun, 22 Oct 2023 21:07:33 +0000 SeongJae Park <sj@kernel.org> wrote:

> damon_sysfs_set_targets(), which updates the targets of the context for
> online commitment, do not remove targets that removed from the
> corresponding sysfs files.  As a result, more than intended targets of
> the context can exist and hence consume memory and monitoring CPU
> resource more than expected.
> 
> Fix it by removing all targets of the context and fill up again using
> the user input.  This could cause unnecessary memory dealloc and realloc
> operations, but this is not a hot code path.  Also, note that
> damon_target is stateless, and hence no data is lost.

This is not true.  'struct damon_target' contains monitoring results
(regions_list).  Hence, this patch makes all monitoring results to be removed
whenever doing online-commit.  I was confused with init_regions at the time of
this writing, sorry.

I will send a fix for this soon.


Thanks,
SJ
diff mbox series

Patch

diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index f73dc88d2d19..5268e8503722 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1150,58 +1150,26 @@  static int damon_sysfs_add_target(struct damon_sysfs_target *sys_target,
 	return err;
 }
 
-/*
- * Search a target in a context that corresponds to the sysfs target input.
- *
- * Return: pointer to the target if found, NULL if not found, or negative
- * error code if the search failed.
- */
-static struct damon_target *damon_sysfs_existing_target(
-		struct damon_sysfs_target *sys_target, struct damon_ctx *ctx)
-{
-	struct pid *pid;
-	struct damon_target *t;
-
-	if (!damon_target_has_pid(ctx)) {
-		/* Up to only one target for paddr could exist */
-		damon_for_each_target(t, ctx)
-			return t;
-		return NULL;
-	}
-
-	/* ops.id should be DAMON_OPS_VADDR or DAMON_OPS_FVADDR */
-	pid = find_get_pid(sys_target->pid);
-	if (!pid)
-		return ERR_PTR(-EINVAL);
-	damon_for_each_target(t, ctx) {
-		if (t->pid == pid) {
-			put_pid(pid);
-			return t;
-		}
-	}
-	put_pid(pid);
-	return NULL;
-}
-
 static int damon_sysfs_set_targets(struct damon_ctx *ctx,
 		struct damon_sysfs_targets *sysfs_targets)
 {
+	struct damon_target *t, *next;
 	int i, err;
 
 	/* Multiple physical address space monitoring targets makes no sense */
 	if (ctx->ops.id == DAMON_OPS_PADDR && sysfs_targets->nr > 1)
 		return -EINVAL;
 
+	damon_for_each_target_safe(t, next, ctx) {
+		if (damon_target_has_pid(ctx))
+			put_pid(t->pid);
+		damon_destroy_target(t);
+	}
+
 	for (i = 0; i < sysfs_targets->nr; i++) {
 		struct damon_sysfs_target *st = sysfs_targets->targets_arr[i];
-		struct damon_target *t = damon_sysfs_existing_target(st, ctx);
-
-		if (IS_ERR(t))
-			return PTR_ERR(t);
-		if (!t)
-			err = damon_sysfs_add_target(st, ctx);
-		else
-			err = damon_sysfs_set_regions(t, st->regions);
+
+		err = damon_sysfs_add_target(st, ctx);
 		if (err)
 			return err;
 	}