This patchset contains trivial fixups and improvements for DAMON and its
kunit/kselftest tests.
SeongJae Park (11):
mm/damon/core: Use better timer mechanisms selection threshold
mm/damon/dbgfs: Remove an unnecessary error message
mm/damon/core: Remove unnecessary error messages
mm/damon/vaddr: Remove an unnecessary warning message
mm/damon/vaddr-test: Split a test function having >1024 bytes frame
size
mm/damon/vaddr-test: Remove unnecessary variables
selftests/damon: Skip test if DAMON is running
selftests/damon: Test DAMON enabling with empty target_ids case
selftests/damon: Test wrong DAMOS condition ranges input
selftests/damon: Test debugfs file reads/writes with huge count
selftests/damon: Split test cases
mm/damon/core.c | 14 +---
mm/damon/dbgfs.c | 4 +-
mm/damon/vaddr-test.h | 79 +++++++++----------
mm/damon/vaddr.c | 1 -
tools/testing/selftests/damon/.gitignore | 2 +
tools/testing/selftests/damon/Makefile | 7 +-
.../selftests/damon/_debugfs_common.sh | 52 ++++++++++++
.../testing/selftests/damon/debugfs_attrs.sh | 73 +----------------
.../selftests/damon/debugfs_empty_targets.sh | 13 +++
.../damon/debugfs_huge_count_read_write.sh | 22 ++++++
.../selftests/damon/debugfs_schemes.sh | 19 +++++
.../selftests/damon/debugfs_target_ids.sh | 19 +++++
.../selftests/damon/huge_count_read_write.c | 39 +++++++++
13 files changed, 214 insertions(+), 130 deletions(-)
create mode 100644 tools/testing/selftests/damon/.gitignore
create mode 100644 tools/testing/selftests/damon/_debugfs_common.sh
create mode 100644 tools/testing/selftests/damon/debugfs_empty_targets.sh
create mode 100644 tools/testing/selftests/damon/debugfs_huge_count_read_write.sh
create mode 100644 tools/testing/selftests/damon/debugfs_schemes.sh
create mode 100644 tools/testing/selftests/damon/debugfs_target_ids.sh
create mode 100644 tools/testing/selftests/damon/huge_count_read_write.c
--
2.17.1
DAMON is using hrtimer if requested sleep time is <=100ms, while the
suggested threshold[1] is <=20ms. This commit applies the threshold.
[1] Documentation/timers/timers-howto.rst
Fixes: ee801b7dd7822 ("mm/damon/schemes: activate schemes based on a watermarks mechanism")
Signed-off-by: SeongJae Park <[email protected]>
---
mm/damon/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 8cd8fddc931e..ccc62479549a 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -978,7 +978,8 @@ static unsigned long damos_wmark_wait_us(struct damos *scheme)
static void kdamond_usleep(unsigned long usecs)
{
- if (usecs > 100 * 1000)
+ /* See Documentation/timers/timers-howto.rst for the thresholds */
+ if (usecs > 20 * USEC_PER_MSEC)
schedule_timeout_idle(usecs_to_jiffies(usecs));
else
usleep_idle_range(usecs, usecs + 1);
--
2.17.1
When wrong scheme action is requested via the debugfs interface, DAMON
prints an error message. Because the function returns error code, this
is not really needed. Because the code path is triggered by the user
specified input, this can result in kernel log mistakenly being messy.
To avoid the case, this commit removes the message.
Fixes: af122dd8f3c0 ("mm/damon/dbgfs: support DAMON-based Operation Schemes")
Signed-off-by: SeongJae Park <[email protected]>
---
mm/damon/dbgfs.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index 4bf4204444ab..5b628990ae6e 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -210,10 +210,8 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
&wmarks.low, &parsed);
if (ret != 18)
break;
- if (!damos_action_valid(action)) {
- pr_err("wrong action %d\n", action);
+ if (!damos_action_valid(action))
goto fail;
- }
if (min_sz > max_sz || min_nr_a > max_nr_a || min_age > max_age)
goto fail;
--
2.17.1
The DAMON virtual address space monitoring primitive prints a warning
message for wrong DAMOS action. However, it is not essential as the
code returns appropriate failure in the case. This commit removes the
message to make the log clean.
Fixes: 6dea8add4d28 ("mm/damon/vaddr: support DAMON-based Operation Schemes")
Signed-off-by: SeongJae Park <[email protected]>
---
mm/damon/vaddr.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index 79481f0c2838..a65b1a4d236c 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -617,7 +617,6 @@ static int damon_va_apply_scheme(struct damon_ctx *ctx, struct damon_target *t,
case DAMOS_STAT:
return 0;
default:
- pr_warn("Wrong action %d\n", scheme->action);
return -EINVAL;
}
--
2.17.1
DAMON core prints error messages when damon_target object creation is
failed or wrong monitoring attributes are given. Because appropriate
error code is returned for each case, the messages are not essential.
Also, because the code path can be triggered with user-specified input,
this could result in kernel log mistakenly being messy. To avoid the
case, this commit removes the messages.
Fixes: 4bc05954d007 ("mm/damon: implement a debugfs-based user space interface")
Fixes: b9a6ac4e4ede ("mm/damon: adaptively adjust regions")
Signed-off-by: SeongJae Park <[email protected]>
---
mm/damon/core.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/mm/damon/core.c b/mm/damon/core.c
index ccc62479549a..04b8df7fd9e9 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -282,7 +282,6 @@ int damon_set_targets(struct damon_ctx *ctx,
for (i = 0; i < nr_ids; i++) {
t = damon_new_target(ids[i]);
if (!t) {
- pr_err("Failed to alloc damon_target\n");
/* The caller should do cleanup of the ids itself */
damon_for_each_target_safe(t, next, ctx)
damon_destroy_target(t);
@@ -312,16 +311,10 @@ int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
unsigned long aggr_int, unsigned long primitive_upd_int,
unsigned long min_nr_reg, unsigned long max_nr_reg)
{
- if (min_nr_reg < 3) {
- pr_err("min_nr_regions (%lu) must be at least 3\n",
- min_nr_reg);
+ if (min_nr_reg < 3)
return -EINVAL;
- }
- if (min_nr_reg > max_nr_reg) {
- pr_err("invalid nr_regions. min (%lu) > max (%lu)\n",
- min_nr_reg, max_nr_reg);
+ if (min_nr_reg > max_nr_reg)
return -EINVAL;
- }
ctx->sample_interval = sample_int;
ctx->aggr_interval = aggr_int;
--
2.17.1
On some configuration[1], 'damon_test_split_evenly()' kunit test
function has >1024 bytes frame size, so below build warning is
triggered:
CC mm/damon/vaddr.o
In file included from mm/damon/vaddr.c:672:
mm/damon/vaddr-test.h: In function 'damon_test_split_evenly':
mm/damon/vaddr-test.h:309:1: warning: the frame size of 1064 bytes is larger than 1024 bytes [-Wframe-larger-than=]
309 | }
| ^
This commit fixes the warning by separating the common logics in the
function.
[1] https://lore.kernel.org/linux-mm/[email protected]/
Reported-by: kernel test robot <[email protected]>
Fixes: 17ccae8bb5c9 ("mm/damon: add kunit tests")
Signed-off-by: SeongJae Park <[email protected]>
---
mm/damon/vaddr-test.h | 77 ++++++++++++++++++++++---------------------
1 file changed, 40 insertions(+), 37 deletions(-)
diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
index ecfd0b2ed222..3097ef9c662a 100644
--- a/mm/damon/vaddr-test.h
+++ b/mm/damon/vaddr-test.h
@@ -252,59 +252,62 @@ static void damon_test_apply_three_regions4(struct kunit *test)
new_three_regions, expected, ARRAY_SIZE(expected));
}
-static void damon_test_split_evenly(struct kunit *test)
+static void damon_test_split_evenly_fail(struct kunit *test,
+ unsigned long start, unsigned long end, unsigned int nr_pieces)
{
- struct damon_ctx *c = damon_new_ctx();
- struct damon_target *t;
- struct damon_region *r;
- unsigned long i;
-
- KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(NULL, NULL, 5),
- -EINVAL);
-
- t = damon_new_target(42);
- r = damon_new_region(0, 100);
- KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 0), -EINVAL);
+ struct damon_target *t = damon_new_target(42);
+ struct damon_region *r = damon_new_region(start, end);
damon_add_region(r, t);
- KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 10), 0);
- KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 10u);
+ KUNIT_EXPECT_EQ(test,
+ damon_va_evenly_split_region(t, r, nr_pieces), -EINVAL);
+ KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 1u);
- i = 0;
damon_for_each_region(r, t) {
- KUNIT_EXPECT_EQ(test, r->ar.start, i++ * 10);
- KUNIT_EXPECT_EQ(test, r->ar.end, i * 10);
+ KUNIT_EXPECT_EQ(test, r->ar.start, start);
+ KUNIT_EXPECT_EQ(test, r->ar.end, end);
}
+
damon_free_target(t);
+}
+
+static void damon_test_split_evenly_succ(struct kunit *test,
+ unsigned long start, unsigned long end, unsigned int nr_pieces)
+{
+ struct damon_target *t = damon_new_target(42);
+ struct damon_region *r = damon_new_region(start, end);
+ unsigned long expected_width = (end - start) / nr_pieces;
+ unsigned long i = 0;
- t = damon_new_target(42);
- r = damon_new_region(5, 59);
damon_add_region(r, t);
- KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 5), 0);
- KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
+ KUNIT_EXPECT_EQ(test,
+ damon_va_evenly_split_region(t, r, nr_pieces), 0);
+ KUNIT_EXPECT_EQ(test, damon_nr_regions(t), nr_pieces);
- i = 0;
damon_for_each_region(r, t) {
- if (i == 4)
+ if (i == nr_pieces - 1)
break;
- KUNIT_EXPECT_EQ(test, r->ar.start, 5 + 10 * i++);
- KUNIT_EXPECT_EQ(test, r->ar.end, 5 + 10 * i);
+ KUNIT_EXPECT_EQ(test,
+ r->ar.start, start + i++ * expected_width);
+ KUNIT_EXPECT_EQ(test, r->ar.end, start + i * expected_width);
}
- KUNIT_EXPECT_EQ(test, r->ar.start, 5 + 10 * i);
- KUNIT_EXPECT_EQ(test, r->ar.end, 59ul);
+ KUNIT_EXPECT_EQ(test, r->ar.start, start + i * expected_width);
+ KUNIT_EXPECT_EQ(test, r->ar.end, end);
damon_free_target(t);
+}
- t = damon_new_target(42);
- r = damon_new_region(5, 6);
- damon_add_region(r, t);
- KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 2), -EINVAL);
- KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 1u);
+static void damon_test_split_evenly(struct kunit *test)
+{
+ struct damon_ctx *c = damon_new_ctx();
+
+ KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(NULL, NULL, 5),
+ -EINVAL);
+
+ damon_test_split_evenly_fail(test, 0, 100, 0);
+ damon_test_split_evenly_succ(test, 0, 100, 10);
+ damon_test_split_evenly_succ(test, 5, 59, 5);
+ damon_test_split_evenly_fail(test, 5, 6, 2);
- damon_for_each_region(r, t) {
- KUNIT_EXPECT_EQ(test, r->ar.start, 5ul);
- KUNIT_EXPECT_EQ(test, r->ar.end, 6ul);
- }
- damon_free_target(t);
damon_destroy_ctx(c);
}
--
2.17.1
A couple of test functions in DAMON virtual address space monitoring
primitives implementation has unnecessary damon_ctx variables. This
commit removes those.
Signed-off-by: SeongJae Park <[email protected]>
---
mm/damon/vaddr-test.h | 8 --------
1 file changed, 8 deletions(-)
diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
index 3097ef9c662a..6a1b9272ea12 100644
--- a/mm/damon/vaddr-test.h
+++ b/mm/damon/vaddr-test.h
@@ -135,7 +135,6 @@ static void damon_do_test_apply_three_regions(struct kunit *test,
struct damon_addr_range *three_regions,
unsigned long *expected, int nr_expected)
{
- struct damon_ctx *ctx = damon_new_ctx();
struct damon_target *t;
struct damon_region *r;
int i;
@@ -145,7 +144,6 @@ static void damon_do_test_apply_three_regions(struct kunit *test,
r = damon_new_region(regions[i * 2], regions[i * 2 + 1]);
damon_add_region(r, t);
}
- damon_add_target(ctx, t);
damon_va_apply_three_regions(t, three_regions);
@@ -154,8 +152,6 @@ static void damon_do_test_apply_three_regions(struct kunit *test,
KUNIT_EXPECT_EQ(test, r->ar.start, expected[i * 2]);
KUNIT_EXPECT_EQ(test, r->ar.end, expected[i * 2 + 1]);
}
-
- damon_destroy_ctx(ctx);
}
/*
@@ -298,8 +294,6 @@ static void damon_test_split_evenly_succ(struct kunit *test,
static void damon_test_split_evenly(struct kunit *test)
{
- struct damon_ctx *c = damon_new_ctx();
-
KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(NULL, NULL, 5),
-EINVAL);
@@ -307,8 +301,6 @@ static void damon_test_split_evenly(struct kunit *test)
damon_test_split_evenly_succ(test, 0, 100, 10);
damon_test_split_evenly_succ(test, 5, 59, 5);
damon_test_split_evenly_fail(test, 5, 6, 2);
-
- damon_destroy_ctx(c);
}
static struct kunit_case damon_test_cases[] = {
--
2.17.1
DAMON debugfs didn't check empty targets when starting monitoring, and
the issue is fixed with commit b5ca3e83ddb0 ("mm/damon/dbgfs: add
adaptive_targets list check before enable monitor_on"). To avoid future
regression, this commit adds a test case for that in DAMON selftests.
Signed-off-by: SeongJae Park <[email protected]>
---
tools/testing/selftests/damon/debugfs_attrs.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/testing/selftests/damon/debugfs_attrs.sh b/tools/testing/selftests/damon/debugfs_attrs.sh
index fc80380c59f0..d0916373f310 100644
--- a/tools/testing/selftests/damon/debugfs_attrs.sh
+++ b/tools/testing/selftests/damon/debugfs_attrs.sh
@@ -94,4 +94,13 @@ test_write_succ "$file" "" "$orig_content" "empty input"
test_content "$file" "$orig_content" "" "empty input written"
echo "$orig_content" > "$file"
+# Test empty targets case
+# =======================
+
+orig_target_ids=$(cat "$DBGFS/target_ids")
+echo "" > "$DBGFS/target_ids"
+orig_monitor_on=$(cat "$DBGFS/monitor_on")
+test_write_fail "$DBGFS/monitor_on" "on" "orig_monitor_on" "empty target ids"
+echo "$orig_target_ids" > "$DBGFS/target_ids"
+
echo "PASS"
--
2.17.1
Testing the DAMON debugfs files while DAMON is running makes no sense,
as any write to the debugfs files will fails. This commit makes the
test be skipped in the case.
Signed-off-by: SeongJae Park <[email protected]>
---
tools/testing/selftests/damon/debugfs_attrs.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/testing/selftests/damon/debugfs_attrs.sh b/tools/testing/selftests/damon/debugfs_attrs.sh
index 196b6640bf37..fc80380c59f0 100644
--- a/tools/testing/selftests/damon/debugfs_attrs.sh
+++ b/tools/testing/selftests/damon/debugfs_attrs.sh
@@ -44,6 +44,15 @@ test_content() {
source ./_chk_dependency.sh
+ksft_skip=4
+
+damon_onoff="$DBGFS/monitor_on"
+if [ $(cat "$damon_onoff") = "on" ]
+then
+ echo "monitoring is on"
+ exit $ksft_skip
+fi
+
# Test attrs file
# ===============
--
2.17.1
A patch titled "mm/damon/schemes: add the validity judgment of
thresholds"[1] makes DAMON debugfs interface to validate DAMON scheme
inputs. This commit adds a test case for the validation logic in DAMON
selftests.
[1] https://lore.kernel.org/linux-mm/d78360e52158d786fcbf20bc62c96785742e76d3.1637239568.git.xhao@linux.alibaba.com/
Signed-off-by: SeongJae Park <[email protected]>
---
tools/testing/selftests/damon/debugfs_attrs.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/damon/debugfs_attrs.sh b/tools/testing/selftests/damon/debugfs_attrs.sh
index d0916373f310..1ef118617167 100644
--- a/tools/testing/selftests/damon/debugfs_attrs.sh
+++ b/tools/testing/selftests/damon/debugfs_attrs.sh
@@ -77,6 +77,8 @@ test_write_succ "$file" "1 2 3 4 5 6 4 0 0 0 1 2 3 1 100 3 2 1" \
test_write_fail "$file" "1 2
3 4 5 6 3 0 0 0 1 2 3 1 100 3 2 1" "$orig_content" "multi lines"
test_write_succ "$file" "" "$orig_content" "disabling"
+test_write_fail "$file" "2 1 2 1 10 1 3 10 1 1 1 1 1 1 1 1 2 3" \
+ "$orig_content" "wrong condition ranges"
echo "$orig_content" > "$file"
# Test target_ids file
--
2.17.1
DAMON debugfs interface users were able to trigger warning by writing
some files with arbitrarily large 'count' parameter. The issue is fixed
with commit db7a347b26fe ("mm/damon/dbgfs: use '__GFP_NOWARN' for
user-specified size buffer allocation"). This commit adds a test case
for the issue in DAMON selftests to avoid future regressions.
Signed-off-by: SeongJae Park <[email protected]>
---
tools/testing/selftests/damon/.gitignore | 2 +
tools/testing/selftests/damon/Makefile | 2 +
.../testing/selftests/damon/debugfs_attrs.sh | 18 +++++++++
.../selftests/damon/huge_count_read_write.c | 39 +++++++++++++++++++
4 files changed, 61 insertions(+)
create mode 100644 tools/testing/selftests/damon/.gitignore
create mode 100644 tools/testing/selftests/damon/huge_count_read_write.c
diff --git a/tools/testing/selftests/damon/.gitignore b/tools/testing/selftests/damon/.gitignore
new file mode 100644
index 000000000000..c6c2965a6607
--- /dev/null
+++ b/tools/testing/selftests/damon/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+huge_count_read_write
diff --git a/tools/testing/selftests/damon/Makefile b/tools/testing/selftests/damon/Makefile
index 8a3f2cd9fec0..f0aa954b5d13 100644
--- a/tools/testing/selftests/damon/Makefile
+++ b/tools/testing/selftests/damon/Makefile
@@ -1,6 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for damon selftests
+TEST_GEN_FILES += huge_count_read_write
+
TEST_FILES = _chk_dependency.sh
TEST_PROGS = debugfs_attrs.sh
diff --git a/tools/testing/selftests/damon/debugfs_attrs.sh b/tools/testing/selftests/damon/debugfs_attrs.sh
index 1ef118617167..23a7b48ca7d3 100644
--- a/tools/testing/selftests/damon/debugfs_attrs.sh
+++ b/tools/testing/selftests/damon/debugfs_attrs.sh
@@ -105,4 +105,22 @@ orig_monitor_on=$(cat "$DBGFS/monitor_on")
test_write_fail "$DBGFS/monitor_on" "on" "orig_monitor_on" "empty target ids"
echo "$orig_target_ids" > "$DBGFS/target_ids"
+# Test huge count read write
+# ==========================
+
+dmesg -C
+
+for file in "$DBGFS/"*
+do
+ ./huge_count_read_write "$file"
+done
+
+if dmesg | grep -q WARNING
+then
+ dmesg
+ exit 1
+else
+ exit 0
+fi
+
echo "PASS"
diff --git a/tools/testing/selftests/damon/huge_count_read_write.c b/tools/testing/selftests/damon/huge_count_read_write.c
new file mode 100644
index 000000000000..ad7a6b4cf338
--- /dev/null
+++ b/tools/testing/selftests/damon/huge_count_read_write.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Author: SeongJae Park <[email protected]>
+ */
+
+#include <fcntl.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <stdio.h>
+
+void write_read_with_huge_count(char *file)
+{
+ int filedesc = open(file, O_RDWR);
+ char buf[25];
+ int ret;
+
+ printf("%s %s\n", __func__, file);
+ if (filedesc < 0) {
+ fprintf(stderr, "failed opening %s\n", file);
+ exit(1);
+ }
+
+ write(filedesc, "", 0xfffffffful);
+ perror("after write: ");
+ ret = read(filedesc, buf, 0xfffffffful);
+ perror("after read: ");
+ close(filedesc);
+}
+
+int main(int argc, char *argv[])
+{
+ if (argc != 2) {
+ fprintf(stderr, "Usage: %s <file>\n", argv[0]);
+ exit(1);
+ }
+ write_read_with_huge_count(argv[1]);
+
+ return 0;
+}
--
2.17.1
Currently, the single test program, debugfs.sh, contains all test cases
for DAMON. When one of the cases is failed, finding which case is
failed from the test log is not so easy, and all remaining test will be
skipped. To improve the situation, this commit splits the single
program into small test programs having their own names.
Signed-off-by: SeongJae Park <[email protected]>
---
tools/testing/selftests/damon/Makefile | 5 +-
.../selftests/damon/_debugfs_common.sh | 52 ++++++++
.../testing/selftests/damon/debugfs_attrs.sh | 111 +-----------------
.../selftests/damon/debugfs_empty_targets.sh | 13 ++
.../damon/debugfs_huge_count_read_write.sh | 22 ++++
.../selftests/damon/debugfs_schemes.sh | 19 +++
.../selftests/damon/debugfs_target_ids.sh | 19 +++
7 files changed, 129 insertions(+), 112 deletions(-)
create mode 100644 tools/testing/selftests/damon/_debugfs_common.sh
create mode 100644 tools/testing/selftests/damon/debugfs_empty_targets.sh
create mode 100644 tools/testing/selftests/damon/debugfs_huge_count_read_write.sh
create mode 100644 tools/testing/selftests/damon/debugfs_schemes.sh
create mode 100644 tools/testing/selftests/damon/debugfs_target_ids.sh
diff --git a/tools/testing/selftests/damon/Makefile b/tools/testing/selftests/damon/Makefile
index f0aa954b5d13..937d36ae9a69 100644
--- a/tools/testing/selftests/damon/Makefile
+++ b/tools/testing/selftests/damon/Makefile
@@ -3,7 +3,8 @@
TEST_GEN_FILES += huge_count_read_write
-TEST_FILES = _chk_dependency.sh
-TEST_PROGS = debugfs_attrs.sh
+TEST_FILES = _chk_dependency.sh _debugfs_common.sh
+TEST_PROGS = debugfs_attrs.sh debugfs_schemes.sh debugfs_target_ids.sh
+TEST_PROGS += debugfs_empty_targets.sh debugfs_huge_count_read_write.sh
include ../lib.mk
diff --git a/tools/testing/selftests/damon/_debugfs_common.sh b/tools/testing/selftests/damon/_debugfs_common.sh
new file mode 100644
index 000000000000..48989d4813ae
--- /dev/null
+++ b/tools/testing/selftests/damon/_debugfs_common.sh
@@ -0,0 +1,52 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+test_write_result() {
+ file=$1
+ content=$2
+ orig_content=$3
+ expect_reason=$4
+ expected=$5
+
+ echo "$content" > "$file"
+ if [ $? -ne "$expected" ]
+ then
+ echo "writing $content to $file doesn't return $expected"
+ echo "expected because: $expect_reason"
+ echo "$orig_content" > "$file"
+ exit 1
+ fi
+}
+
+test_write_succ() {
+ test_write_result "$1" "$2" "$3" "$4" 0
+}
+
+test_write_fail() {
+ test_write_result "$1" "$2" "$3" "$4" 1
+}
+
+test_content() {
+ file=$1
+ orig_content=$2
+ expected=$3
+ expect_reason=$4
+
+ content=$(cat "$file")
+ if [ "$content" != "$expected" ]
+ then
+ echo "reading $file expected $expected but $content"
+ echo "expected because: $expect_reason"
+ echo "$orig_content" > "$file"
+ exit 1
+ fi
+}
+
+source ./_chk_dependency.sh
+
+damon_onoff="$DBGFS/monitor_on"
+if [ $(cat "$damon_onoff") = "on" ]
+then
+ echo "monitoring is on"
+ exit $ksft_skip
+fi
diff --git a/tools/testing/selftests/damon/debugfs_attrs.sh b/tools/testing/selftests/damon/debugfs_attrs.sh
index 23a7b48ca7d3..902e312bca89 100644
--- a/tools/testing/selftests/damon/debugfs_attrs.sh
+++ b/tools/testing/selftests/damon/debugfs_attrs.sh
@@ -1,57 +1,7 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
-test_write_result() {
- file=$1
- content=$2
- orig_content=$3
- expect_reason=$4
- expected=$5
-
- echo "$content" > "$file"
- if [ $? -ne "$expected" ]
- then
- echo "writing $content to $file doesn't return $expected"
- echo "expected because: $expect_reason"
- echo "$orig_content" > "$file"
- exit 1
- fi
-}
-
-test_write_succ() {
- test_write_result "$1" "$2" "$3" "$4" 0
-}
-
-test_write_fail() {
- test_write_result "$1" "$2" "$3" "$4" 1
-}
-
-test_content() {
- file=$1
- orig_content=$2
- expected=$3
- expect_reason=$4
-
- content=$(cat "$file")
- if [ "$content" != "$expected" ]
- then
- echo "reading $file expected $expected but $content"
- echo "expected because: $expect_reason"
- echo "$orig_content" > "$file"
- exit 1
- fi
-}
-
-source ./_chk_dependency.sh
-
-ksft_skip=4
-
-damon_onoff="$DBGFS/monitor_on"
-if [ $(cat "$damon_onoff") = "on" ]
-then
- echo "monitoring is on"
- exit $ksft_skip
-fi
+source _debugfs_common.sh
# Test attrs file
# ===============
@@ -65,62 +15,3 @@ test_write_fail "$file" "1 2 3 5 4" "$orig_content" \
"min_nr_regions > max_nr_regions"
test_content "$file" "$orig_content" "1 2 3 4 5" "successfully written"
echo "$orig_content" > "$file"
-
-# Test schemes file
-# =================
-
-file="$DBGFS/schemes"
-orig_content=$(cat "$file")
-
-test_write_succ "$file" "1 2 3 4 5 6 4 0 0 0 1 2 3 1 100 3 2 1" \
- "$orig_content" "valid input"
-test_write_fail "$file" "1 2
-3 4 5 6 3 0 0 0 1 2 3 1 100 3 2 1" "$orig_content" "multi lines"
-test_write_succ "$file" "" "$orig_content" "disabling"
-test_write_fail "$file" "2 1 2 1 10 1 3 10 1 1 1 1 1 1 1 1 2 3" \
- "$orig_content" "wrong condition ranges"
-echo "$orig_content" > "$file"
-
-# Test target_ids file
-# ====================
-
-file="$DBGFS/target_ids"
-orig_content=$(cat "$file")
-
-test_write_succ "$file" "1 2 3 4" "$orig_content" "valid input"
-test_write_succ "$file" "1 2 abc 4" "$orig_content" "still valid input"
-test_content "$file" "$orig_content" "1 2" "non-integer was there"
-test_write_succ "$file" "abc 2 3" "$orig_content" "the file allows wrong input"
-test_content "$file" "$orig_content" "" "wrong input written"
-test_write_succ "$file" "" "$orig_content" "empty input"
-test_content "$file" "$orig_content" "" "empty input written"
-echo "$orig_content" > "$file"
-
-# Test empty targets case
-# =======================
-
-orig_target_ids=$(cat "$DBGFS/target_ids")
-echo "" > "$DBGFS/target_ids"
-orig_monitor_on=$(cat "$DBGFS/monitor_on")
-test_write_fail "$DBGFS/monitor_on" "on" "orig_monitor_on" "empty target ids"
-echo "$orig_target_ids" > "$DBGFS/target_ids"
-
-# Test huge count read write
-# ==========================
-
-dmesg -C
-
-for file in "$DBGFS/"*
-do
- ./huge_count_read_write "$file"
-done
-
-if dmesg | grep -q WARNING
-then
- dmesg
- exit 1
-else
- exit 0
-fi
-
-echo "PASS"
diff --git a/tools/testing/selftests/damon/debugfs_empty_targets.sh b/tools/testing/selftests/damon/debugfs_empty_targets.sh
new file mode 100644
index 000000000000..87aff8083822
--- /dev/null
+++ b/tools/testing/selftests/damon/debugfs_empty_targets.sh
@@ -0,0 +1,13 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+source _debugfs_common.sh
+
+# Test empty targets case
+# =======================
+
+orig_target_ids=$(cat "$DBGFS/target_ids")
+echo "" > "$DBGFS/target_ids"
+orig_monitor_on=$(cat "$DBGFS/monitor_on")
+test_write_fail "$DBGFS/monitor_on" "on" "orig_monitor_on" "empty target ids"
+echo "$orig_target_ids" > "$DBGFS/target_ids"
diff --git a/tools/testing/selftests/damon/debugfs_huge_count_read_write.sh b/tools/testing/selftests/damon/debugfs_huge_count_read_write.sh
new file mode 100644
index 000000000000..922cadac2950
--- /dev/null
+++ b/tools/testing/selftests/damon/debugfs_huge_count_read_write.sh
@@ -0,0 +1,22 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+source _debugfs_common.sh
+
+# Test huge count read write
+# ==========================
+
+dmesg -C
+
+for file in "$DBGFS/"*
+do
+ ./huge_count_read_write "$file"
+done
+
+if dmesg | grep -q WARNING
+then
+ dmesg
+ exit 1
+else
+ exit 0
+fi
diff --git a/tools/testing/selftests/damon/debugfs_schemes.sh b/tools/testing/selftests/damon/debugfs_schemes.sh
new file mode 100644
index 000000000000..5b39ab44731c
--- /dev/null
+++ b/tools/testing/selftests/damon/debugfs_schemes.sh
@@ -0,0 +1,19 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+source _debugfs_common.sh
+
+# Test schemes file
+# =================
+
+file="$DBGFS/schemes"
+orig_content=$(cat "$file")
+
+test_write_succ "$file" "1 2 3 4 5 6 4 0 0 0 1 2 3 1 100 3 2 1" \
+ "$orig_content" "valid input"
+test_write_fail "$file" "1 2
+3 4 5 6 3 0 0 0 1 2 3 1 100 3 2 1" "$orig_content" "multi lines"
+test_write_succ "$file" "" "$orig_content" "disabling"
+test_write_fail "$file" "2 1 2 1 10 1 3 10 1 1 1 1 1 1 1 1 2 3" \
+ "$orig_content" "wrong condition ranges"
+echo "$orig_content" > "$file"
diff --git a/tools/testing/selftests/damon/debugfs_target_ids.sh b/tools/testing/selftests/damon/debugfs_target_ids.sh
new file mode 100644
index 000000000000..49aeabdb0aae
--- /dev/null
+++ b/tools/testing/selftests/damon/debugfs_target_ids.sh
@@ -0,0 +1,19 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+source _debugfs_common.sh
+
+# Test target_ids file
+# ====================
+
+file="$DBGFS/target_ids"
+orig_content=$(cat "$file")
+
+test_write_succ "$file" "1 2 3 4" "$orig_content" "valid input"
+test_write_succ "$file" "1 2 abc 4" "$orig_content" "still valid input"
+test_content "$file" "$orig_content" "1 2" "non-integer was there"
+test_write_succ "$file" "abc 2 3" "$orig_content" "the file allows wrong input"
+test_content "$file" "$orig_content" "" "wrong input written"
+test_write_succ "$file" "" "$orig_content" "empty input"
+test_content "$file" "$orig_content" "" "empty input written"
+echo "$orig_content" > "$file"
--
2.17.1
On Wed, Dec 1, 2021 at 11:05 PM SeongJae Park <[email protected]> wrote:
>
> The DAMON virtual address space monitoring primitive prints a warning
> message for wrong DAMOS action. However, it is not essential as the
> code returns appropriate failure in the case. This commit removes the
> message to make the log clean.
>
> Fixes: 6dea8add4d28 ("mm/damon/vaddr: support DAMON-based Operation Schemes")
I don't think there should be a Fixes tag since it's not a serious bug.
Without this:
Reviewed-by: Muchun Song <[email protected]>
Thanks.
On Fri, 3 Dec 2021 11:01:36 +0800 Muchun Song <[email protected]> wrote:
> On Wed, Dec 1, 2021 at 11:05 PM SeongJae Park <[email protected]> wrote:
> >
> > The DAMON virtual address space monitoring primitive prints a warning
> > message for wrong DAMOS action. However, it is not essential as the
> > code returns appropriate failure in the case. This commit removes the
> > message to make the log clean.
> >
> > Fixes: 6dea8add4d28 ("mm/damon/vaddr: support DAMON-based Operation Schemes")
>
> I don't think there should be a Fixes tag since it's not a serious bug.
"Fixes:" doesn't mean "backport to stable". At least, not for MM
patches. Other subsystems do get their Fixes:-tagged patches
automatically backported.
On Sat, Dec 4, 2021 at 4:44 AM Andrew Morton <[email protected]> wrote:
>
> On Fri, 3 Dec 2021 11:01:36 +0800 Muchun Song <[email protected]> wrote:
>
> > On Wed, Dec 1, 2021 at 11:05 PM SeongJae Park <[email protected]> wrote:
> > >
> > > The DAMON virtual address space monitoring primitive prints a warning
> > > message for wrong DAMOS action. However, it is not essential as the
> > > code returns appropriate failure in the case. This commit removes the
> > > message to make the log clean.
> > >
> > > Fixes: 6dea8add4d28 ("mm/damon/vaddr: support DAMON-based Operation Schemes")
> >
> > I don't think there should be a Fixes tag since it's not a serious bug.
>
> "Fixes:" doesn't mean "backport to stable". At least, not for MM
> patches. Other subsystems do get their Fixes:-tagged patches
> automatically backported.
>
Got it. Thanks.
Hi park:
On 12/1/21 11:04 PM, SeongJae Park wrote:
> When wrong scheme action is requested via the debugfs interface, DAMON
> prints an error message. Because the function returns error code, this
> is not really needed. Because the code path is triggered by the user
> specified input, this can result in kernel log mistakenly being messy.
Completely correct, but there will also be a problem that users can’t
quickly locate where the problem is,
Especially too many parameters need to be written into the interface.
I think it is necessary to add some debugging methods to help users find
the error without polluting the kernel log.
And i have an idea, like this:
in dbgfs, add a last_cmd_stat interface.
# echo "1 2 1 2 1 2 1 2 1 2 100 ..." > schemes
# cat last_cmd_stat
# wrong action 100
In this way, on the one hand, it will not pollute the kernel log, on the
other hand, it will help users find the cause of the operation
interface error.
Park, how do you think of about this idea, if ok, i will send a patch.
> To avoid the case, this commit removes the message
>
> Fixes: af122dd8f3c0 ("mm/damon/dbgfs: support DAMON-based Operation Schemes")
> Signed-off-by: SeongJae Park <[email protected]>
> ---
> mm/damon/dbgfs.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index 4bf4204444ab..5b628990ae6e 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -210,10 +210,8 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
> &wmarks.low, &parsed);
> if (ret != 18)
> break;
> - if (!damos_action_valid(action)) {
> - pr_err("wrong action %d\n", action);
> + if (!damos_action_valid(action))
> goto fail;
> - }
>
> if (min_sz > max_sz || min_nr_a > max_nr_a || min_age > max_age)
> goto fail;
--
Best Regards!
Xin Hao
On Wed, 8 Dec 2021 14:29:40 +0800 Xin Hao <[email protected]> wrote:
Hi Xin,
>
> Hi park:
>
> On 12/1/21 11:04 PM, SeongJae Park wrote:
> > When wrong scheme action is requested via the debugfs interface, DAMON
> > prints an error message. Because the function returns error code, this
> > is not really needed. Because the code path is triggered by the user
> > specified input, this can result in kernel log mistakenly being messy.
>
> Completely correct, but there will also be a problem that users can’t
> quickly locate where the problem is,
>
> Especially too many parameters need to be written into the interface.
>
> I think it is necessary to add some debugging methods to help users find
> the error without polluting the kernel log.
>
> And i have an idea, like this:
>
> in dbgfs, add a last_cmd_stat interface.
>
> # echo "1 2 1 2 1 2 1 2 1 2 100 ..." > schemes
>
> # cat last_cmd_stat
>
> # wrong action 100
>
> In this way, on the one hand, it will not pollute the kernel log, on the
> other hand, it will help users find the cause of the operation
> interface error.
>
> Park, how do you think of about this idea, if ok, i will send a patch.
Thank you always for your great suggestions and efforts! BTW, I prefer to be
called with my first name ;)
I want DAMON kernel code to be as simple and small as possible, while putting
fancy but complicated features for user conveniences in user space tools like
DAMO[1]. In other words, I hope the DAMON debugfs interface to be used as an
interface for such user space tools, not an interface for human hands.
IMHO, implementing the feature you proposed in the kernel could make the code
slightly bigger, while it can easily implemented in user space. I therefore
think the feature would be better to be implemented in user space. If you
could send a pull request of the feature for DAMO, it would be so great.
[1] https://github.com/awslabs/damo
Thanks,
SJ
>
> > To avoid the case, this commit removes the message
> >
> > Fixes: af122dd8f3c0 ("mm/damon/dbgfs: support DAMON-based Operation Schemes")
> > Signed-off-by: SeongJae Park <[email protected]>
> > ---
> > mm/damon/dbgfs.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> > index 4bf4204444ab..5b628990ae6e 100644
> > --- a/mm/damon/dbgfs.c
> > +++ b/mm/damon/dbgfs.c
> > @@ -210,10 +210,8 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
> > &wmarks.low, &parsed);
> > if (ret != 18)
> > break;
> > - if (!damos_action_valid(action)) {
> > - pr_err("wrong action %d\n", action);
> > + if (!damos_action_valid(action))
> > goto fail;
> > - }
> >
> > if (min_sz > max_sz || min_nr_a > max_nr_a || min_age > max_age)
> > goto fail;
>
> --
> Best Regards!
> Xin Hao
>
Hi SeongJae:
On 12/8/21 8:49 PM, SeongJae Park wrote:
> On Wed, 8 Dec 2021 14:29:40 +0800 Xin Hao <[email protected]> wrote:
>
> Hi Xin,
>
>> Hi park:
>>
>> On 12/1/21 11:04 PM, SeongJae Park wrote:
>>> When wrong scheme action is requested via the debugfs interface, DAMON
>>> prints an error message. Because the function returns error code, this
>>> is not really needed. Because the code path is triggered by the user
>>> specified input, this can result in kernel log mistakenly being messy.
>> Completely correct, but there will also be a problem that users can’t
>> quickly locate where the problem is,
>>
>> Especially too many parameters need to be written into the interface.
>>
>> I think it is necessary to add some debugging methods to help users find
>> the error without polluting the kernel log.
>>
>> And i have an idea, like this:
>>
>> in dbgfs, add a last_cmd_stat interface.
>>
>> # echo "1 2 1 2 1 2 1 2 1 2 100 ..." > schemes
>>
>> # cat last_cmd_stat
>>
>> # wrong action 100
>>
>> In this way, on the one hand, it will not pollute the kernel log, on the
>> other hand, it will help users find the cause of the operation
>> interface error.
>>
>> Park, how do you think of about this idea, if ok, i will send a patch.
> Thank you always for your great suggestions and efforts! BTW, I prefer to be
> called with my first name ;)
Ha-Ha, Sorry!
>
> I want DAMON kernel code to be as simple and small as possible, while putting
> fancy but complicated features for user conveniences in user space tools like
> DAMO[1]. In other words, I hope the DAMON debugfs interface to be used as an
> interface for such user space tools, not an interface for human hands.
Ok, I know what you mean.
>
> IMHO, implementing the feature you proposed in the kernel could make the code
> slightly bigger, while it can easily implemented in user space. I therefore
> think the feature would be better to be implemented in user space. If you
> could send a pull request of the feature for DAMO, it would be so great.
Ok, i will do it, But there's a problem here, If the user does not use
the DAMO tools to operate the dbgfs interface,
the operation interface error will still hard to find the cause of errors.
>
> [1] https://github.com/awslabs/damo
>
>
> Thanks,
> SJ
>
>>> To avoid the case, this commit removes the message
>>>
>>> Fixes: af122dd8f3c0 ("mm/damon/dbgfs: support DAMON-based Operation Schemes")
>>> Signed-off-by: SeongJae Park <[email protected]>
>>> ---
>>> mm/damon/dbgfs.c | 4 +---
>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
>>> index 4bf4204444ab..5b628990ae6e 100644
>>> --- a/mm/damon/dbgfs.c
>>> +++ b/mm/damon/dbgfs.c
>>> @@ -210,10 +210,8 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
>>> &wmarks.low, &parsed);
>>> if (ret != 18)
>>> break;
>>> - if (!damos_action_valid(action)) {
>>> - pr_err("wrong action %d\n", action);
>>> + if (!damos_action_valid(action))
>>> goto fail;
>>> - }
>>>
>>> if (min_sz > max_sz || min_nr_a > max_nr_a || min_age > max_age)
>>> goto fail;
>> --
>> Best Regards!
>> Xin Hao
>>
--
Best Regards!
Xin Hao
On Wed, 8 Dec 2021 23:13:34 +0800 Xin Hao <[email protected]> wrote:
> Hi SeongJae:
>
> On 12/8/21 8:49 PM, SeongJae Park wrote:
> > On Wed, 8 Dec 2021 14:29:40 +0800 Xin Hao <[email protected]> wrote:
> >
> > Hi Xin,
> >
> >> Hi park:
> >>
> >> On 12/1/21 11:04 PM, SeongJae Park wrote:
> >>> When wrong scheme action is requested via the debugfs interface, DAMON
> >>> prints an error message. Because the function returns error code, this
> >>> is not really needed. Because the code path is triggered by the user
> >>> specified input, this can result in kernel log mistakenly being messy.
> >> Completely correct, but there will also be a problem that users can’t
> >> quickly locate where the problem is,
> >>
> >> Especially too many parameters need to be written into the interface.
> >>
> >> I think it is necessary to add some debugging methods to help users find
> >> the error without polluting the kernel log.
> >>
> >> And i have an idea, like this:
> >>
> >> in dbgfs, add a last_cmd_stat interface.
> >>
> >> # echo "1 2 1 2 1 2 1 2 1 2 100 ..." > schemes
> >>
> >> # cat last_cmd_stat
> >>
> >> # wrong action 100
> >>
> >> In this way, on the one hand, it will not pollute the kernel log, on the
> >> other hand, it will help users find the cause of the operation
> >> interface error.
> >>
> >> Park, how do you think of about this idea, if ok, i will send a patch.
> > Thank you always for your great suggestions and efforts! BTW, I prefer to be
> > called with my first name ;)
> Ha-Ha, Sorry!
> >
> > I want DAMON kernel code to be as simple and small as possible, while putting
> > fancy but complicated features for user conveniences in user space tools like
> > DAMO[1]. In other words, I hope the DAMON debugfs interface to be used as an
> > interface for such user space tools, not an interface for human hands.
> Ok, I know what you mean.
> >
> > IMHO, implementing the feature you proposed in the kernel could make the code
> > slightly bigger, while it can easily implemented in user space. I therefore
> > think the feature would be better to be implemented in user space. If you
> > could send a pull request of the feature for DAMO, it would be so great.
>
> Ok, i will do it, But there's a problem here, If the user does not use
> the DAMO tools to operate the dbgfs interface,
Well, I don't think that as a problem, but a room for improvement. Maybe we
could improve the documentation.
Thanks,
SJ
>
> the operation interface error will still hard to find the cause of errors.
>
>
> >
> > [1] https://github.com/awslabs/damo
> >
> >
> > Thanks,
> > SJ
> >
> >>> To avoid the case, this commit removes the message
> >>>
> >>> Fixes: af122dd8f3c0 ("mm/damon/dbgfs: support DAMON-based Operation Schemes")
> >>> Signed-off-by: SeongJae Park <[email protected]>
> >>> ---
> >>> mm/damon/dbgfs.c | 4 +---
> >>> 1 file changed, 1 insertion(+), 3 deletions(-)
> >>>
> >>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> >>> index 4bf4204444ab..5b628990ae6e 100644
> >>> --- a/mm/damon/dbgfs.c
> >>> +++ b/mm/damon/dbgfs.c
> >>> @@ -210,10 +210,8 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
> >>> &wmarks.low, &parsed);
> >>> if (ret != 18)
> >>> break;
> >>> - if (!damos_action_valid(action)) {
> >>> - pr_err("wrong action %d\n", action);
> >>> + if (!damos_action_valid(action))
> >>> goto fail;
> >>> - }
> >>>
> >>> if (min_sz > max_sz || min_nr_a > max_nr_a || min_age > max_age)
> >>> goto fail;
> >> --
> >> Best Regards!
> >> Xin Hao
> >>
> --
> Best Regards!
> Xin Hao