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

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

[Xin Hao]

When writing some pids to target_ids interface, calling scanf()
to get 'id' may be failed. If the value of '*nr_ids' is 0 at this time,
there is no need to return 'ids' here, we just need to release it and
return NULL pointer to improve related code operation efficiency.

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

diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index a02cf6bee8e8..2d77bf579ffb 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -308,21 +308,25 @@ static unsigned long *str_to_target_ids(const char *str, ssize_t len,
 	unsigned long *ids;
 	const int max_nr_ids = 32;
 	unsigned long id;
-	int pos = 0, parsed, ret;
+	int pos = 0, parsed;
 
 	*nr_ids = 0;
 	ids = kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL);
 	if (!ids)
 		return NULL;
 	while (*nr_ids < max_nr_ids && pos < len) {
-		ret = sscanf(&str[pos], "%lu%n", &id, &parsed);
-		pos += parsed;
-		if (ret != 1)
+		if (sscanf(&str[pos], "%lu%n", &id, &parsed) != 1)
 			break;
+		pos += parsed;
 		ids[*nr_ids] = id;
 		*nr_ids += 1;
 	}
 
+	if (!*nr_ids) {
+		kfree(ids);
+		return NULL;
+	}
+
 	return ids;
 }
 
-- 
2.31.0

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

[SeongJae Park]

Hello Xin,

On Thu, 21 Oct 2021 16:56:11 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> When writing some pids to target_ids interface, calling scanf()
> to get 'id' may be failed. If the value of '*nr_ids' is 0 at this time,
> there is no need to return 'ids' here, we just need to release it and
> return NULL pointer to improve related code operation efficiency.

Thank you for the patch!  But, I don't think this patch makes sense, because
the case (*nr_ids == 0) means not error but an ask to remove 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, with your patch, the behavior will be changed as below:

    # echo 42 > target_ids
    # cat target_ids
    42
    # echo > target_ids
    bash: echo: write error: Cannot allocate memory
    # cat target_ids
    42
    #

Also, this patch makes the DAMON selftest fails as below:

    $ sudo make -C tools/testing/selftests/damon run_tests
    make: Entering directory '/home/sjpark/linux/tools/testing/selftests/damon'
    TAP version 13
    1..2
    # selftests: damon: debugfs_attrs.sh
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Cannot allocate memory
    # writing abc 2 3 to /sys/kernel/debug/damon/target_ids doesn't return 0
    # expected because: the file allows wrong input
    not ok 1 selftests: damon: debugfs_attrs.sh # exit=1

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


Thanks,
SJ

> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  mm/damon/dbgfs.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index a02cf6bee8e8..2d77bf579ffb 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -308,21 +308,25 @@ static unsigned long *str_to_target_ids(const char *str, ssize_t len,
>  	unsigned long *ids;
>  	const int max_nr_ids = 32;
>  	unsigned long id;
> -	int pos = 0, parsed, ret;
> +	int pos = 0, parsed;
>  
>  	*nr_ids = 0;
>  	ids = kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL);
>  	if (!ids)
>  		return NULL;
>  	while (*nr_ids < max_nr_ids && pos < len) {
> -		ret = sscanf(&str[pos], "%lu%n", &id, &parsed);
> -		pos += parsed;
> -		if (ret != 1)
> +		if (sscanf(&str[pos], "%lu%n", &id, &parsed) != 1)
>  			break;
> +		pos += parsed;
>  		ids[*nr_ids] = id;
>  		*nr_ids += 1;
>  	}
>  
> +	if (!*nr_ids) {
> +		kfree(ids);
> +		return NULL;
> +	}
> +
>  	return ids;
>  }
>  
> -- 
> 2.31.0

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

[Xin Hao]

