On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt [off-list ref] wrote:

On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park [off-list ref] wrote:

quoted

From: SeongJae Park <redacted>

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'

quoted

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.

[...]

quoted

Signed-off-by: SeongJae Park <redacted>
Reviewed-by: Leonard Foerster <redacted>
---
 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.

quoted

+
+       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.

quoted

+
+       return nr_ctxs;
+}
+

[...]

quoted

+
+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!

quoted

+       }
+
+       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.

quoted

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

[...]

Thanks,
SeongJae Park