[PATCH V2] mm/damon/dbgfs: Optimize target_ids interface write operation

[PATCH V2] mm/damon/dbgfs: Optimize target_ids interface write operation

[Xin Hao]

When we want to clear previously set target ids,
For example, it works as below now:
    # echo 42 > target_ids
    # cat target_ids
    42
    # echo > target_ids
    # cat target_ids

But in 'dbgfs_target_ids_write', there is no need to
execute other codes, except call 'damon_set_targets'
to clear previously set target ids. So there adds
the 'nr_targets' judgment, if the value is 0, just
call 'damon_set_targets', and then return.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 mm/damon/dbgfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index a02cf6bee8e8..1d83f4138fad 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -362,6 +362,12 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
 		goto out;
 	}

+	if (!nr_targets) {
+		/* remove targets with previously-set primitive */
+		damon_set_targets(ctx, NULL, 0);
+		goto free_targets_out;
+	}
+
 	if (id_is_pid) {
 		for (i = 0; i < nr_targets; i++) {
 			targets[i] = (unsigned long)find_get_pid(
--
2.31.0

Re: [PATCH V2] mm/damon/dbgfs: Optimize target_ids interface write operation

[SeongJae Park]

Hello Xin,

On Fri, 22 Oct 2021 00:44:16 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> When we want to clear previously set target ids,
> For example, it works as below now:
>     # echo 42 > target_ids
>     # cat target_ids
>     42
>     # echo > target_ids
>     # cat target_ids
> 
> But in 'dbgfs_target_ids_write', there is no need to
> execute other codes, except call 'damon_set_targets'
> to clear previously set target ids. So there adds
> the 'nr_targets' judgment, if the value is 0, just
> call 'damon_set_targets', and then return.

It's true that it executes some unnecessary code.  However, I unsure if that is
a problem, as the code that will be additionally executed in this case are
quite simple ones, and therefore not supposed to incur viewable overhead.
After all, this is not a performance critical path.

Thanks,
SJ

> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  mm/damon/dbgfs.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index a02cf6bee8e8..1d83f4138fad 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -362,6 +362,12 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
>  		goto out;
>  	}
> 
> +	if (!nr_targets) {
> +		/* remove targets with previously-set primitive */
> +		damon_set_targets(ctx, NULL, 0);
> +		goto free_targets_out;
> +	}
> +
>  	if (id_is_pid) {
>  		for (i = 0; i < nr_targets; i++) {
>  			targets[i] = (unsigned long)find_get_pid(
> --
> 2.31.0

Re: [PATCH V2] mm/damon/dbgfs: Optimize target_ids interface write operation

[Xin Hao]

On 2021/10/22 上午1:30, SeongJae Park wrote:
> Hello Xin,
>
> On Fri, 22 Oct 2021 00:44:16 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>
>> When we want to clear previously set target ids,
>> For example, it works as below now:
>>      # echo 42 > target_ids
>>      # cat target_ids
>>      42
>>      # echo > target_ids
>>      # cat target_ids
>>
>> But in 'dbgfs_target_ids_write', there is no need to
>> execute other codes, except call 'damon_set_targets'
>> to clear previously set target ids. So there adds
>> the 'nr_targets' judgment, if the value is 0, just
>> call 'damon_set_targets', and then return.
> It's true that it executes some unnecessary code.  However, I unsure if that is
> a problem, as the code that will be additionally executed in this case are
> quite simple ones, and therefore not supposed to incur viewable overhead.
> After all, this is not a performance critical path.

Thank you for your detailed explanation. I may not describe it clearly, 
making you think that i am making this

modification to improve performance，I just want to avoid irrelevant code 
execution, thank you so much.

>
> Thanks,
> SJ
>
>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>> ---
>>   mm/damon/dbgfs.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
>> index a02cf6bee8e8..1d83f4138fad 100644
>> --- a/mm/damon/dbgfs.c
>> +++ b/mm/damon/dbgfs.c
>> @@ -362,6 +362,12 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
>>   		goto out;
>>   	}
>>
>> +	if (!nr_targets) {
>> +		/* remove targets with previously-set primitive */
>> +		damon_set_targets(ctx, NULL, 0);
>> +		goto free_targets_out;
>> +	}
>> +
>>   	if (id_is_pid) {
>>   		for (i = 0; i < nr_targets; i++) {
>>   			targets[i] = (unsigned long)find_get_pid(
>> --
>> 2.31.0

-- 
Best Regards!
Xin Hao

Re: [PATCH V2] mm/damon/dbgfs: Optimize target_ids interface write operation

[SeongJae Park]

On Fri, 22 Oct 2021 10:43:22 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> 
> On 2021/10/22 上午1:30, SeongJae Park wrote:
> > Hello Xin,
> >
> > On Fri, 22 Oct 2021 00:44:16 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
> >
> >> When we want to clear previously set target ids,
> >> For example, it works as below now:
> >>      # echo 42 > target_ids
> >>      # cat target_ids
> >>      42
> >>      # echo > target_ids
> >>      # cat target_ids
> >>
> >> But in 'dbgfs_target_ids_write', there is no need to
> >> execute other codes, except call 'damon_set_targets'
> >> to clear previously set target ids. So there adds
> >> the 'nr_targets' judgment, if the value is 0, just
> >> call 'damon_set_targets', and then return.
> > It's true that it executes some unnecessary code.  However, I unsure if that is
> > a problem, as the code that will be additionally executed in this case are
> > quite simple ones, and therefore not supposed to incur viewable overhead.
> > After all, this is not a performance critical path.
> 
> Thank you for your detailed explanation. I may not describe it clearly, 
> making you think that i am making this
> 
> modification to improve performance，I just want to avoid irrelevant code 
> execution, thank you so much.

I guess I didn't make my point clear enough, sorry.  My concern in this patch
is the fact that it is adding more code.  IMHO, as the code is already working
correctly and benefit of this change is quite subtle as you also agreed, adding
the code here doesn't seem worthy but only making it harder to maintain, to me.

If I'm missing something, please let me know.


Thanks,
SJ

[...]