Messages in this thread Patch in this message | | | From | Xin Hao <> | Subject | Re: [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation | Date | Fri, 22 Oct 2021 00:42:50 +0800 |
| |
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
| |