On 2021/10/21 下午5:45, SeongJae Park wrote:
Hello Xin,
On Thu, 21 Oct 2021 16:56:11 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
---
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
