* [PATCH 01/11] mm/damon/core: Use better timer mechanisms selection threshold
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message SeongJae Park
` (9 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
2021-12-01 15:04 ` [PATCH 01/11] mm/damon/core: Use better timer mechanisms selection threshold SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-08 6:29 ` Xin Hao
2021-12-01 15:04 ` [PATCH 03/11] mm/damon/core: Remove unnecessary error messages SeongJae Park
` (8 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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;
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message
2021-12-01 15:04 ` [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message SeongJae Park
@ 2021-12-08 6:29 ` Xin Hao
2021-12-08 12:49 ` SeongJae Park
0 siblings, 1 reply; 19+ messages in thread
From: Xin Hao @ 2021-12-08 6:29 UTC (permalink / raw)
To: SeongJae Park, akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel
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;
--
Best Regards!
Xin Hao
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message
2021-12-08 6:29 ` Xin Hao
@ 2021-12-08 12:49 ` SeongJae Park
2021-12-08 15:13 ` Xin Hao
0 siblings, 1 reply; 19+ messages in thread
From: SeongJae Park @ 2021-12-08 12:49 UTC (permalink / raw)
To: Xin Hao
Cc: SeongJae Park, akpm, shuah, brendanhiggins, linux-mm,
linux-kselftest, linux-kernel
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 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 <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
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message
2021-12-08 12:49 ` SeongJae Park
@ 2021-12-08 15:13 ` Xin Hao
2021-12-08 16:48 ` SeongJae Park
0 siblings, 1 reply; 19+ messages in thread
From: Xin Hao @ 2021-12-08 15:13 UTC (permalink / raw)
To: SeongJae Park
Cc: akpm, shuah, brendanhiggins, linux-mm, linux-kselftest,
linux-kernel
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 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 <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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message
2021-12-08 15:13 ` Xin Hao
@ 2021-12-08 16:48 ` SeongJae Park
0 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-08 16:48 UTC (permalink / raw)
To: Xin Hao
Cc: SeongJae Park, akpm, shuah, brendanhiggins, linux-mm,
linux-kselftest, linux-kernel
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 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 <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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 03/11] mm/damon/core: Remove unnecessary error messages
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
2021-12-01 15:04 ` [PATCH 01/11] mm/damon/core: Use better timer mechanisms selection threshold SeongJae Park
2021-12-01 15:04 ` [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 04/11] mm/damon/vaddr: Remove an unnecessary warning message SeongJae Park
` (7 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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;
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 04/11] mm/damon/vaddr: Remove an unnecessary warning message
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (2 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 03/11] mm/damon/core: Remove unnecessary error messages SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-03 3:01 ` Muchun Song
2021-12-01 15:04 ` [PATCH 05/11] mm/damon/vaddr-test: Split a test function having >1024 bytes frame size SeongJae Park
` (6 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 04/11] mm/damon/vaddr: Remove an unnecessary warning message
2021-12-01 15:04 ` [PATCH 04/11] mm/damon/vaddr: Remove an unnecessary warning message SeongJae Park
@ 2021-12-03 3:01 ` Muchun Song
2021-12-03 20:44 ` Andrew Morton
0 siblings, 1 reply; 19+ messages in thread
From: Muchun Song @ 2021-12-03 3:01 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrew Morton, Shuah Khan, brendanhiggins,
Linux Memory Management List, linux-kselftest, LKML
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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 04/11] mm/damon/vaddr: Remove an unnecessary warning message
2021-12-03 3:01 ` Muchun Song
@ 2021-12-03 20:44 ` Andrew Morton
2021-12-04 2:37 ` Muchun Song
0 siblings, 1 reply; 19+ messages in thread
From: Andrew Morton @ 2021-12-03 20:44 UTC (permalink / raw)
To: Muchun Song
Cc: SeongJae Park, Shuah Khan, brendanhiggins,
Linux Memory Management List, linux-kselftest, LKML
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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 04/11] mm/damon/vaddr: Remove an unnecessary warning message
2021-12-03 20:44 ` Andrew Morton
@ 2021-12-04 2:37 ` Muchun Song
0 siblings, 0 replies; 19+ messages in thread
From: Muchun Song @ 2021-12-04 2:37 UTC (permalink / raw)
To: Andrew Morton
Cc: SeongJae Park, Shuah Khan, brendanhiggins,
Linux Memory Management List, linux-kselftest, LKML
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 05/11] mm/damon/vaddr-test: Split a test function having >1024 bytes frame size
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (3 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 04/11] mm/damon/vaddr: Remove an unnecessary warning message SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 06/11] mm/damon/vaddr-test: Remove unnecessary variables SeongJae Park
` (5 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 06/11] mm/damon/vaddr-test: Remove unnecessary variables
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (4 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 05/11] mm/damon/vaddr-test: Split a test function having >1024 bytes frame size SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 07/11] selftests/damon: Skip test if DAMON is running SeongJae Park
` (4 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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[] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 07/11] selftests/damon: Skip test if DAMON is running
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (5 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 06/11] mm/damon/vaddr-test: Remove unnecessary variables SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 08/11] selftests/damon: Test DAMON enabling with empty target_ids case SeongJae Park
` (3 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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
# ===============
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 08/11] selftests/damon: Test DAMON enabling with empty target_ids case
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (6 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 07/11] selftests/damon: Skip test if DAMON is running SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 09/11] selftests/damon: Test wrong DAMOS condition ranges input SeongJae Park
` (2 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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"
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 09/11] selftests/damon: Test wrong DAMOS condition ranges input
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (7 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 08/11] selftests/damon: Test DAMON enabling with empty target_ids case SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 10/11] selftests/damon: Test debugfs file reads/writes with huge count SeongJae Park
2021-12-01 15:04 ` [PATCH 11/11] selftests/damon: Split test cases SeongJae Park
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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 <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
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 10/11] selftests/damon: Test debugfs file reads/writes with huge count
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (8 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 09/11] selftests/damon: Test wrong DAMOS condition ranges input SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
2021-12-01 15:04 ` [PATCH 11/11] selftests/damon: Split test cases SeongJae Park
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 11/11] selftests/damon: Split test cases
2021-12-01 15:04 [PATCH 00/11] mm/damon: Trivial fixups and improvements SeongJae Park
` (9 preceding siblings ...)
2021-12-01 15:04 ` [PATCH 10/11] selftests/damon: Test debugfs file reads/writes with huge count SeongJae Park
@ 2021-12-01 15:04 ` SeongJae Park
10 siblings, 0 replies; 19+ messages in thread
From: SeongJae Park @ 2021-12-01 15:04 UTC (permalink / raw)
To: akpm
Cc: shuah, brendanhiggins, linux-mm, linux-kselftest, linux-kernel,
SeongJae Park
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"
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread