Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
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> (raw)
In-Reply-To: <20200310090209.00000d6b@Huawei.com> (raw)

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");
> >  }
> >  
> 


  reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-24 12:30 [PATCH v6 00/14] Introduce Data Access MONitor (DAMON) SeongJae Park
2020-02-24 12:30 ` [PATCH v6 01/14] mm: " SeongJae Park
2020-03-10  8:54   ` Jonathan Cameron
2020-03-10 11:50     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 02/14] mm/damon: Implement region based sampling SeongJae Park
2020-03-10  8:57   ` Jonathan Cameron
2020-03-10 11:52     ` SeongJae Park
2020-03-10 15:55       ` Jonathan Cameron
2020-03-10 16:22         ` SeongJae Park
2020-03-10 17:39           ` Jonathan Cameron
2020-03-12  9:20             ` SeongJae Park
2020-03-13 17:29   ` Jonathan Cameron
2020-03-13 20:16     ` SeongJae Park
2020-03-17 11:32       ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 03/14] mm/damon: Adaptively adjust regions SeongJae Park
2020-03-10  8:57   ` Jonathan Cameron
2020-03-10 11:53     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 04/14] mm/damon: Apply dynamic memory mapping changes SeongJae Park
2020-03-10  9:00   ` Jonathan Cameron
2020-03-10 11:53     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 05/14] mm/damon: Implement callbacks SeongJae Park
2020-03-10  9:01   ` Jonathan Cameron
2020-03-10 11:55     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 06/14] mm/damon: Implement access pattern recording SeongJae Park
2020-03-10  9:01   ` Jonathan Cameron
2020-03-10 11:55     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 07/14] mm/damon: Implement kernel space API SeongJae Park
2020-03-10  9:01   ` Jonathan Cameron
2020-03-10 11:56     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 08/14] mm/damon: Add debugfs interface SeongJae Park
2020-03-10  9:02   ` Jonathan Cameron
2020-03-10 11:56     ` SeongJae Park [this message]
2020-02-24 12:30 ` [PATCH v6 09/14] mm/damon: Add a tracepoint for result writing SeongJae Park
2020-03-10  9:03   ` Jonathan Cameron
2020-03-10 11:57     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 10/14] tools: Add a minimal user-space tool for DAMON SeongJae Park
2020-02-24 12:30 ` [PATCH v6 11/14] Documentation/admin-guide/mm: Add a document " SeongJae Park
2020-03-10  9:03   ` Jonathan Cameron
2020-03-10 11:57     ` SeongJae Park
2020-02-24 12:30 ` [PATCH v6 12/14] mm/damon: Add kunit tests SeongJae Park
2020-02-24 12:30 ` [PATCH v6 13/14] mm/damon: Add user selftests SeongJae Park
2020-02-24 12:30 ` [PATCH v6 14/14] MAINTAINERS: Update for DAMON SeongJae Park
2020-03-02 11:35 ` [PATCH v6 00/14] Introduce Data Access MONitor (DAMON) SeongJae Park
2020-03-09 10:23   ` SeongJae Park
2020-03-10 17:21 ` Shakeel Butt
2020-03-12 10:07   ` SeongJae Park
2020-03-12 10:43     ` SeongJae Park
2020-03-18 19:52       ` Shakeel Butt
2020-03-19  9:03         ` SeongJae Park
2020-03-23 17:29           ` Shakeel Butt
2020-03-24  8:34             ` SeongJae Park

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200310115654.24999-1-sjpark@amazon.com \
    --to=sjpark@amazon.com \
    --cc=Jonathan.Cameron@Huawei.com \
    --cc=aarcange@redhat.com \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=amit@kernel.org \
    --cc=brendan.d.gregg@gmail.com \
    --cc=brendanhiggins@google.com \
    --cc=cai@lca.pw \
    --cc=colin.king@canonical.com \
    --cc=corbet@lwn.net \
    --cc=dwmw@amazon.com \
    --cc=jolsa@redhat.com \
    --cc=kirill@shutemov.name \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mark.rutland@arm.com \
    --cc=mgorman@suse.de \
    --cc=minchan@kernel.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rdunlap@infradead.org \
    --cc=rientjes@google.com \
    --cc=rostedt@goodmis.org \
    --cc=shuah@kernel.org \
    --cc=sj38.park@gmail.com \
    --cc=sjpark@amazon.de \
    --cc=vbabka@suse.cz \
    --cc=vdavydov.dev@gmail.com \
    --cc=yang.shi@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git