Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: 5nYFytLj2UvIzlSxHw1B4kO6bi6VWzxV80E6jVROjexrYtyj8ba+izZcDxMFVExK0r6c4Ry5zL
 NqiPIMkT3hmw==
From:   SeongJae Park <sjpark@amazon.com>
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
CC:     SeongJae Park <sjpark@amazon.com>, <akpm@linux-foundation.org>,
        "SeongJae Park" <sjpark@amazon.de>, <aarcange@redhat.com>,
        <yang.shi@linux.alibaba.com>, <acme@kernel.org>,
        <alexander.shishkin@linux.intel.com>, <amit@kernel.org>,
        <brendan.d.gregg@gmail.com>, <brendanhiggins@google.com>,
        <cai@lca.pw>, <colin.king@canonical.com>, <corbet@lwn.net>,
        <dwmw@amazon.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>, <rientjes@google.com>,
        <rostedt@goodmis.org>, <shuah@kernel.org>, <sj38.park@gmail.com>,
        <vbabka@suse.cz>, <vdavydov.dev@gmail.com>, <linux-mm@kvack.org>,
        <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: Re: [PATCH v6 08/14] mm/damon: Add debugfs interface
Date:   Tue, 10 Mar 2020 12:56:54 +0100
Message-ID: <20200310115654.24999-1-sjpark@amazon.com>
In-Reply-To: <20200310090209.00000d6b@Huawei.com> (raw)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 10 Mar 2020 09:02:09 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 24 Feb 2020 13:30:41 +0100
> SeongJae Park <sjpark@amazon.com> wrote:
> 
> > From: SeongJae Park <sjpark@amazon.de>
> > 
> > This commit adds a debugfs interface for DAMON.
[...]
> > 
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> 
> Some of the code in here seems a bit fragile and convoluted.

Indeed, it needs many fixes.

