* [PATCH 1/2] mm/damon/sysfs: remove requested targets when online-commit inputs
2023-10-22 21:07 [PATCH 0/2] mm/damon/sysfs: fix unexpected targets adding bug SeongJae Park
@ 2023-10-22 21:07 ` SeongJae Park
2023-10-28 21:09 ` SeongJae Park
2023-10-22 21:07 ` [PATCH 2/2] mm/damon/sysfs-test: add a unit test for damon_sysfs_set_targets() SeongJae Park
1 sibling, 1 reply; 4+ messages in thread
From: SeongJae Park @ 2023-10-22 21:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: SeongJae Park, damon, linux-mm, linux-kernel, stable
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(-)
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;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] mm/damon/sysfs-test: add a unit test for damon_sysfs_set_targets()
2023-10-22 21:07 [PATCH 0/2] mm/damon/sysfs: fix unexpected targets adding bug SeongJae Park
2023-10-22 21:07 ` [PATCH 1/2] mm/damon/sysfs: remove requested targets when online-commit inputs SeongJae Park
@ 2023-10-22 21:07 ` SeongJae Park
1 sibling, 0 replies; 4+ messages in thread
From: SeongJae Park @ 2023-10-22 21:07 UTC (permalink / raw)
To: Andrew Morton
Cc: SeongJae Park, Brendan Higgins, damon, linux-mm, kunit-dev,
linux-kselftest, linux-kernel
damon_sysfs_set_targets() had a bug that can result in unexpected memory
usage and monitoring overhead increase. The bug has fixed by a previous
commit. Add a unit test for avoiding a similar bug of future.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/Kconfig | 12 ++++++
mm/damon/sysfs-test.h | 86 +++++++++++++++++++++++++++++++++++++++++++
mm/damon/sysfs.c | 2 +
3 files changed, 100 insertions(+)
create mode 100644 mm/damon/sysfs-test.h
diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
index 436c6b4cb5ec..29f43fbc2eff 100644
--- a/mm/damon/Kconfig
+++ b/mm/damon/Kconfig
@@ -59,6 +59,18 @@ config DAMON_SYSFS
This builds the sysfs interface for DAMON. The user space can use
the interface for arbitrary data access monitoring.
+config DAMON_SYSFS_KUNIT_TEST
+ bool "Test for damon debugfs interface" if !KUNIT_ALL_TESTS
+ depends on DAMON_SYSFS && KUNIT=y
+ default KUNIT_ALL_TESTS
+ help
+ This builds the DAMON sysfs interface Kunit test suite.
+
+ For more information on KUnit and unit tests in general, please refer
+ to the KUnit documentation.
+
+ If unsure, say N.
+
config DAMON_DBGFS
bool "DAMON debugfs interface (DEPRECATED!)"
depends on DAMON_VADDR && DAMON_PADDR && DEBUG_FS
diff --git a/mm/damon/sysfs-test.h b/mm/damon/sysfs-test.h
new file mode 100644
index 000000000000..73bdce2452c1
--- /dev/null
+++ b/mm/damon/sysfs-test.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Data Access Monitor Unit Tests
+ *
+ * Author: SeongJae Park <sj@kernel.org>
+ */
+
+#ifdef CONFIG_DAMON_SYSFS_KUNIT_TEST
+
+#ifndef _DAMON_SYSFS_TEST_H
+#define _DAMON_SYSFS_TEST_H
+
+#include <kunit/test.h>
+
+static unsigned int nr_damon_targets(struct damon_ctx *ctx)
+{
+ struct damon_target *t;
+ unsigned int nr_targets = 0;
+
+ damon_for_each_target(t, ctx)
+ nr_targets++;
+
+ return nr_targets;
+}
+
+static int __damon_sysfs_test_get_any_pid(int min, int max)
+{
+ struct pid *pid;
+ int i;
+
+ for (i = min; i <= max; i++) {
+ pid = find_get_pid(i);
+ if (pid) {
+ put_pid(pid);
+ return i;
+ }
+ }
+ return -1;
+}
+
+static void damon_sysfs_test_set_targets(struct kunit *test)
+{
+ struct damon_sysfs_targets *sysfs_targets;
+ struct damon_sysfs_target *sysfs_target;
+ struct damon_ctx *ctx;
+
+ sysfs_targets = damon_sysfs_targets_alloc();
+ sysfs_targets->nr = 1;
+ sysfs_targets->targets_arr = kmalloc_array(1,
+ sizeof(*sysfs_targets->targets_arr), GFP_KERNEL);
+
+ sysfs_target = damon_sysfs_target_alloc();
+ sysfs_target->pid = __damon_sysfs_test_get_any_pid(12, 100);
+ sysfs_target->regions = damon_sysfs_regions_alloc();
+ sysfs_targets->targets_arr[0] = sysfs_target;
+
+ ctx = damon_new_ctx();
+
+ damon_sysfs_set_targets(ctx, sysfs_targets);
+ KUNIT_EXPECT_EQ(test, 1u, nr_damon_targets(ctx));
+
+ sysfs_target->pid = __damon_sysfs_test_get_any_pid(
+ sysfs_target->pid + 1, 200);
+ damon_sysfs_set_targets(ctx, sysfs_targets);
+ KUNIT_EXPECT_EQ(test, 1u, nr_damon_targets(ctx));
+
+ damon_destroy_ctx(ctx);
+ kfree(sysfs_targets->targets_arr);
+ kfree(sysfs_targets);
+ kfree(sysfs_target);
+}
+
+static struct kunit_case damon_sysfs_test_cases[] = {
+ KUNIT_CASE(damon_sysfs_test_set_targets),
+ {},
+};
+
+static struct kunit_suite damon_sysfs_test_suite = {
+ .name = "damon-sysfs",
+ .test_cases = damon_sysfs_test_cases,
+};
+kunit_test_suite(damon_sysfs_test_suite);
+
+#endif /* _DAMON_SYSFS_TEST_H */
+
+#endif /* CONFIG_DAMON_SYSFS_KUNIT_TEST */
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 5268e8503722..d13e353bee52 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1804,3 +1804,5 @@ static int __init damon_sysfs_init(void)
return err;
}
subsys_initcall(damon_sysfs_init);
+
+#include "sysfs-test.h"
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread