Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface

[SeongJae Park <sjpark@amazon.com>]

On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <shakeelb@google.com> wrote:

> On Tue, Dec 15, 2020 at 3:59 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.
> 
> Which kernel space APIs are being referred here?

The symbols in 'include/linux/damon.h'

> 
> > 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.
> >
[...]
> >
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> > ---
> >  include/linux/damon.h |   3 +
> >  mm/damon/Kconfig      |   9 ++
> >  mm/damon/Makefile     |   1 +
> >  mm/damon/core.c       |  45 ++++++
> >  mm/damon/dbgfs.c      | 366 ++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 424 insertions(+)
> >  create mode 100644 mm/damon/dbgfs.c
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 39b4d6d3ddee..f9e0d4349352 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -265,9 +265,12 @@ unsigned int damon_nr_regions(struct damon_target *t);
> >
> >  struct damon_ctx *damon_new_ctx(enum damon_target_type type);
> >  void damon_destroy_ctx(struct damon_ctx *ctx);
> > +int damon_set_targets(struct damon_ctx *ctx,
> > +               unsigned long *ids, ssize_t nr_ids);
> >  int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> >                 unsigned long aggr_int, unsigned long regions_update_int,
> >                 unsigned long min_nr_reg, unsigned long max_nr_reg);
> > +int damon_nr_running_ctxs(void);
> >
> >  int damon_start(struct damon_ctx **ctxs, int nr_ctxs);
> >  int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> > diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
> > index 8ae080c52950..72f1683ba0ee 100644
> > --- a/mm/damon/Kconfig
> > +++ b/mm/damon/Kconfig
> > @@ -21,4 +21,13 @@ config DAMON_VADDR
> >           This builds the default data access monitoring primitives for DAMON
> >           that works for virtual address spaces.
> >
> > +config DAMON_DBGFS
> > +       bool "DAMON debugfs interface"
> > +       depends on DAMON_VADDR && DEBUG_FS
> > +       help
> > +         This builds the debugfs interface for DAMON.  The user space admins
> > +         can use the interface for arbitrary data access monitoring.
> > +
> > +         If unsure, say N.
> > +
> >  endmenu
> > diff --git a/mm/damon/Makefile b/mm/damon/Makefile
> > index 6ebbd08aed67..fed4be3bace3 100644
> > --- a/mm/damon/Makefile
> > +++ b/mm/damon/Makefile
> > @@ -2,3 +2,4 @@
> >
> >  obj-$(CONFIG_DAMON)            := core.o
> >  obj-$(CONFIG_DAMON_VADDR)      += vaddr.o
> > +obj-$(CONFIG_DAMON_DBGFS)      += dbgfs.o
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 5ca9f79ccbb6..b9575a6bebff 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -166,6 +166,37 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
> >         kfree(ctx);
> >  }
> >
> > +/**
> > + * damon_set_targets() - Set monitoring targets.
> > + * @ctx:       monitoring context
> > + * @ids:       array of target ids
> > + * @nr_ids:    number of entries in @ids
> > + *
> > + * This function should not be called while the kdamond is running.
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + */
> > +int damon_set_targets(struct damon_ctx *ctx,
> > +                     unsigned long *ids, ssize_t nr_ids)
> > +{
> > +       ssize_t i;
> > +       struct damon_target *t, *next;
> > +
> > +       damon_for_each_target_safe(t, next, ctx)
> > +               damon_destroy_target(t);
> 
> You need to put the reference on the target before destroying.

Oops, you're right.  I will fix this in the next version.

> 
> > +
> > +       for (i = 0; i < nr_ids; i++) {
> > +               t = damon_new_target(ids[i]);
> > +               if (!t) {
> > +                       pr_err("Failed to alloc damon_target\n");
> > +                       return -ENOMEM;
> > +               }
> > +               damon_add_target(ctx, t);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  /**
> >   * damon_set_attrs() - Set attributes for the monitoring.
> >   * @ctx:               monitoring context
> > @@ -206,6 +237,20 @@ int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> >         return 0;
> >  }
> >
> > +/**
> > + * 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(nr_running_ctxs) ?

I'd like to keep the code simpler to read, unless this turns out to be a real
performance bottleneck.

> 
> > +
> > +       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;
> > +       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;
> > +
> > +       targets = str_to_target_ids(nrs, ret, &nr_targets);
> > +       if (!targets) {
> > +               ret = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       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;
> 
> You need to put_pid on the targets array.

Good catch!

> 
> > +       }
> > +
> > +       err = damon_set_targets(ctx, targets, nr_targets);
> > +       if (err)
> > +               ret = err;
> 
> You need to handle the partial failure from damon_set_targets().

My intention is to keep partial success as is.

> 
> 
> > +unlock_out:
> > +       mutex_unlock(&ctx->kdamond_lock);
> > +       kfree(targets);
> > +out:
> > +       kfree(kbuf);
> > +       return ret;
> > +}
[...]

Thanks,
SeongJae Park