lkml.org 
[lkml]   [2021]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation
Date

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

\
 
 \ /
  Last update: 2021-10-21 18:43    [W:0.040 / U:0.836 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site