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

[Xin Hao]

Hi SeongJae:

On 12/8/21 8:49 PM, SeongJae Park wrote:
> On Wed, 8 Dec 2021 14:29:40 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> 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,

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@xxxxxxxxxx>
> > > ---
> > >    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