On 2021/10/21 下午5:45, SeongJae Park wrote:
> Hello Xin,
>
> On Thu, 21 Oct 2021 16:56:11 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>
>> When writing some pids to target_ids interface, calling scanf()
>> to get 'id' may be failed. If the value of '*nr_ids' is 0 at this time,
>> there is no need to return 'ids' here, we just need to release it and
>> return NULL pointer to improve related code operation efficiency.
> Thank you for the patch!  But, I don't think this patch makes sense, because
> the case (*nr_ids == 0) means not error but an ask to remove 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, with your patch, the behavior will be changed as below:
>
>      # echo 42 > target_ids
>      # cat target_ids
>      42
>      # echo > target_ids
>      bash: echo: write error: Cannot allocate memory
>      # cat target_ids
>      42
>      #

Hi Park:

     I did not use the lastest code to test, so I ignored that it calls 
'damon_set_targets'

to clear previously set target ids, Sorry.

     I think if we want only want to clear the previously set target 
ids, this codes are all no need to execute, except call

'damon_set_targets(ctx, NULL, 0);'

----------------------------------------------------------

         if (id_is_pid) {
                 for (i = 0; i < nr_targets; i++) {
                         targets[i] = (unsigned long)find_get_pid(
                                         (int)targets[i]);
                         if (!targets[i]) {
                                 dbgfs_put_pids(targets, i);
                                 ret = -EINVAL;
                                 goto free_targets_out;
                         }
                 }
         }

         mutex_lock(&ctx->kdamond_lock);
         if (ctx->kdamond) {
                 if (id_is_pid)
                         dbgfs_put_pids(targets, nr_targets);
                 ret = -EBUSY;
                 goto unlock_out;
         }

       . . .

         /* Configure the context for the address space type */
         if (id_is_pid)
                 damon_va_set_primitives(ctx);
         else
                 damon_pa_set_primitives(ctx);

         ret = damon_set_targets(ctx, targets, nr_targets);
         if (ret) {
                 if (id_is_pid)
                         dbgfs_put_pids(targets, nr_targets);
         } else {
                 ret = count;

>
> Also, this patch makes the DAMON selftest fails as below:
>
>      $ sudo make -C tools/testing/selftests/damon run_tests
>      make: Entering directory '/home/sjpark/linux/tools/testing/selftests/damon'
>      TAP version 13
>      1..2
>      # selftests: damon: debugfs_attrs.sh
>      # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
>      # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
>      # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
>      # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
>      # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
>      # ./debugfs_attrs.sh: line 11: echo: write error: Cannot allocate memory
>      # writing abc 2 3 to /sys/kernel/debug/damon/target_ids doesn't return 0
>      # expected because: the file allows wrong input
>      not ok 1 selftests: damon: debugfs_attrs.sh # exit=1
>
> If I'm missing something, please let me know.
I will post a new patch to fix the problems you encountered in the above 
test, thank you so much!
>
>
> Thanks,
> SJ
>
>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>> ---
>>   mm/damon/dbgfs.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
>> index a02cf6bee8e8..2d77bf579ffb 100644
>> --- a/mm/damon/dbgfs.c
>> +++ b/mm/damon/dbgfs.c
>> @@ -308,21 +308,25 @@ static unsigned long *str_to_target_ids(const char *str, ssize_t len,
>>   	unsigned long *ids;
>>   	const int max_nr_ids = 32;
>>   	unsigned long id;
>> -	int pos = 0, parsed, ret;
>> +	int pos = 0, parsed;
>>   
>>   	*nr_ids = 0;
>>   	ids = kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL);
>>   	if (!ids)
>>   		return NULL;
>>   	while (*nr_ids < max_nr_ids && pos < len) {
>> -		ret = sscanf(&str[pos], "%lu%n", &id, &parsed);
>> -		pos += parsed;
>> -		if (ret != 1)
>> +		if (sscanf(&str[pos], "%lu%n", &id, &parsed) != 1)
>>   			break;
>> +		pos += parsed;
>>   		ids[*nr_ids] = id;
>>   		*nr_ids += 1;
>>   	}
>>   
>> +	if (!*nr_ids) {
>> +		kfree(ids);
>> +		return NULL;
>> +	}
>> +
>>   	return ids;
>>   }
>>   
>> -- 
>> 2.31.0

-- 
Best Regards!
Xin Hao