linux-kernel - Re: [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message

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:   Wed,  8 Dec 2021 16:48:38 +0000
From:   SeongJae Park <sj@...nel.org>
To:     Xin Hao <xhao@...ux.alibaba.com>
Cc:     SeongJae Park <sj@...nel.org>, akpm@...ux-foundation.org,
        shuah@...nel.org, brendanhiggins@...gle.com, linux-mm@...ck.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/11] mm/damon/dbgfs: Remove an unnecessary error message

On Wed, 8 Dec 2021 23:13:34 +0800 Xin Hao <xhao@...ux.alibaba.com> wrote:

> Hi SeongJae:
> 
> On 12/8/21 8:49 PM, SeongJae Park wrote:
> > On Wed, 8 Dec 2021 14:29:40 +0800 Xin Hao <xhao@...ux.alibaba.com> wrote:
> >
> > Hi Xin,
> >
> >> Hi park:
> >>
> >> On 12/1/21 11:04 PM, SeongJae Park wrote:
> >>> When wrong scheme action is requested via the debugfs interface, DAMON
> >>> prints an error message.  Because the function returns error code, this
> >>> is not really needed.  Because the code path is triggered by the user
> >>> specified input, this can result in kernel log mistakenly being messy.
> >> Completely correct, but there will also be a problem that users can’t
> >> quickly locate where the problem is,
> >>
> >> Especially too many parameters need to be written into the interface.
> >>
> >> I think it is necessary to add some debugging methods to help users find
> >> the error without polluting the kernel log.
> >>
> >> And i have an idea, like this:
> >>
> >> in dbgfs, add a last_cmd_stat interface.
> >>
> >>       # echo "1 2 1 2 1 2  1 2 1 2 100 ..."  > schemes
> >>
> >>       #  cat last_cmd_stat
> >>
> >>       #  wrong action 100
> >>
> >> In this way, on the one hand, it will not pollute the kernel log, on the
> >> other hand, it will help users find  the cause of the operation
> >> interface error.
> >>
> >> Park, how do you think of about this idea, if ok, i will send a patch.
> > Thank you always for your great suggestions and efforts!  BTW, I prefer to be
> > called with my first name ;)
> Ha-Ha, Sorry!
> >
> > I want DAMON kernel code to be as simple and small as possible, while putting
> > fancy but complicated features for user conveniences in user space tools like
> > DAMO[1].  In other words, I hope the DAMON debugfs interface to be used as an
> > interface for such user space tools, not an interface for human hands.
> Ok, I know what you mean.
> >
> > IMHO, implementing the feature you proposed in the kernel could make the code
> > slightly bigger, while it can easily implemented in user space.  I therefore
> > think the feature would be better to be implemented in user space.  If you
> > could send a pull request of the feature for DAMO, it would be so great.
> 
> Ok,  i will do it,  But there's a problem here, If the user does not use 
> the DAMO tools to operate  the dbgfs interface,

Well, I don't think that as a problem, but a room for improvement.  Maybe we
could improve the documentation.


Thanks,
SJ

> 
> the operation interface error will still hard to find the cause of errors.
> 
> 
> >
> > [1] https://github.com/awslabs/damo
> >
> >
> > Thanks,
> > SJ
> >
> >>> To avoid the case, this commit removes the message
> >>>
> >>> Fixes: af122dd8f3c0 ("mm/damon/dbgfs: support DAMON-based Operation Schemes")
> >>> Signed-off-by: SeongJae Park <sj@...nel.org>
> >>> ---
> >>>    mm/damon/dbgfs.c | 4 +---
> >>>    1 file changed, 1 insertion(+), 3 deletions(-)
> >>>
> >>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> >>> index 4bf4204444ab..5b628990ae6e 100644
> >>> --- a/mm/damon/dbgfs.c
> >>> +++ b/mm/damon/dbgfs.c
> >>> @@ -210,10 +210,8 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
> >>>    				&wmarks.low, &parsed);
> >>>    		if (ret != 18)
> >>>    			break;
> >>> -		if (!damos_action_valid(action)) {
> >>> -			pr_err("wrong action %d\n", action);
> >>> +		if (!damos_action_valid(action))
> >>>    			goto fail;
> >>> -		}
> >>>    
> >>>    		if (min_sz > max_sz || min_nr_a > max_nr_a || min_age > max_age)
> >>>    			goto fail;
> >> -- 
> >> Best Regards!
> >> Xin Hao
> >>
> -- 
> Best Regards!
> Xin Hao