On Tue, 10 Mar 2020 09:02:09 +0000 Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> On Mon, 24 Feb 2020 13:30:41 +0100
> SeongJae Park <sjpark@xxxxxxxxxx> wrote:
>
> > From: SeongJae Park <sjpark@xxxxxxxxx>
> >
> > This commit adds a debugfs interface for DAMON.
[...]
> >
> > Signed-off-by: SeongJae Park <sjpark@xxxxxxxxx>
>
> 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");
> > }
> >
>
