Re: [PATCH v22 10/18] mm/damon: Implement a debugfs-based user space interface

[SeongJae Park <sjpark@amazon.com>]

On Wed, 25 Nov 2020 07:30:36 -0800 Shakeel Butt <shakeelb@google.com> wrote:

> On Tue, Oct 20, 2020 at 2:06 AM SeongJae Park <sjpark@amazon.com> 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/``.
> >>
[...]
> > +/**
> > + * damon_nr_running_ctxs() - Return number of currently running contexts.
> > + */
> > +int damon_nr_running_ctxs(void)
> > +{
> > +       int nr_ctxs;
> > +
> > +       mutex_lock(&damon_lock);
> > +       nr_ctxs = nr_running_ctxs;
> > +       mutex_unlock(&damon_lock);
> > +
> 
> READ_ONCE() instead of mutex?

Right, it would be ok and even make the code slightly faster.  But, if you're
ok, I'd like to keep this as is because this helps reader easily find what
variables are protected by the mutex and this is not performance critical.

> 
> > +       return nr_ctxs;
> > +}
> > +
[...]
> > +
> > +static ssize_t dbgfs_target_ids_write(struct file *file,
> > +               const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > +       struct damon_ctx *ctx = file->private_data;
> > +       char *kbuf, *nrs;
> > +       bool received_pidfds = false;
> > +       unsigned long *targets;
> > +       ssize_t nr_targets;
> > +       ssize_t ret = count;
> > +       int i;
> > +       int err;
> > +
> > +       kbuf = user_input_str(buf, count, ppos);
> > +       if (IS_ERR(kbuf))
> > +               return PTR_ERR(kbuf);
> > +
> > +       nrs = kbuf;
> > +
> > +       if (!strncmp(kbuf, "pidfd ", 6)) {
> > +               received_pidfds = true;
> 
> I am inclining towards having simple pids instead of pidfds. Basically
> what cgroup/resctrl does.

Ok, I will drop the pidfd support for simplicity.  Restoring it back when real
requirement comes out would not be too late.

> 
> 
> > +               nrs = &kbuf[6];
> > +       }
> > +
> > +       targets = str_to_target_ids(nrs, ret, &nr_targets);
> > +       if (!targets) {
> > +               ret = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       if (received_pidfds) {
> > +               for (i = 0; i < nr_targets; i++)
> > +                       targets[i] = (unsigned long)damon_get_pidfd_pid(
> > +                                       (unsigned int)targets[i]);
> > +       } else if (targetid_is_pid(ctx)) {
> > +               for (i = 0; i < nr_targets; i++)
> > +                       targets[i] = (unsigned long)find_get_pid(
> > +                                       (int)targets[i]);
> > +       }
> > +
> > +       mutex_lock(&ctx->kdamond_lock);
> > +       if (ctx->kdamond) {
> > +               ret = -EINVAL;
> > +               goto unlock_out;
> > +       }
> > +
> > +       err = damon_set_targets(ctx, targets, nr_targets);
> 
> Hmm this is leaking the references to the previous targets.

'damon_set_targets()' frees the previous targets itself, so we don't leak.

> 
> > +       if (err)
> > +               ret = err;
> > +unlock_out:
> > +       mutex_unlock(&ctx->kdamond_lock);
> > +       kfree(targets);
> > +out:
> > +       kfree(kbuf);
> > +       return ret;
> > +}
> > +
> 
> Still looking.

Looking forward your comments!


Thanks,
SeongJae Park