[PATCH v2 0/2] mm/damon: Fix some small bugs

[PATCH v2 0/2] mm/damon: Fix some small bugs

[Xin Hao]

From: Xin Hao <bier@B-X3VXMD6M-2058.local>

V1 -> V2
  -- As SeongJae Park advised, put 'ctx->adaptive_targets
  check' into 'dbgfs_monitor_on_write()'

Xin Hao (2):
  mm/damon: Remove unnecessary variable initialization
  mm/damon/dbgfs: Add adaptive_targets list check before enable
    monitor_on

 include/linux/damon.h |  1 +
 mm/damon/core.c       |  5 +++++
 mm/damon/dbgfs.c      | 10 ++++++++--
 mm/damon/vaddr.c      |  2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

--
2.31.0

[PATCH v2 1/2] mm/damon: Remove unnecessary variable initialization

[Xin Hao]

In 'damon_va_apply_three_regions', There is no
need to set variable 'i' as 0

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 mm/damon/vaddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index b8ba44f69db6..90be463efff7 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -305,7 +305,7 @@ static void damon_va_apply_three_regions(struct damon_target *t,
 		struct damon_addr_range bregions[3])
 {
 	struct damon_region *r, *next;
-	unsigned int i = 0;
+	unsigned int i;
 
 	/* Remove regions which are not in the three big regions now */
 	damon_for_each_region_safe(r, next, t) {
-- 
2.31.0

[PATCH v2 2/2] mm/damon/dbgfs: Add adaptive_targets list check before enable monitor_on

[Xin Hao]

When the ctx->adaptive_targets list is empty,
i did some test on monitor_on interface like this.

echo on > /sys/kernel/debug/damon/monitor_on
[  851.988307] damon: kdamond (5390) starts

Though the ctx->adaptive_targets list is empty, but the
kthread_run still be called, and the kdamond.x thread still
be created, this is meaningless.

So there adds a judgment in 'dbgfs_monitor_on_write',
if the ctx->adaptive_targets list is empty, return -EINVAL.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 include/linux/damon.h |  1 +
 mm/damon/core.c       |  5 +++++
 mm/damon/dbgfs.c      | 10 ++++++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 715dadd21f7c..06acf73fe9db 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -316,6 +316,7 @@ void damon_destroy_scheme(struct damos *s);

 struct damon_target *damon_new_target(unsigned long id);
 void damon_add_target(struct damon_ctx *ctx, struct damon_target *t);
+int damon_target_empty(struct damon_ctx *ctx);
 void damon_free_target(struct damon_target *t);
 void damon_destroy_target(struct damon_target *t);
 unsigned int damon_nr_regions(struct damon_target *t);
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 2f6785737902..c651b49249e5 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -156,6 +156,11 @@ void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
 	list_add_tail(&t->list, &ctx->adaptive_targets);
 }

+int damon_target_empty(struct damon_ctx *ctx)
+{
+	return list_empty(&ctx->adaptive_targets);
+}
+
 static void damon_del_target(struct damon_target *t)
 {
 	list_del(&t->list);
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index 38188347d8ab..bac42dd358d1 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -865,9 +865,15 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
 		return -EINVAL;
 	}

-	if (!strncmp(kbuf, "on", count))
+	if (!strncmp(kbuf, "on", count)) {
+		int i;
+
+		for (i = 0; i < dbgfs_nr_ctxs; i++) {
+			if (damon_target_empty(dbgfs_ctxs[i]))
+				return -EINVAL;
+		}
 		err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
-	else if (!strncmp(kbuf, "off", count))
+	} else if (!strncmp(kbuf, "off", count))
 		err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
 	else
 		err = -EINVAL;
--
2.31.0

Re: [PATCH v2 1/2] mm/damon: Remove unnecessary variable initialization

[SeongJae Park]

On Tue, 19 Oct 2021 22:26:03 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> In 'damon_va_apply_three_regions', There is no
> need to set variable 'i' as 0
> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>

Reviewed-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

> ---
>  mm/damon/vaddr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index b8ba44f69db6..90be463efff7 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -305,7 +305,7 @@ static void damon_va_apply_three_regions(struct damon_target *t,
>  		struct damon_addr_range bregions[3])
>  {
>  	struct damon_region *r, *next;
> -	unsigned int i = 0;
> +	unsigned int i;
>  
>  	/* Remove regions which are not in the three big regions now */
>  	damon_for_each_region_safe(r, next, t) {
> -- 
> 2.31.0

Re: [PATCH v2 2/2] mm/damon/dbgfs: Add adaptive_targets list check before enable monitor_on

[SeongJae Park]

Hello Xin,

On Tue, 19 Oct 2021 22:26:04 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> When the ctx->adaptive_targets list is empty,
> i did some test on monitor_on interface like this.

s/i/I/

> 
> echo on > /sys/kernel/debug/damon/monitor_on
> [  851.988307] damon: kdamond (5390) starts

Could you please add four spaces in the beginning of this example terminal
outputs?  Also, I think confirming targets are empty like below before writing
'on' could make this example better.

    # cat /sys/kernel/debug/damon/target_ids
    #

or, like below.

    # echo > /sys/kernel/debug/damon/target_ids

> 
> Though the ctx->adaptive_targets list is empty, but the
> kthread_run still be called, and the kdamond.x thread still
> be created, this is meaningless.
> 
> So there adds a judgment in 'dbgfs_monitor_on_write',
> if the ctx->adaptive_targets list is empty, return -EINVAL.
> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  include/linux/damon.h |  1 +
>  mm/damon/core.c       |  5 +++++
>  mm/damon/dbgfs.c      | 10 ++++++++--
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 715dadd21f7c..06acf73fe9db 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -316,6 +316,7 @@ void damon_destroy_scheme(struct damos *s);
> 
>  struct damon_target *damon_new_target(unsigned long id);
>  void damon_add_target(struct damon_ctx *ctx, struct damon_target *t);
> +int damon_target_empty(struct damon_ctx *ctx);

Seems 'bool' would be better for the return type?

>  void damon_free_target(struct damon_target *t);
>  void damon_destroy_target(struct damon_target *t);
>  unsigned int damon_nr_regions(struct damon_target *t);
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 2f6785737902..c651b49249e5 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -156,6 +156,11 @@ void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
>  	list_add_tail(&t->list, &ctx->adaptive_targets);
>  }
> 
> +int damon_target_empty(struct damon_ctx *ctx)
> +{
> +	return list_empty(&ctx->adaptive_targets);
> +}
> +
>  static void damon_del_target(struct damon_target *t)
>  {
>  	list_del(&t->list);
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index 38188347d8ab..bac42dd358d1 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -865,9 +865,15 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
>  		return -EINVAL;
>  	}
> 
> -	if (!strncmp(kbuf, "on", count))
> +	if (!strncmp(kbuf, "on", count)) {
> +		int i;
> +
> +		for (i = 0; i < dbgfs_nr_ctxs; i++) {
> +			if (damon_target_empty(dbgfs_ctxs[i]))
> +				return -EINVAL;
> +		}
>  		err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
> -	else if (!strncmp(kbuf, "off", count))
> +	} else if (!strncmp(kbuf, "off", count))
>  		err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
>  	else
>  		err = -EINVAL;

Please use braces for all above branches:

https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces


Thanks,
SJ

> --
> 2.31.0

Re: [PATCH v2 2/2] mm/damon/dbgfs: Add adaptive_targets list check before enable monitor_on

[SeongJae Park]

On Tue, 19 Oct 2021 15:42:43 +0000 SeongJae Park <sj@kernel.org> wrote:

> Hello Xin,
> 
> On Tue, 19 Oct 2021 22:26:04 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
> 
> > When the ctx->adaptive_targets list is empty,
> > i did some test on monitor_on interface like this.
> 
> s/i/I/
> 
> > 
> > echo on > /sys/kernel/debug/damon/monitor_on
> > [  851.988307] damon: kdamond (5390) starts
> 
> Could you please add four spaces in the beginning of this example terminal
> outputs?  Also, I think confirming targets are empty like below before writing
> 'on' could make this example better.
> 
>     # cat /sys/kernel/debug/damon/target_ids
>     #
> 
> or, like below.
> 
>     # echo > /sys/kernel/debug/damon/target_ids
> 
> > 
> > Though the ctx->adaptive_targets list is empty, but the
> > kthread_run still be called, and the kdamond.x thread still
> > be created, this is meaningless.
> > 
> > So there adds a judgment in 'dbgfs_monitor_on_write',
> > if the ctx->adaptive_targets list is empty, return -EINVAL.
> > 
> > Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> > ---
> >  include/linux/damon.h |  1 +
> >  mm/damon/core.c       |  5 +++++
> >  mm/damon/dbgfs.c      | 10 ++++++++--
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 715dadd21f7c..06acf73fe9db 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -316,6 +316,7 @@ void damon_destroy_scheme(struct damos *s);
> > 
> >  struct damon_target *damon_new_target(unsigned long id);
> >  void damon_add_target(struct damon_ctx *ctx, struct damon_target *t);
> > +int damon_target_empty(struct damon_ctx *ctx);
> 
> Seems 'bool' would be better for the return type?

Also, I think 'damon_targets_empty()' (s/target/targets/) might be a better
naming?


Thanks,
SJ

> 
> >  void damon_free_target(struct damon_target *t);
> >  void damon_destroy_target(struct damon_target *t);
> >  unsigned int damon_nr_regions(struct damon_target *t);
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 2f6785737902..c651b49249e5 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -156,6 +156,11 @@ void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
> >  	list_add_tail(&t->list, &ctx->adaptive_targets);
> >  }
> > 
> > +int damon_target_empty(struct damon_ctx *ctx)
> > +{
> > +	return list_empty(&ctx->adaptive_targets);
> > +}
> > +
> >  static void damon_del_target(struct damon_target *t)
> >  {
> >  	list_del(&t->list);
> > diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> > index 38188347d8ab..bac42dd358d1 100644
> > --- a/mm/damon/dbgfs.c
> > +++ b/mm/damon/dbgfs.c
> > @@ -865,9 +865,15 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
> >  		return -EINVAL;
> >  	}
> > 
> > -	if (!strncmp(kbuf, "on", count))
> > +	if (!strncmp(kbuf, "on", count)) {
> > +		int i;
> > +
> > +		for (i = 0; i < dbgfs_nr_ctxs; i++) {
> > +			if (damon_target_empty(dbgfs_ctxs[i]))
> > +				return -EINVAL;
> > +		}
> >  		err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
> > -	else if (!strncmp(kbuf, "off", count))
> > +	} else if (!strncmp(kbuf, "off", count))
> >  		err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
> >  	else
> >  		err = -EINVAL;
> 
> Please use braces for all above branches:
> 
> https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
> 
> 
> Thanks,
> SJ
> 
> > --
> > 2.31.0

Re: [PATCH v2 2/2] mm/damon/dbgfs: Add adaptive_targets list check before enable monitor_on

[Xin Hao]

On 2021/10/19 下午11:42, SeongJae Park wrote:
> Hello Xin,
>
> On Tue, 19 Oct 2021 22:26:04 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>
>> When the ctx->adaptive_targets list is empty,
>> i did some test on monitor_on interface like this.
> s/i/I/
thanks
>> echo on > /sys/kernel/debug/damon/monitor_on
>> [  851.988307] damon: kdamond (5390) starts
> Could you please add four spaces in the beginning of this example terminal
> outputs?  Also, I think confirming targets are empty like below before writing
> 'on' could make this example better.
>
>      # cat /sys/kernel/debug/damon/target_ids
>      #
>
> or, like below.
>
>      # echo > /sys/kernel/debug/damon/target_ids
Oh! thank you so much, i will fix it.
>
>> Though the ctx->adaptive_targets list is empty, but the
>> kthread_run still be called, and the kdamond.x thread still
>> be created, this is meaningless.
>>
>> So there adds a judgment in 'dbgfs_monitor_on_write',
>> if the ctx->adaptive_targets list is empty, return -EINVAL.
>>
>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>> ---
>>   include/linux/damon.h |  1 +
>>   mm/damon/core.c       |  5 +++++
>>   mm/damon/dbgfs.c      | 10 ++++++++--
>>   3 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>> index 715dadd21f7c..06acf73fe9db 100644
>> --- a/include/linux/damon.h
>> +++ b/include/linux/damon.h
>> @@ -316,6 +316,7 @@ void damon_destroy_scheme(struct damos *s);
>>
>>   struct damon_target *damon_new_target(unsigned long id);
>>   void damon_add_target(struct damon_ctx *ctx, struct damon_target *t);
>> +int damon_target_empty(struct damon_ctx *ctx);
> Seems 'bool' would be better for the return type?
ok!
>
>>   void damon_free_target(struct damon_target *t);
>>   void damon_destroy_target(struct damon_target *t);
>>   unsigned int damon_nr_regions(struct damon_target *t);
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index 2f6785737902..c651b49249e5 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -156,6 +156,11 @@ void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
>>   	list_add_tail(&t->list, &ctx->adaptive_targets);
>>   }
>>
>> +int damon_target_empty(struct damon_ctx *ctx)
>> +{
>> +	return list_empty(&ctx->adaptive_targets);
>> +}
>> +
>>   static void damon_del_target(struct damon_target *t)
>>   {
>>   	list_del(&t->list);
>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
>> index 38188347d8ab..bac42dd358d1 100644
>> --- a/mm/damon/dbgfs.c
>> +++ b/mm/damon/dbgfs.c
>> @@ -865,9 +865,15 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
>>   		return -EINVAL;
>>   	}
>>
>> -	if (!strncmp(kbuf, "on", count))
>> +	if (!strncmp(kbuf, "on", count)) {
>> +		int i;
>> +
>> +		for (i = 0; i < dbgfs_nr_ctxs; i++) {
>> +			if (damon_target_empty(dbgfs_ctxs[i]))
>> +				return -EINVAL;
>> +		}
>>   		err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
>> -	else if (!strncmp(kbuf, "off", count))
>> +	} else if (!strncmp(kbuf, "off", count))
>>   		err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
>>   	else
>>   		err = -EINVAL;
> Please use braces for all above branches:
Ok，i will correct it.
>
> https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
>
>
> Thanks,
> SJ
>
>> --
>> 2.31.0

