On Wed, 8 Dec 2021 23:13:34 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> 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@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, 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@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
