Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   SeongJae Park <sjpark@amazon.com>
To:     Greg KH <greg@kroah.com>
CC:     SeongJae Park <sjpark@amazon.com>,
        SeongJae Park <sjpark@amazon.de>,
        <Jonathan.Cameron@huawei.com>, <aarcange@redhat.com>,
        <acme@kernel.org>, <alexander.shishkin@linux.intel.com>,
        <amit@kernel.org>, <benh@kernel.crashing.org>,
        <brendan.d.gregg@gmail.com>, <brendanhiggins@google.com>,
        <cai@lca.pw>, <colin.king@canonical.com>, <corbet@lwn.net>,
        <david@redhat.com>, <dwmw@amazon.com>, <elver@google.com>,
        <fan.du@intel.com>, <foersleo@amazon.de>, <gthelen@google.com>,
        <irogers@google.com>, <jolsa@redhat.com>, <kirill@shutemov.name>,
        <mark.rutland@arm.com>, <mgorman@suse.de>, <minchan@kernel.org>,
        <mingo@redhat.com>, <namhyung@kernel.org>, <peterz@infradead.org>,
        <rdunlap@infradead.org>, <riel@surriel.com>, <rientjes@google.com>,
        <rostedt@goodmis.org>, <rppt@kernel.org>, <sblbir@amazon.com>,
        <shakeelb@google.com>, <shuah@kernel.org>, <sj38.park@gmail.com>,
        <snu@amazon.de>, <vbabka@suse.cz>, <vdavydov.dev@gmail.com>,
        <yang.shi@linux.alibaba.com>, <ying.huang@intel.com>,
        <zgf574564920@gmail.com>, <linux-damon@amazon.com>,
        <linux-mm@kvack.org>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v24 07/14] mm/damon: Implement a debugfs-based user space interface
Date:   Fri, 5 Feb 2021 16:59:02 +0100
Message-ID: <20210205155902.31102-1-sjpark@amazon.com>
In-Reply-To: <YB1kZaD/7omxXztF@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Fri, 5 Feb 2021 16:29:41 +0100 Greg KH <greg@kroah.com> wrote:

> On Thu, Feb 04, 2021 at 04:31:43PM +0100, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> > 
> > DAMON is designed to be used by kernel space code such as the memory
> > management subsystems, and therefore it provides only kernel space API.
> > That said, letting the user space control DAMON could provide some
> > benefits to them.  For example, it will allow user space to analyze
> > their specific workloads and make their own special optimizations.
> > 
> > For such cases, this commit implements a simple DAMON application kernel
> > module, namely 'damon-dbgfs', which merely wraps the DAMON api and
> > exports those to the user space via the debugfs.
> > 
> > 'damon-dbgfs' exports three files, ``attrs``, ``target_ids``, and
> > ``monitor_on`` under its debugfs directory, ``<debugfs>/damon/``.
[...]
> > ---
> >  include/linux/damon.h |   3 +
> >  mm/damon/Kconfig      |   9 +
> >  mm/damon/Makefile     |   1 +
> >  mm/damon/core.c       |  47 +++++
> >  mm/damon/dbgfs.c      | 387 ++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 447 insertions(+)
> >  create mode 100644 mm/damon/dbgfs.c
[...]
> > diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> > new file mode 100644
> > index 000000000000..db15380737d1
> > --- /dev/null
> > +++ b/mm/damon/dbgfs.c
[...]
> > +
> > +static int dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
> > +{
> > +	const char * const file_names[] = {"attrs", "target_ids"};
> > +	const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops};
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> > +		if (!debugfs_create_file(file_names[i], 0600, dir,
> > +					ctx, fops[i])) {
> > +			pr_err("failed to create %s file\n", file_names[i]);
> > +			return -ENOMEM;
> 
> No need to check the return value of this function, just keep going and
> ignore it as there's nothing to do and kernel code should not do
> different things based on the output of any debugfs calls.
> 
> Also, this check is totally wrong and doesn't do what you think it is
> doing...

Ok, I will drop the check.

> 
> > +static int __init __damon_dbgfs_init(void)
> > +{
> > +	struct dentry *dbgfs_root;
> > +	const char * const file_names[] = {"monitor_on"};
> > +	const struct file_operations *fops[] = {&monitor_on_fops};
> > +	int i;
> > +
> > +	dbgfs_root = debugfs_create_dir("damon", NULL);
> > +	if (IS_ERR(dbgfs_root)) {
> > +		pr_err("failed to create the dbgfs dir\n");
> > +		return PTR_ERR(dbgfs_root);
> 
> Again, no need to check anything, just pass the result of a debugfs call
> back into another one just fine.

Ok.

> 
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> > +		if (!debugfs_create_file(file_names[i], 0600, dbgfs_root,
> > +					NULL, fops[i])) {
> 
> Again, this isn't checking what you think it is, so please don't do it.

Got it.

I will fix those as you suggested in the next version.


Thanks,
SeongJae Park

> 
> thanks,
> 
> greg k-h