From: SeongJae Park <redacted>

On Tue, 22 Jun 2021 07:59:11 -0700 Shakeel Butt [off-list ref] wrote:

On Mon, Jun 21, 2021 at 1:31 AM SeongJae Park [off-list ref] wrote:

quoted

From: SeongJae Park <redacted>

DAMON is a data access monitoring framework for the Linux kernel.  The
core mechanisms of DAMON make it

 - accurate (the monitoring output is useful enough for DRAM level
   performance-centric memory management; It might be inappropriate for
   CPU cache levels, though),
 - light-weight (the monitoring overhead is normally low enough to be
   applied online), and
 - scalable (the upper-bound of the overhead is in constant range
   regardless of the size of target workloads).

Using this framework, hence, we can easily write efficient kernel space
data access monitoring applications.  For example, the kernel's memory
management mechanisms can make advanced decisions using this.
Experimental data access aware optimization works that incurring high
access monitoring overhead could again be implemented on top of this.

Due to its simple and flexible interface, providing user space interface
would be also easy.  Then, user space users who have some special
workloads can write personalized applications for better understanding
and optimizations of their workloads and systems.

===

Nevertheless, this commit is defining and implementing only basic access
check part without the overhead-accuracy handling core logic.  The basic
access check is as below.

The output of DAMON says what memory regions are how frequently accessed
for a given duration.  The resolution of the access frequency is
controlled by setting ``sampling interval`` and ``aggregation
interval``.  In detail, DAMON checks access to each page per ``sampling
interval`` and aggregates the results.  In other words, counts the
number of the accesses to each region.  After each ``aggregation
interval`` passes, DAMON calls callback functions that previously
registered by users so that users can read the aggregated results and
then clears the results.  This can be described in below simple
pseudo-code::

    init()
    while monitoring_on:
        for page in monitoring_target:
            if accessed(page):
                nr_accesses[page] += 1
        if time() % aggregation_interval == 0:
            for callback in user_registered_callbacks:
                callback(monitoring_target, nr_accesses)
            for page in monitoring_target:
                nr_accesses[page] = 0
        if time() % update_interval == 0:

regions_update_interval?

It used the name before.  But, I changed the name in this way to use it as a
general periodic updates of monitoring primitives.  Of course we can use the
specific name only in this specific example, but also want to make this as
similar to the actual code as possible.

If you strongly want to rename this, please feel free to let me know.

quoted

            update()
        sleep(sampling interval)

The target regions constructed at the beginning of the monitoring and
updated after each ``regions_update_interval``, because the target
regions could be dynamically changed (e.g., mmap() or memory hotplug).
The monitoring overhead of this mechanism will arbitrarily increase as
the size of the target workload grows.

The basic monitoring primitives for actual access check and dynamic
target regions construction aren't in the core part of DAMON.  Instead,
it allows users to implement their own primitives that are optimized for
their use case and configure DAMON to use those.  In other words, users
cannot use current version of DAMON without some additional works.

Following commits will implement the core mechanisms for the
overhead-accuracy control and default primitives implementations.

Signed-off-by: SeongJae Park <redacted>
Reviewed-by: Leonard Foerster <redacted>
Reviewed-by: Fernand Sieber <redacted>

Few nits below otherwise look good to me. You can add:

Acked-by: Shakeel Butt <redacted>

Thank you!

[...]

quoted

+/*
+ * __damon_start() - Starts monitoring with given context.
+ * @ctx:       monitoring context
+ *
+ * This function should be called while damon_lock is hold.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int __damon_start(struct damon_ctx *ctx)
+{
+       int err = -EBUSY;
+
+       mutex_lock(&ctx->kdamond_lock);
+       if (!ctx->kdamond) {
+               err = 0;
+               ctx->kdamond_stop = false;
+               ctx->kdamond = kthread_create(kdamond_fn, ctx, "kdamond.%d",
+                               nr_running_ctxs);
+               if (IS_ERR(ctx->kdamond))
+                       err = PTR_ERR(ctx->kdamond);
+               else
+                       wake_up_process(ctx->kdamond);

Nit: You can use kthread_run() here.

Ok, I will use that from the next spin.

quoted

+       }
+       mutex_unlock(&ctx->kdamond_lock);
+
+       return err;
+}
+

[...]

quoted

+static int __damon_stop(struct damon_ctx *ctx)
+{
+       mutex_lock(&ctx->kdamond_lock);
+       if (ctx->kdamond) {
+               ctx->kdamond_stop = true;
+               mutex_unlock(&ctx->kdamond_lock);
+               while (damon_kdamond_running(ctx))
+                       usleep_range(ctx->sample_interval,
+                                       ctx->sample_interval * 2);

Any reason to not use kthread_stop() here?

Using 'kthread_stop()' here will make the code much simpler.  But, 'kdamond'
also stops itself when all monitoring targets became invalid (e.g., all
monitoring target processes terminated).  However, 'kthread_stop()' is not easy
to be used for the use case (self stopping).  It's of course possible, but it
would make the code longer.  That's why I use 'kdamond_stop' flag here.  So,
I'd like leave this as is.  If you think 'kthread_stop()' should be used,
please feel free to let me know.


Thanks,
SeongJae Park