| Message ID | bc341f48b5558f6816dcef22eca4f4a590efdc67.1634834628.git.xhao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] mm/damon/dbgfs: Optimize target_ids interface write operation | expand |
Hello Xin, On Fri, 22 Oct 2021 00:44:16 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > When we want to clear 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 in 'dbgfs_target_ids_write', there is no need to > execute other codes, except call 'damon_set_targets' > to clear previously set target ids. So there adds > the 'nr_targets' judgment, if the value is 0, just > call 'damon_set_targets', and then return. It's true that it executes some unnecessary code. However, I unsure if that is a problem, as the code that will be additionally executed in this case are quite simple ones, and therefore not supposed to incur viewable overhead. After all, this is not a performance critical path. Thanks, SJ > > Signed-off-by: Xin Hao <xhao@linux.alibaba.com> > --- > mm/damon/dbgfs.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c > index a02cf6bee8e8..1d83f4138fad 100644 > --- a/mm/damon/dbgfs.c > +++ b/mm/damon/dbgfs.c > @@ -362,6 +362,12 @@ static ssize_t dbgfs_target_ids_write(struct file *file, > goto out; > } > > + if (!nr_targets) { > + /* remove targets with previously-set primitive */ > + damon_set_targets(ctx, NULL, 0); > + goto free_targets_out; > + } > + > if (id_is_pid) { > for (i = 0; i < nr_targets; i++) { > targets[i] = (unsigned long)find_get_pid( > -- > 2.31.0
On 2021/10/22 上午1:30, SeongJae Park wrote: > Hello Xin, > > On Fri, 22 Oct 2021 00:44:16 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > >> When we want to clear 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 in 'dbgfs_target_ids_write', there is no need to >> execute other codes, except call 'damon_set_targets' >> to clear previously set target ids. So there adds >> the 'nr_targets' judgment, if the value is 0, just >> call 'damon_set_targets', and then return. > It's true that it executes some unnecessary code. However, I unsure if that is > a problem, as the code that will be additionally executed in this case are > quite simple ones, and therefore not supposed to incur viewable overhead. > After all, this is not a performance critical path. Thank you for your detailed explanation. I may not describe it clearly, making you think that i am making this modification to improve performance，I just want to avoid irrelevant code execution, thank you so much. > > Thanks, > SJ > >> Signed-off-by: Xin Hao <xhao@linux.alibaba.com> >> --- >> mm/damon/dbgfs.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c >> index a02cf6bee8e8..1d83f4138fad 100644 >> --- a/mm/damon/dbgfs.c >> +++ b/mm/damon/dbgfs.c >> @@ -362,6 +362,12 @@ static ssize_t dbgfs_target_ids_write(struct file *file, >> goto out; >> } >> >> + if (!nr_targets) { >> + /* remove targets with previously-set primitive */ >> + damon_set_targets(ctx, NULL, 0); >> + goto free_targets_out; >> + } >> + >> if (id_is_pid) { >> for (i = 0; i < nr_targets; i++) { >> targets[i] = (unsigned long)find_get_pid( >> -- >> 2.31.0
On Fri, 22 Oct 2021 10:43:22 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > > On 2021/10/22 上午1:30, SeongJae Park wrote: > > Hello Xin, > > > > On Fri, 22 Oct 2021 00:44:16 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > > > >> When we want to clear 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 in 'dbgfs_target_ids_write', there is no need to > >> execute other codes, except call 'damon_set_targets' > >> to clear previously set target ids. So there adds > >> the 'nr_targets' judgment, if the value is 0, just > >> call 'damon_set_targets', and then return. > > It's true that it executes some unnecessary code. However, I unsure if that is > > a problem, as the code that will be additionally executed in this case are > > quite simple ones, and therefore not supposed to incur viewable overhead. > > After all, this is not a performance critical path. > > Thank you for your detailed explanation. I may not describe it clearly, > making you think that i am making this > > modification to improve performance，I just want to avoid irrelevant code > execution, thank you so much. I guess I didn't make my point clear enough, sorry. My concern in this patch is the fact that it is adding more code. IMHO, as the code is already working correctly and benefit of this change is quite subtle as you also agreed, adding the code here doesn't seem worthy but only making it harder to maintain, to me. If I'm missing something, please let me know. Thanks, SJ [...]
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c index a02cf6bee8e8..1d83f4138fad 100644 --- a/mm/damon/dbgfs.c +++ b/mm/damon/dbgfs.c @@ -362,6 +362,12 @@ static ssize_t dbgfs_target_ids_write(struct file *file, goto out; } + if (!nr_targets) { + /* remove targets with previously-set primitive */ + damon_set_targets(ctx, NULL, 0); + goto free_targets_out; + } + if (id_is_pid) { for (i = 0; i < nr_targets; i++) { targets[i] = (unsigned long)find_get_pid(
When we want to clear 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 in 'dbgfs_target_ids_write', there is no need to execute other codes, except call 'damon_set_targets' to clear previously set target ids. So there adds the 'nr_targets' judgment, if the value is 0, just call 'damon_set_targets', and then return. Signed-off-by: Xin Hao <xhao@linux.alibaba.com> --- mm/damon/dbgfs.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.31.0