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
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 sj@kernel.org --- 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);
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 sj@kernel.org --- 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;
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 sj@kernel.org
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;
On Wed, 8 Dec 2021 14:29:40 +0800 Xin Hao xhao@linux.alibaba.com 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 100In 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 sj@kernel.org
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 xhao@linux.alibaba.com 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 100In 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 sj@kernel.org
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 23:13:34 +0800 Xin Hao xhao@linux.alibaba.com wrote:
Hi SeongJae:
On 12/8/21 8:49 PM, SeongJae Park wrote:
On Wed, 8 Dec 2021 14:29:40 +0800 Xin Hao xhao@linux.alibaba.com 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 100In 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 sj@kernel.org
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
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 sj@kernel.org --- 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;
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 sj@kernel.org --- 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; }
On Wed, Dec 1, 2021 at 11:05 PM SeongJae Park sj@kernel.org 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 songmuchun@bytedance.com
Thanks.
On Fri, 3 Dec 2021 11:01:36 +0800 Muchun Song songmuchun@bytedance.com wrote:
On Wed, Dec 1, 2021 at 11:05 PM SeongJae Park sj@kernel.org 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 akpm@linux-foundation.org wrote:
On Fri, 3 Dec 2021 11:01:36 +0800 Muchun Song songmuchun@bytedance.com wrote:
On Wed, Dec 1, 2021 at 11:05 PM SeongJae Park sj@kernel.org 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.
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/202111182146.OV3C4uGr-lkp@intel.com/
Reported-by: kernel test robot lkp@intel.com Fixes: 17ccae8bb5c9 ("mm/damon: add kunit tests") Signed-off-by: SeongJae Park sj@kernel.org --- 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); }
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 sj@kernel.org --- 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[] = {
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 sj@kernel.org --- 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 # ===============
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 sj@kernel.org --- 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"
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.16...
Signed-off-by: SeongJae Park sj@kernel.org --- 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
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 sj@kernel.org --- 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 sj@kernel.org + */ + +#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; +}
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 sj@kernel.org --- 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"
linux-kselftest-mirror@lists.linaro.org
-
Andrew Morton -
Muchun Song -
SeongJae Park -
Xin Hao