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 [thread overview]
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");
> > }
> >
>
next prev parent reply other threads:[~2020-03-10 11:57 UTC|newest]
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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).