* [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation
@ 2021-10-21 8:56 Xin Hao
2021-10-21 9:45 ` SeongJae Park
0 siblings, 1 reply; 3+ messages in thread
From: Xin Hao @ 2021-10-21 8:56 UTC (permalink / raw)
To: sjpark; +Cc: xhao, akpm, linux-mm, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation
2021-10-21 8:56 [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation Xin Hao
@ 2021-10-21 9:45 ` SeongJae Park
2021-10-21 16:42 ` Xin Hao
0 siblings, 1 reply; 3+ messages in thread
From: SeongJae Park @ 2021-10-21 9:45 UTC (permalink / raw)
To: Xin Hao; +Cc: sjpark, akpm, linux-mm, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation
2021-10-21 9:45 ` SeongJae Park
@ 2021-10-21 16:42 ` Xin Hao
0 siblings, 0 replies; 3+ messages in thread
From: Xin Hao @ 2021-10-21 16:42 UTC (permalink / raw)
To: SeongJae Park; +Cc: sjpark, akpm, linux-mm, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-21 16:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-21 8:56 [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation Xin Hao
2021-10-21 9:45 ` SeongJae Park
2021-10-21 16:42 ` Xin Hao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.