> 
> > ---
> >  mm/damon.c | 377 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 376 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/damon.c b/mm/damon.c
> > index b3e9b9da5720..facb1d7f121b 100644
> > --- a/mm/damon.c
> > +++ b/mm/damon.c
> > @@ -10,6 +10,7 @@
[...]
> >  
> > +/*
> > + * debugfs functions
> 
> Seems unnecessary when their naming makes this clear.

Agreed, will remove it.

> 
[...]
> > +static ssize_t debugfs_pids_write(struct file *file,
> > +		const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > +	struct damon_ctx *ctx = &damon_user_ctx;
> > +	char *kbuf;
> > +	unsigned long *targets;
> > +	ssize_t nr_targets;
> > +	ssize_t ret;
> > +
> > +	kbuf = kmalloc_array(count, sizeof(char), GFP_KERNEL);
> > +	if (!kbuf)
> > +		return -ENOMEM;
> > +
> > +	ret = simple_write_to_buffer(kbuf, 512, ppos, buf, count);
> 
> Why only 512?

I might lost my mind at that time :'(
Good catch, it should be 'count'.

> 
[...]
> > +
> > +static ssize_t debugfs_attrs_write(struct file *file,
> > +		const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > +	struct damon_ctx *ctx = &damon_user_ctx;
> > +	unsigned long s, a, r, minr, maxr;
> > +	char *kbuf;
> > +	ssize_t ret;
> > +
> > +	kbuf = kmalloc_array(count, sizeof(char), GFP_KERNEL);
> 
> malloc fine for array of characters.   The checks on overflow etc cannot be
> relevant here.

You're right, will use 'kamlloc()' instead.

> 
> > +	if (!kbuf)
> > +		return -ENOMEM;
> > +
> > +	ret = simple_write_to_buffer(kbuf, count, ppos, buf, count);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (sscanf(kbuf, "%lu %lu %lu %lu %lu",
> > +				&s, &a, &r, &minr, &maxr) != 5) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	spin_lock(&ctx->kdamond_lock);
> > +	if (ctx->kdamond)
> > +		goto monitor_running;
> > +
> > +	damon_set_attrs(ctx, s, a, r, minr, maxr);
> > +	spin_unlock(&ctx->kdamond_lock);
> > +
> > +	goto out;
> > +
> > +monitor_running:
> > +	spin_unlock(&ctx->kdamond_lock);
> > +	pr_err("%s: kdamond is running. Turn it off first.\n", __func__);
> > +	ret = -EINVAL;
> 
> This complex exit path is a bad idea from maintainability point of view...
> Just put the pr_err and spin_unlock in the error path above.

Agreed, will do so.

> 
> > +out:
> > +	kfree(kbuf);
> > +	return ret;
> > +}
> > +
> > +static const struct file_operations monitor_on_fops = {
> > +	.owner = THIS_MODULE,
> > +	.read = debugfs_monitor_on_read,
> > +	.write = debugfs_monitor_on_write,
> > +};
> > +
> > +static const struct file_operations pids_fops = {
> > +	.owner = THIS_MODULE,
> > +	.read = debugfs_pids_read,
> > +	.write = debugfs_pids_write,
> > +};
> > +
> > +static const struct file_operations record_fops = {
> > +	.owner = THIS_MODULE,
> > +	.read = debugfs_record_read,
> > +	.write = debugfs_record_write,
> > +};
> > +
> > +static const struct file_operations attrs_fops = {
> > +	.owner = THIS_MODULE,
> > +	.read = debugfs_attrs_read,
> > +	.write = debugfs_attrs_write,
> > +};
> > +
> > +static struct dentry *debugfs_root;
> > +
> > +static int __init debugfs_init(void)
> 
> Prefix this function.  Chances of sometime getting a header
> that includes debugfs_init feels rather too high!

That's right, I will rename it.

> 
> > +{
> > +	const char * const file_names[] = {"attrs", "record",
> > +		"pids", "monitor_on"};
> > +	const struct file_operations *fops[] = {&attrs_fops, &record_fops,
> > +		&pids_fops, &monitor_on_fops};
> > +	int i;
> > +
> > +	debugfs_root = debugfs_create_dir("damon", NULL);
> > +	if (!debugfs_root) {
> > +		pr_err("failed to create the debugfs dir\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> > +		if (!debugfs_create_file(file_names[i], 0600, debugfs_root,
> > +					NULL, fops[i])) {
> > +			pr_err("failed to create %s file\n", file_names[i]);
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init damon_init_user_ctx(void)
> > +{
> > +	int rc;
> > +
> > +	struct damon_ctx *ctx = &damon_user_ctx;
> > +
> > +	ktime_get_coarse_ts64(&ctx->last_aggregation);
> > +	ctx->last_regions_update = ctx->last_aggregation;
> > +
> > +	ctx->rbuf_offset = 0;
> > +	rc = damon_set_recording(ctx, 1024 * 1024, "/damon.data");
> > +	if (rc)
> > +		return rc;
> > +
> > +	ctx->kdamond = NULL;
> > +	ctx->kdamond_stop = false;
> > +	spin_lock_init(&ctx->kdamond_lock);
> > +
> > +	prandom_seed_state(&ctx->rndseed, 42);
> 
> :)

You got the answer ;)

> 
> > +	INIT_LIST_HEAD(&ctx->tasks_list);
> > +
> > +	ctx->sample_cb = NULL;
> > +	ctx->aggregate_cb = NULL;
> 
> Should already be set to 0.

Oops, right!

> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int __init damon_init(void)
> >  {
> > +	int rc;
> > +
> >  	pr_info("init\n");
> >  
> > -	return 0;
> > +	rc = damon_init_user_ctx();
> > +	if (rc)
> > +		return rc;
> > +
> > +	return debugfs_init();
> 
> In theory no code should ever be dependent on debugfs succeeding..
> There might be other daemon users so you should just eat the return
> code.

Right!  Thank you for catching this!


Thanks,
SeongJae Park

> 
> 
> >  }
> >  
> >  static void __exit damon_exit(void)
> >  {
> > +	damon_turn_kdamond(&damon_user_ctx, false);
> > +	debugfs_remove_recursive(debugfs_root);
> > +
> > +	kfree(damon_user_ctx.rbuf);
> > +	kfree(damon_user_ctx.rfile_path);
> > +
> >  	pr_info("exit\n");
> >  }
> >  
>