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

[Xin Hao <xhao@linux.alibaba.com>]

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