-- 
Best Regards!
Xin Hao

Re: [PATCH v2 2/2] mm/damon/dbgfs: Add adaptive_targets list check before enable monitor_on

[Xin Hao]

On 2021/10/19 下午11:49, SeongJae Park wrote:
> On Tue, 19 Oct 2021 15:42:43 +0000 SeongJae Park <sj@kernel.org> wrote:
>
>> Hello Xin,
>>
>> On Tue, 19 Oct 2021 22:26:04 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>>
>>> When the ctx->adaptive_targets list is empty,
>>> i did some test on monitor_on interface like this.
>> s/i/I/
>>
>>> echo on > /sys/kernel/debug/damon/monitor_on
>>> [  851.988307] damon: kdamond (5390) starts
>> Could you please add four spaces in the beginning of this example terminal
>> outputs?  Also, I think confirming targets are empty like below before writing
>> 'on' could make this example better.
>>
>>      # cat /sys/kernel/debug/damon/target_ids
>>      #
>>
>> or, like below.
>>
>>      # echo > /sys/kernel/debug/damon/target_ids
>>
>>> Though the ctx->adaptive_targets list is empty, but the
>>> kthread_run still be called, and the kdamond.x thread still
>>> be created, this is meaningless.
>>>
>>> So there adds a judgment in 'dbgfs_monitor_on_write',
>>> if the ctx->adaptive_targets list is empty, return -EINVAL.
>>>
>>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>>> ---
>>>   include/linux/damon.h |  1 +
>>>   mm/damon/core.c       |  5 +++++
>>>   mm/damon/dbgfs.c      | 10 ++++++++--
>>>   3 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>>> index 715dadd21f7c..06acf73fe9db 100644
>>> --- a/include/linux/damon.h
>>> +++ b/include/linux/damon.h
>>> @@ -316,6 +316,7 @@ void damon_destroy_scheme(struct damos *s);
>>>
>>>   struct damon_target *damon_new_target(unsigned long id);
>>>   void damon_add_target(struct damon_ctx *ctx, struct damon_target *t);
>>> +int damon_target_empty(struct damon_ctx *ctx);
>> Seems 'bool' would be better for the return type?
> Also, I think 'damon_targets_empty()' (s/target/targets/) might be a better
> naming?
Yeah, thanks.
>
>
> Thanks,
> SJ
>
>>>   void damon_free_target(struct damon_target *t);
>>>   void damon_destroy_target(struct damon_target *t);
>>>   unsigned int damon_nr_regions(struct damon_target *t);
>>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>>> index 2f6785737902..c651b49249e5 100644
>>> --- a/mm/damon/core.c
>>> +++ b/mm/damon/core.c
>>> @@ -156,6 +156,11 @@ void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
>>>   	list_add_tail(&t->list, &ctx->adaptive_targets);
>>>   }
>>>
>>> +int damon_target_empty(struct damon_ctx *ctx)
>>> +{
>>> +	return list_empty(&ctx->adaptive_targets);
>>> +}
>>> +
>>>   static void damon_del_target(struct damon_target *t)
>>>   {
>>>   	list_del(&t->list);
>>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
>>> index 38188347d8ab..bac42dd358d1 100644
>>> --- a/mm/damon/dbgfs.c
>>> +++ b/mm/damon/dbgfs.c
>>> @@ -865,9 +865,15 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
>>>   		return -EINVAL;
>>>   	}
>>>
>>> -	if (!strncmp(kbuf, "on", count))
>>> +	if (!strncmp(kbuf, "on", count)) {
>>> +		int i;
>>> +
>>> +		for (i = 0; i < dbgfs_nr_ctxs; i++) {
>>> +			if (damon_target_empty(dbgfs_ctxs[i]))
>>> +				return -EINVAL;
>>> +		}
>>>   		err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
>>> -	else if (!strncmp(kbuf, "off", count))
>>> +	} else if (!strncmp(kbuf, "off", count))
>>>   		err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
>>>   	else
>>>   		err = -EINVAL;
>> Please use braces for all above branches:
>>
>> https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
>>
>>
>> Thanks,
>> SJ
>>
>>> --
>>> 2.31.0

-- 
Best Regards!
Xin Hao