linux-kernel - Re: [PATCH V2] mm/damon/dbgfs: Optimize target

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 22 Oct 2021 09:43:41 +0000
From:   SeongJae Park <sj@...nel.org>
To:     Xin Hao <xhao@...ux.alibaba.com>
Cc:     SeongJae Park <sj@...nel.org>, sjpark@...zon.de,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] mm/damon/dbgfs: Optimize target_ids interface write operation

On Fri, 22 Oct 2021 10:43:22 +0800 Xin Hao <xhao@...ux.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@...ux.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

[...]