Currently kdamond uses only one context per kthread
and most of its time it sleeps, so utilizing several
contexts can scale kdamond and allow it to use
another set of operations.
This patch-set implements support for multiple contexts
per kdamond.
In pseudo code previous versions worked like
the following:
while (!kdamond_should_stop()) {
/* prepare accesses for only 1 context */
prepare_accesses(damon_context);
sleep(sample_interval);
/* check accesses for only 1 context */
check_accesses(damon_context);
...
}
With this patch kdamond workflow will look
like the following:
while (!kdamond_shoule_stop()) {
/* prepare accesses for all contexts in kdamond */
damon_for_each_context(ctx, kdamond)
prepare_accesses(ctx);
sleep(sample_interval);
/* check_accesses for all contexts in kdamond */
damon_for_each_context(ctx, kdamond)
check_accesses(ctx);
...
}
To try this you can use modified kernel[1] and
damo[2]. I also have written few simple shell scripts[3]
to collect data for damo.
[1] https://github.com/onlyoneofme/damon-multi-contexts.git
[2] https://github.com/onlyoneofme/damo/tree/multi-contexts
[3] https://github.com/onlyoneofme/damon-multi-contexts-tests.git
---
Changes from v1 (https://lore.kernel.org/damon/[email protected]/)
- Compatibility for DebugFS interface is kept
- Kunit tests build/execution issues are fixed
- Patches from v1 are sqaushed, so that consistency between patches is
kept
- Added/Fixed comments about data structures/functions
Alex Rusuf (2):
mm/damon/core: add 'struct kdamond' abstraction layer
mm/damon/core: implement multi-context support
include/linux/damon.h | 80 ++++--
include/trace/events/damon.h | 14 +-
mm/damon/core-test.h | 2 +-
mm/damon/core.c | 509 ++++++++++++++++++++++-------------
mm/damon/dbgfs-test.h | 4 +-
mm/damon/dbgfs.c | 342 ++++++++++++++---------
mm/damon/lru_sort.c | 31 ++-
mm/damon/modules-common.c | 35 ++-
mm/damon/modules-common.h | 3 +-
mm/damon/reclaim.c | 30 ++-
mm/damon/sysfs.c | 303 +++++++++++++--------
11 files changed, 872 insertions(+), 481 deletions(-)
--
2.42.0
In current implementation kdamond tracks only 1
context, that is kdamond _is_ damon_ctx what makes
it very difficult to implement multiple contexts.
This patch adds another level of abstraction, that is
'struct kdamond' - structure which represents kdamond itself.
It holds references to all contexts organized in list.
Few fields like ->kdamond_started and ->kdamond_lock
(just ->lock for 'struct kdamond') also has been moved
to 'struct kdamond', because they have nothing to do
with the context itself, but with the whole kdamond
daemon.
Signed-off-by: Alex Rusuf <[email protected]>
---
include/linux/damon.h | 73 ++++++++---
mm/damon/core.c | 249 ++++++++++++++++++++++-------------
mm/damon/lru_sort.c | 31 +++--
mm/damon/modules-common.c | 36 +++--
mm/damon/modules-common.h | 3 +-
mm/damon/reclaim.c | 30 +++--
mm/damon/sysfs.c | 268 ++++++++++++++++++++++++--------------
7 files changed, 463 insertions(+), 227 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 886d07294..7cb9979a0 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -568,29 +568,49 @@ struct damon_attrs {
unsigned long max_nr_regions;
};
+/**
+ * struct kdamond - Represents a background daemon that is responsible
+ * for executing each context.
+ *
+ * @lock: Kdamond's global lock, serializes accesses to any field.
+ * @self: Kernel thread which is actually being executed.
+ * @contexts: Head of contexts (&damon_ctx) list.
+ * @nr_ctxs: Number of contexts being monitored.
+ *
+ * Each DAMON's background daemon has this structure. Once
+ * configured, daemon can be started by calling damon_start().
+ *
+ * Monitoring can be explicitly stopped by calling damon_stop(). Once
+ * daemon is terminated @self is set to NULL, so users can know if
+ * monitoring is stopped by reading @self pointer. Access to @self
+ * must also be protected by @lock.
+ */
+struct kdamond {
+ struct mutex lock;
+ struct task_struct *self;
+ struct list_head contexts;
+ size_t nr_ctxs;
+
+/* private: */
+ /* for waiting until the execution of the kdamond_fn is started */
+ struct completion kdamond_started;
+};
+
/**
* struct damon_ctx - Represents a context for each monitoring. This is the
* main interface that allows users to set the attributes and get the results
* of the monitoring.
*
* @attrs: Monitoring attributes for accuracy/overhead control.
- * @kdamond: Kernel thread who does the monitoring.
- * @kdamond_lock: Mutex for the synchronizations with @kdamond.
+ * @kdamond: Back reference to daemon who is the owner of this context.
+ * @list: List head of siblings.
*
* For each monitoring context, one kernel thread for the monitoring is
* created. The pointer to the thread is stored in @kdamond.
*
* Once started, the monitoring thread runs until explicitly required to be
* terminated or every monitoring target is invalid. The validity of the
- * targets is checked via the &damon_operations.target_valid of @ops. The
- * termination can also be explicitly requested by calling damon_stop().
- * The thread sets @kdamond to NULL when it terminates. Therefore, users can
- * know whether the monitoring is ongoing or terminated by reading @kdamond.
- * Reads and writes to @kdamond from outside of the monitoring thread must
- * be protected by @kdamond_lock.
- *
- * Note that the monitoring thread protects only @kdamond via @kdamond_lock.
- * Accesses to other fields must be protected by themselves.
+ * targets is checked via the &damon_operations.target_valid of @ops.
*
* @ops: Set of monitoring operations for given use cases.
* @callback: Set of callbacks for monitoring events notifications.
@@ -614,12 +634,11 @@ struct damon_ctx {
* update
*/
unsigned long next_ops_update_sis;
- /* for waiting until the execution of the kdamond_fn is started */
- struct completion kdamond_started;
+ unsigned long sz_limit;
/* public: */
- struct task_struct *kdamond;
- struct mutex kdamond_lock;
+ struct kdamond *kdamond;
+ struct list_head list;
struct damon_operations ops;
struct damon_callback callback;
@@ -653,6 +672,15 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
return r->ar.end - r->ar.start;
}
+static inline struct damon_target *damon_first_target(struct damon_ctx *ctx)
+{
+ return list_first_entry(&ctx->adaptive_targets, struct damon_target, list);
+}
+
+static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
+{
+ return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
+}
#define damon_for_each_region(r, t) \
list_for_each_entry(r, &t->regions_list, list)
@@ -675,6 +703,12 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
#define damon_for_each_scheme_safe(s, next, ctx) \
list_for_each_entry_safe(s, next, &(ctx)->schemes, list)
+#define damon_for_each_context(c, kdamond) \
+ list_for_each_entry(c, &(kdamond)->contexts, list)
+
+#define damon_for_each_context_safe(c, next, kdamond) \
+ list_for_each_entry_safe(c, next, &(kdamond)->contexts, list)
+
#define damos_for_each_quota_goal(goal, quota) \
list_for_each_entry(goal, "a->goals, list)
@@ -736,7 +770,12 @@ void damon_destroy_target(struct damon_target *t);
unsigned int damon_nr_regions(struct damon_target *t);
struct damon_ctx *damon_new_ctx(void);
+void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx);
+struct kdamond *damon_new_kdamond(void);
void damon_destroy_ctx(struct damon_ctx *ctx);
+void damon_destroy_ctxs(struct kdamond *kdamond);
+void damon_destroy_kdamond(struct kdamond *kdamond);
+bool damon_kdamond_running(struct kdamond *kdamond);
int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs);
void damon_set_schemes(struct damon_ctx *ctx,
struct damos **schemes, ssize_t nr_schemes);
@@ -758,8 +797,8 @@ static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs
}
-int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive);
-int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
+int damon_start(struct kdamond *kdamond, bool exclusive);
+int damon_stop(struct kdamond *kdamond);
int damon_set_region_biggest_system_ram_default(struct damon_target *t,
unsigned long *start, unsigned long *end);
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 6d503c1c1..cfc9c803d 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -24,7 +24,7 @@
#endif
static DEFINE_MUTEX(damon_lock);
-static int nr_running_ctxs;
+static int nr_running_kdamonds;
static bool running_exclusive_ctxs;
static DEFINE_MUTEX(damon_ops_lock);
@@ -488,8 +488,6 @@ struct damon_ctx *damon_new_ctx(void)
if (!ctx)
return NULL;
- init_completion(&ctx->kdamond_started);
-
ctx->attrs.sample_interval = 5 * 1000;
ctx->attrs.aggr_interval = 100 * 1000;
ctx->attrs.ops_update_interval = 60 * 1000 * 1000;
@@ -499,17 +497,41 @@ struct damon_ctx *damon_new_ctx(void)
ctx->next_aggregation_sis = 0;
ctx->next_ops_update_sis = 0;
- mutex_init(&ctx->kdamond_lock);
-
ctx->attrs.min_nr_regions = 10;
ctx->attrs.max_nr_regions = 1000;
INIT_LIST_HEAD(&ctx->adaptive_targets);
INIT_LIST_HEAD(&ctx->schemes);
+ INIT_LIST_HEAD(&ctx->list);
return ctx;
}
+/**
+ * Adds newly allocated and configured @ctx to @kdamond.
+ */
+void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
+{
+ list_add_tail(&ctx->list, &kdamond->contexts);
+ ++kdamond->nr_ctxs;
+}
+
+struct kdamond *damon_new_kdamond(void)
+{
+ struct kdamond *kdamond;
+
+ kdamond = kzalloc(sizeof(*kdamond), GFP_KERNEL);
+ if (!kdamond)
+ return NULL;
+
+ init_completion(&kdamond->kdamond_started);
+ mutex_init(&kdamond->lock);
+
+ INIT_LIST_HEAD(&kdamond->contexts);
+
+ return kdamond;
+}
+
static void damon_destroy_targets(struct damon_ctx *ctx)
{
struct damon_target *t, *next_t;
@@ -523,6 +545,11 @@ static void damon_destroy_targets(struct damon_ctx *ctx)
damon_destroy_target(t);
}
+static inline void damon_del_ctx(struct damon_ctx *ctx)
+{
+ list_del(&ctx->list);
+}
+
void damon_destroy_ctx(struct damon_ctx *ctx)
{
struct damos *s, *next_s;
@@ -532,9 +559,27 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
damon_for_each_scheme_safe(s, next_s, ctx)
damon_destroy_scheme(s);
+ damon_del_ctx(ctx);
kfree(ctx);
}
+void damon_destroy_ctxs(struct kdamond *kdamond)
+{
+ struct damon_ctx *c, *next;
+
+ damon_for_each_context_safe(c, next, kdamond) {
+ damon_destroy_ctx(c);
+ --kdamond->nr_ctxs;
+ }
+}
+
+void damon_destroy_kdamond(struct kdamond *kdamond)
+{
+ damon_destroy_ctxs(kdamond);
+ mutex_destroy(&kdamond->lock);
+ kfree(kdamond);
+}
+
static unsigned int damon_age_for_new_attrs(unsigned int age,
struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
{
@@ -667,13 +712,27 @@ void damon_set_schemes(struct damon_ctx *ctx, struct damos **schemes,
*/
int damon_nr_running_ctxs(void)
{
- int nr_ctxs;
+ int nr_kdamonds;
mutex_lock(&damon_lock);
- nr_ctxs = nr_running_ctxs;
+ nr_kdamonds = nr_running_kdamonds;
mutex_unlock(&damon_lock);
- return nr_ctxs;
+ return nr_kdamonds;
+}
+
+/**
+ * damon_kdamond_running() - Return true if kdamond is running
+ * false otherwise.
+ */
+bool damon_kdamond_running(struct kdamond *kdamond)
+{
+ bool running;
+
+ mutex_lock(&kdamond->lock);
+ running = kdamond->self != NULL;
+ mutex_unlock(&kdamond->lock);
+ return running;
}
/* Returns the size upper limit for each monitoring region */
@@ -700,38 +759,37 @@ static int kdamond_fn(void *data);
/*
* __damon_start() - Starts monitoring with given context.
- * @ctx: monitoring context
+ * @kdamond: daemon to be started
*
* 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)
+static int __damon_start(struct kdamond *kdamond)
{
int err = -EBUSY;
- mutex_lock(&ctx->kdamond_lock);
- if (!ctx->kdamond) {
+ mutex_lock(&kdamond->lock);
+ if (!kdamond->self) {
err = 0;
- reinit_completion(&ctx->kdamond_started);
- ctx->kdamond = kthread_run(kdamond_fn, ctx, "kdamond.%d",
- nr_running_ctxs);
- if (IS_ERR(ctx->kdamond)) {
- err = PTR_ERR(ctx->kdamond);
- ctx->kdamond = NULL;
+ reinit_completion(&kdamond->kdamond_started);
+ kdamond->self = kthread_run(kdamond_fn, kdamond, "kdamond.%d",
+ nr_running_kdamonds);
+ if (IS_ERR(kdamond->self)) {
+ err = PTR_ERR(kdamond->self);
+ kdamond->self = NULL;
} else {
- wait_for_completion(&ctx->kdamond_started);
+ wait_for_completion(&kdamond->kdamond_started);
}
}
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
return err;
}
/**
* damon_start() - Starts the monitorings for a given group of contexts.
- * @ctxs: an array of the pointers for contexts to start monitoring
- * @nr_ctxs: size of @ctxs
+ * @kdamond: a daemon that contains list of monitoring contexts
* @exclusive: exclusiveness of this contexts group
*
* This function starts a group of monitoring threads for a group of monitoring
@@ -743,74 +801,59 @@ static int __damon_start(struct damon_ctx *ctx)
*
* Return: 0 on success, negative error code otherwise.
*/
-int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
+int damon_start(struct kdamond *kdamond, bool exclusive)
{
- int i;
int err = 0;
+ BUG_ON(!kdamond);
+ BUG_ON(!kdamond->nr_ctxs);
+
+ if (kdamond->nr_ctxs != 1)
+ return -EINVAL;
+
mutex_lock(&damon_lock);
- if ((exclusive && nr_running_ctxs) ||
+ if ((exclusive && nr_running_kdamonds) ||
(!exclusive && running_exclusive_ctxs)) {
mutex_unlock(&damon_lock);
return -EBUSY;
}
- for (i = 0; i < nr_ctxs; i++) {
- err = __damon_start(ctxs[i]);
- if (err)
- break;
- nr_running_ctxs++;
- }
- if (exclusive && nr_running_ctxs)
+ err = __damon_start(kdamond);
+ if (err)
+ return err;
+ nr_running_kdamonds++;
+
+ if (exclusive && nr_running_kdamonds)
running_exclusive_ctxs = true;
mutex_unlock(&damon_lock);
return err;
}
-/*
- * __damon_stop() - Stops monitoring of a given context.
- * @ctx: monitoring context
+/**
+ * damon_stop() - Stops the monitorings for a given group of contexts.
+ * @kdamond: a daemon (that contains list of monitoring contexts)
+ * to be stopped.
*
* Return: 0 on success, negative error code otherwise.
*/
-static int __damon_stop(struct damon_ctx *ctx)
+int damon_stop(struct kdamond *kdamond)
{
struct task_struct *tsk;
- mutex_lock(&ctx->kdamond_lock);
- tsk = ctx->kdamond;
+ mutex_lock(&kdamond->lock);
+ tsk = kdamond->self;
if (tsk) {
get_task_struct(tsk);
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
kthread_stop_put(tsk);
return 0;
}
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
return -EPERM;
}
-/**
- * damon_stop() - Stops the monitorings for a given group of contexts.
- * @ctxs: an array of the pointers for contexts to stop monitoring
- * @nr_ctxs: size of @ctxs
- *
- * Return: 0 on success, negative error code otherwise.
- */
-int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
-{
- int i, err = 0;
-
- for (i = 0; i < nr_ctxs; i++) {
- /* nr_running_ctxs is decremented in kdamond_fn */
- err = __damon_stop(ctxs[i]);
- if (err)
- break;
- }
- return err;
-}
-
/*
* Reset the aggregated monitoring results ('nr_accesses' of each region).
*/
@@ -1582,29 +1625,68 @@ static void kdamond_init_intervals_sis(struct damon_ctx *ctx)
}
}
+static bool kdamond_init_ctx(struct damon_ctx *ctx)
+{
+ if (ctx->ops.init)
+ ctx->ops.init(ctx);
+ if (ctx->callback.before_start && ctx->callback.before_start(ctx))
+ return false;
+
+ kdamond_init_intervals_sis(ctx);
+ ctx->sz_limit = damon_region_sz_limit(ctx);
+
+ return true;
+}
+
+static bool kdamond_init_ctxs(struct kdamond *kdamond)
+{
+ struct damon_ctx *c;
+
+ damon_for_each_context(c, kdamond)
+ if (!kdamond_init_ctx(c))
+ return false;
+ return true;
+}
+
+static void kdamond_finish_ctx(struct damon_ctx *ctx)
+{
+ struct damon_target *t;
+ struct damon_region *r, *next;
+
+ damon_for_each_target(t, ctx) {
+ damon_for_each_region_safe(r, next, t)
+ damon_destroy_region(r, t);
+ }
+
+ if (ctx->callback.before_terminate)
+ ctx->callback.before_terminate(ctx);
+ if (ctx->ops.cleanup)
+ ctx->ops.cleanup(ctx);
+}
+
+static void kdamond_finish_ctxs(struct kdamond *kdamond)
+{
+ struct damon_ctx *c;
+
+ damon_for_each_context(c, kdamond)
+ kdamond_finish_ctx(c);
+}
+
/*
* The monitoring daemon that runs as a kernel thread
*/
static int kdamond_fn(void *data)
{
- struct damon_ctx *ctx = data;
- struct damon_target *t;
- struct damon_region *r, *next;
+ struct kdamond *kdamond = data;
+ struct damon_ctx *ctx = damon_first_ctx(kdamond);
unsigned int max_nr_accesses = 0;
- unsigned long sz_limit = 0;
pr_debug("kdamond (%d) starts\n", current->pid);
- complete(&ctx->kdamond_started);
- kdamond_init_intervals_sis(ctx);
-
- if (ctx->ops.init)
- ctx->ops.init(ctx);
- if (ctx->callback.before_start && ctx->callback.before_start(ctx))
+ complete(&kdamond->kdamond_started);
+ if (!kdamond_init_ctxs(kdamond))
goto done;
- sz_limit = damon_region_sz_limit(ctx);
-
while (!kdamond_need_stop(ctx)) {
/*
* ctx->attrs and ctx->next_{aggregation,ops_update}_sis could
@@ -1616,6 +1698,7 @@ static int kdamond_fn(void *data)
unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
unsigned long sample_interval = ctx->attrs.sample_interval;
+ unsigned long sz_limit = ctx->sz_limit;
if (kdamond_wait_activation(ctx))
break;
@@ -1666,28 +1749,20 @@ static int kdamond_fn(void *data)
sample_interval;
if (ctx->ops.update)
ctx->ops.update(ctx);
- sz_limit = damon_region_sz_limit(ctx);
+ ctx->sz_limit = damon_region_sz_limit(ctx);
}
}
done:
- damon_for_each_target(t, ctx) {
- damon_for_each_region_safe(r, next, t)
- damon_destroy_region(r, t);
- }
-
- if (ctx->callback.before_terminate)
- ctx->callback.before_terminate(ctx);
- if (ctx->ops.cleanup)
- ctx->ops.cleanup(ctx);
+ kdamond_finish_ctxs(kdamond);
pr_debug("kdamond (%d) finishes\n", current->pid);
- mutex_lock(&ctx->kdamond_lock);
- ctx->kdamond = NULL;
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
+ kdamond->self = NULL;
+ mutex_unlock(&kdamond->lock);
mutex_lock(&damon_lock);
- nr_running_ctxs--;
- if (!nr_running_ctxs && running_exclusive_ctxs)
+ nr_running_kdamonds--;
+ if (!nr_running_kdamonds && running_exclusive_ctxs)
running_exclusive_ctxs = false;
mutex_unlock(&damon_lock);
diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 3de2916a6..76c20098a 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -142,8 +142,18 @@ static struct damos_access_pattern damon_lru_sort_stub_pattern = {
.max_age_region = UINT_MAX,
};
-static struct damon_ctx *ctx;
-static struct damon_target *target;
+static struct kdamond *kdamond;
+
+static inline struct damon_ctx *damon_lru_sort_ctx(void)
+{
+ return damon_first_ctx(kdamond);
+}
+
+static inline struct damon_target *damon_lru_sort_target(void)
+{
+ return damon_first_target(
+ damon_lru_sort_ctx());
+}
static struct damos *damon_lru_sort_new_scheme(
struct damos_access_pattern *pattern, enum damos_action action)
@@ -201,6 +211,7 @@ static int damon_lru_sort_apply_parameters(void)
struct damos *scheme, *hot_scheme, *cold_scheme;
struct damos *old_hot_scheme = NULL, *old_cold_scheme = NULL;
unsigned int hot_thres, cold_thres;
+ struct damon_ctx *ctx = damon_lru_sort_ctx();
int err = 0;
err = damon_set_attrs(ctx, &damon_lru_sort_mon_attrs);
@@ -237,7 +248,8 @@ static int damon_lru_sort_apply_parameters(void)
damon_set_schemes(ctx, &hot_scheme, 1);
damon_add_scheme(ctx, cold_scheme);
- return damon_set_region_biggest_system_ram_default(target,
+ return damon_set_region_biggest_system_ram_default(
+ damon_lru_sort_target(),
&monitor_region_start,
&monitor_region_end);
}
@@ -247,7 +259,7 @@ static int damon_lru_sort_turn(bool on)
int err;
if (!on) {
- err = damon_stop(&ctx, 1);
+ err = damon_stop(kdamond);
if (!err)
kdamond_pid = -1;
return err;
@@ -257,10 +269,11 @@ static int damon_lru_sort_turn(bool on)
if (err)
return err;
- err = damon_start(&ctx, 1, true);
+ err = damon_start(kdamond, true);
if (err)
return err;
- kdamond_pid = ctx->kdamond->pid;
+
+ kdamond_pid = kdamond->self->pid;
return 0;
}
@@ -279,7 +292,7 @@ static int damon_lru_sort_enabled_store(const char *val,
return 0;
/* Called before init function. The function will handle this. */
- if (!ctx)
+ if (!kdamond)
goto set_param_out;
err = damon_lru_sort_turn(enable);
@@ -334,11 +347,13 @@ static int damon_lru_sort_after_wmarks_check(struct damon_ctx *c)
static int __init damon_lru_sort_init(void)
{
- int err = damon_modules_new_paddr_ctx_target(&ctx, &target);
+ struct damon_ctx *ctx;
+ int err = damon_modules_new_paddr_kdamond(&kdamond);
if (err)
return err;
+ ctx = damon_lru_sort_ctx();
ctx->callback.after_wmarks_check = damon_lru_sort_after_wmarks_check;
ctx->callback.after_aggregation = damon_lru_sort_after_aggregation;
diff --git a/mm/damon/modules-common.c b/mm/damon/modules-common.c
index 7cf96574c..436bb7948 100644
--- a/mm/damon/modules-common.c
+++ b/mm/damon/modules-common.c
@@ -9,13 +9,7 @@
#include "modules-common.h"
-/*
- * Allocate, set, and return a DAMON context for the physical address space.
- * @ctxp: Pointer to save the point to the newly created context
- * @targetp: Pointer to save the point to the newly created target
- */
-int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
- struct damon_target **targetp)
+static int __damon_modules_new_paddr_kdamond(struct kdamond *kdamond)
{
struct damon_ctx *ctx;
struct damon_target *target;
@@ -34,9 +28,33 @@ int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
damon_destroy_ctx(ctx);
return -ENOMEM;
}
+
damon_add_target(ctx, target);
+ damon_add_ctx(kdamond, ctx);
+
+ return 0;
+}
+
+/*
+ * Allocate, set, and return a DAMON daemon for the physical address space.
+ * @kdamondp: Pointer to save the point to the newly created kdamond
+ */
+int damon_modules_new_paddr_kdamond(struct kdamond **kdamondp)
+{
+ int err;
+ struct kdamond *kdamond;
+
+ kdamond = damon_new_kdamond();
+ if (!kdamond)
+ return -ENOMEM;
+
+ err = __damon_modules_new_paddr_kdamond(kdamond);
+ if (err) {
+ damon_destroy_kdamond(kdamond);
+ return err;
+ }
+ kdamond->nr_ctxs = 1;
- *ctxp = ctx;
- *targetp = target;
+ *kdamondp = kdamond;
return 0;
}
diff --git a/mm/damon/modules-common.h b/mm/damon/modules-common.h
index f49cdb417..5fc5b8ae3 100644
--- a/mm/damon/modules-common.h
+++ b/mm/damon/modules-common.h
@@ -45,5 +45,4 @@
module_param_named(nr_##qt_exceed_name, stat.qt_exceeds, ulong, \
0400);
-int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
- struct damon_target **targetp);
+int damon_modules_new_paddr_kdamond(struct kdamond **kdamondp);
diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 9bd341d62..f6540ef1a 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -150,8 +150,18 @@ static struct damos_stat damon_reclaim_stat;
DEFINE_DAMON_MODULES_DAMOS_STATS_PARAMS(damon_reclaim_stat,
reclaim_tried_regions, reclaimed_regions, quota_exceeds);
-static struct damon_ctx *ctx;
-static struct damon_target *target;
+static struct kdamond *kdamond;
+
+static inline struct damon_ctx *damon_reclaim_ctx(void)
+{
+ return damon_first_ctx(kdamond);
+}
+
+static inline struct damon_target *damon_reclaim_target(void)
+{
+ return damon_first_target(
+ damon_reclaim_ctx());
+}
static struct damos *damon_reclaim_new_scheme(void)
{
@@ -197,6 +207,7 @@ static int damon_reclaim_apply_parameters(void)
struct damos *scheme, *old_scheme;
struct damos_quota_goal *goal;
struct damos_filter *filter;
+ struct damon_ctx *ctx = damon_reclaim_ctx();
int err = 0;
err = damon_set_attrs(ctx, &damon_reclaim_mon_attrs);
@@ -244,7 +255,8 @@ static int damon_reclaim_apply_parameters(void)
}
damon_set_schemes(ctx, &scheme, 1);
- return damon_set_region_biggest_system_ram_default(target,
+ return damon_set_region_biggest_system_ram_default(
+ damon_reclaim_target(),
&monitor_region_start,
&monitor_region_end);
}
@@ -254,7 +266,7 @@ static int damon_reclaim_turn(bool on)
int err;
if (!on) {
- err = damon_stop(&ctx, 1);
+ err = damon_stop(kdamond);
if (!err)
kdamond_pid = -1;
return err;
@@ -264,10 +276,10 @@ static int damon_reclaim_turn(bool on)
if (err)
return err;
- err = damon_start(&ctx, 1, true);
+ err = damon_start(kdamond, true);
if (err)
return err;
- kdamond_pid = ctx->kdamond->pid;
+ kdamond_pid = kdamond->self->pid;
return 0;
}
@@ -286,7 +298,7 @@ static int damon_reclaim_enabled_store(const char *val,
return 0;
/* Called before init function. The function will handle this. */
- if (!ctx)
+ if (!kdamond)
goto set_param_out;
err = damon_reclaim_turn(enable);
@@ -337,11 +349,13 @@ static int damon_reclaim_after_wmarks_check(struct damon_ctx *c)
static int __init damon_reclaim_init(void)
{
- int err = damon_modules_new_paddr_ctx_target(&ctx, &target);
+ struct damon_ctx *ctx;
+ int err = damon_modules_new_paddr_kdamond(&kdamond);
if (err)
return err;
+ ctx = damon_reclaim_ctx();
ctx->callback.after_wmarks_check = damon_reclaim_after_wmarks_check;
ctx->callback.after_aggregation = damon_reclaim_after_aggregation;
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 6fee383bc..bfdb979e6 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -939,7 +939,7 @@ static const struct kobj_type damon_sysfs_contexts_ktype = {
struct damon_sysfs_kdamond {
struct kobject kobj;
struct damon_sysfs_contexts *contexts;
- struct damon_ctx *damon_ctx;
+ struct kdamond *kdamond;
};
static struct damon_sysfs_kdamond *damon_sysfs_kdamond_alloc(void)
@@ -974,16 +974,6 @@ static void damon_sysfs_kdamond_rm_dirs(struct damon_sysfs_kdamond *kdamond)
kobject_put(&kdamond->contexts->kobj);
}
-static bool damon_sysfs_ctx_running(struct damon_ctx *ctx)
-{
- bool running;
-
- mutex_lock(&ctx->kdamond_lock);
- running = ctx->kdamond != NULL;
- mutex_unlock(&ctx->kdamond_lock);
- return running;
-}
-
/*
* enum damon_sysfs_cmd - Commands for a specific kdamond.
*/
@@ -1065,15 +1055,15 @@ static struct damon_sysfs_cmd_request damon_sysfs_cmd_request;
static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
{
- struct damon_sysfs_kdamond *kdamond = container_of(kobj,
+ struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj,
struct damon_sysfs_kdamond, kobj);
- struct damon_ctx *ctx = kdamond->damon_ctx;
+ struct kdamond *kdamond = sys_kdamond->kdamond;
bool running;
- if (!ctx)
+ if (!kdamond)
running = false;
else
- running = damon_sysfs_ctx_running(ctx);
+ running = damon_kdamond_running(kdamond);
return sysfs_emit(buf, "%s\n", running ?
damon_sysfs_cmd_strs[DAMON_SYSFS_CMD_ON] :
@@ -1242,13 +1232,15 @@ static bool damon_sysfs_schemes_regions_updating;
static void damon_sysfs_before_terminate(struct damon_ctx *ctx)
{
struct damon_target *t, *next;
- struct damon_sysfs_kdamond *kdamond;
+ struct damon_sysfs_kdamond *sys_kdamond;
+ struct kdamond *kdamond;
enum damon_sysfs_cmd cmd;
/* damon_sysfs_schemes_update_regions_stop() might not yet called */
- kdamond = damon_sysfs_cmd_request.kdamond;
+ kdamond = ctx->kdamond;
+ sys_kdamond = damon_sysfs_cmd_request.kdamond;
cmd = damon_sysfs_cmd_request.cmd;
- if (kdamond && ctx == kdamond->damon_ctx &&
+ if (sys_kdamond && kdamond == sys_kdamond->kdamond &&
(cmd == DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_REGIONS ||
cmd == DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_BYTES) &&
damon_sysfs_schemes_regions_updating) {
@@ -1260,12 +1252,12 @@ static void damon_sysfs_before_terminate(struct damon_ctx *ctx)
if (!damon_target_has_pid(ctx))
return;
- mutex_lock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
damon_for_each_target_safe(t, next, ctx) {
put_pid(t->pid);
damon_destroy_target(t);
}
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
}
/*
@@ -1277,55 +1269,91 @@ static void damon_sysfs_before_terminate(struct damon_ctx *ctx)
* callbacks while holding ``damon_syfs_lock``, to safely access the DAMON
* contexts-internal data and DAMON sysfs variables.
*/
-static int damon_sysfs_upd_schemes_stats(struct damon_sysfs_kdamond *kdamond)
+static int damon_sysfs_upd_schemes_stats(struct damon_sysfs_kdamond *sys_kdamond)
{
- struct damon_ctx *ctx = kdamond->damon_ctx;
+ struct damon_ctx *c;
+ struct damon_sysfs_context **sysfs_ctxs;
- if (!ctx)
+ if (!sys_kdamond->kdamond)
return -EINVAL;
- damon_sysfs_schemes_update_stats(
- kdamond->contexts->contexts_arr[0]->schemes, ctx);
+
+ sysfs_ctxs = sys_kdamond->contexts->contexts_arr;
+ damon_for_each_context(c, sys_kdamond->kdamond) {
+ struct damon_sysfs_context *sysfs_ctx = *sysfs_ctxs;
+
+ damon_sysfs_schemes_update_stats(sysfs_ctx->schemes, c);
+ ++sysfs_ctxs;
+ }
return 0;
}
static int damon_sysfs_upd_schemes_regions_start(
- struct damon_sysfs_kdamond *kdamond, bool total_bytes_only)
+ struct damon_sysfs_kdamond *sys_kdamond, bool total_bytes_only)
{
- struct damon_ctx *ctx = kdamond->damon_ctx;
+ struct damon_ctx *c;
+ struct damon_sysfs_context **sysfs_ctxs;
+ int err;
- if (!ctx)
+ if (!sys_kdamond->kdamond)
return -EINVAL;
- return damon_sysfs_schemes_update_regions_start(
- kdamond->contexts->contexts_arr[0]->schemes, ctx,
- total_bytes_only);
+
+ sysfs_ctxs = sys_kdamond->contexts->contexts_arr;
+ damon_for_each_context(c, sys_kdamond->kdamond) {
+ struct damon_sysfs_context *sysfs_ctx = *sysfs_ctxs;
+
+ err = damon_sysfs_schemes_update_regions_start(sysfs_ctx->schemes, c,
+ total_bytes_only);
+ if (err)
+ return err;
+ ++sysfs_ctxs;
+ }
+ return 0;
}
static int damon_sysfs_upd_schemes_regions_stop(
- struct damon_sysfs_kdamond *kdamond)
+ struct damon_sysfs_kdamond *sys_kdamond)
{
- struct damon_ctx *ctx = kdamond->damon_ctx;
+ struct damon_ctx *c;
+ int err;
- if (!ctx)
+ if (!sys_kdamond->kdamond)
return -EINVAL;
- return damon_sysfs_schemes_update_regions_stop(ctx);
+
+ damon_for_each_context(c, sys_kdamond->kdamond) {
+ err = damon_sysfs_schemes_update_regions_stop(c);
+ if (err)
+ return err;
+ }
+ return 0;
}
static int damon_sysfs_clear_schemes_regions(
- struct damon_sysfs_kdamond *kdamond)
+ struct damon_sysfs_kdamond *sys_kdamond)
{
- struct damon_ctx *ctx = kdamond->damon_ctx;
+ struct damon_ctx *c;
+ struct damon_sysfs_context **sysfs_ctxs;
+ int err;
- if (!ctx)
+ if (!sys_kdamond->kdamond)
return -EINVAL;
- return damon_sysfs_schemes_clear_regions(
- kdamond->contexts->contexts_arr[0]->schemes, ctx);
+
+ sysfs_ctxs = sys_kdamond->contexts->contexts_arr;
+ damon_for_each_context(c, sys_kdamond->kdamond) {
+ struct damon_sysfs_context *sysfs_ctx = *sysfs_ctxs;
+
+ err = damon_sysfs_schemes_clear_regions(sysfs_ctx->schemes, c);
+ if (err)
+ return err;
+ ++sysfs_ctxs;
+ }
+ return 0;
}
static inline bool damon_sysfs_kdamond_running(
- struct damon_sysfs_kdamond *kdamond)
+ struct damon_sysfs_kdamond *sys_kdamond)
{
- return kdamond->damon_ctx &&
- damon_sysfs_ctx_running(kdamond->damon_ctx);
+ return sys_kdamond->kdamond &&
+ damon_kdamond_running(sys_kdamond->kdamond);
}
static int damon_sysfs_apply_inputs(struct damon_ctx *ctx,
@@ -1351,23 +1379,34 @@ static int damon_sysfs_apply_inputs(struct damon_ctx *ctx,
*
* If the sysfs input is wrong, the kdamond will be terminated.
*/
-static int damon_sysfs_commit_input(struct damon_sysfs_kdamond *kdamond)
+static int damon_sysfs_commit_input(struct damon_sysfs_kdamond *sys_kdamond)
{
- if (!damon_sysfs_kdamond_running(kdamond))
+ struct damon_ctx *c;
+ struct damon_sysfs_context *sysfs_ctx;
+ int err;
+
+ if (!damon_sysfs_kdamond_running(sys_kdamond))
return -EINVAL;
/* TODO: Support multiple contexts per kdamond */
- if (kdamond->contexts->nr != 1)
+ if (sys_kdamond->contexts->nr != 1)
return -EINVAL;
- return damon_sysfs_apply_inputs(kdamond->damon_ctx,
- kdamond->contexts->contexts_arr[0]);
+ sysfs_ctx = sys_kdamond->contexts->contexts_arr[0];
+ damon_for_each_context(c, sys_kdamond->kdamond) {
+ err = damon_sysfs_apply_inputs(c, sysfs_ctx);
+ if (err)
+ return err;
+ ++sysfs_ctx;
+ }
+ return 0;
}
static int damon_sysfs_commit_schemes_quota_goals(
struct damon_sysfs_kdamond *sysfs_kdamond)
{
- struct damon_ctx *ctx;
- struct damon_sysfs_context *sysfs_ctx;
+ struct damon_ctx *c;
+ struct damon_sysfs_context **sysfs_ctxs;
+ int err;
if (!damon_sysfs_kdamond_running(sysfs_kdamond))
return -EINVAL;
@@ -1375,9 +1414,16 @@ static int damon_sysfs_commit_schemes_quota_goals(
if (sysfs_kdamond->contexts->nr != 1)
return -EINVAL;
- ctx = sysfs_kdamond->damon_ctx;
- sysfs_ctx = sysfs_kdamond->contexts->contexts_arr[0];
- return damos_sysfs_set_quota_scores(sysfs_ctx->schemes, ctx);
+ sysfs_ctxs = sysfs_kdamond->contexts->contexts_arr;
+ damon_for_each_context(c, sysfs_kdamond->kdamond) {
+ struct damon_sysfs_context *sysfs_ctx = *sysfs_ctxs;
+
+ err = damos_sysfs_set_quota_scores(sysfs_ctx->schemes, c);
+ if (err)
+ return err;
+ ++sysfs_ctxs;
+ }
+ return 0;
}
/*
@@ -1391,14 +1437,21 @@ static int damon_sysfs_commit_schemes_quota_goals(
* DAMON contexts-internal data and DAMON sysfs variables.
*/
static int damon_sysfs_upd_schemes_effective_quotas(
- struct damon_sysfs_kdamond *kdamond)
+ struct damon_sysfs_kdamond *sys_kdamond)
{
- struct damon_ctx *ctx = kdamond->damon_ctx;
+ struct damon_ctx *c;
+ struct damon_sysfs_context **sysfs_ctxs;
- if (!ctx)
+ if (!sys_kdamond->kdamond)
return -EINVAL;
- damos_sysfs_update_effective_quotas(
- kdamond->contexts->contexts_arr[0]->schemes, ctx);
+
+ sysfs_ctxs = sys_kdamond->contexts->contexts_arr;
+ damon_for_each_context(c, sys_kdamond->kdamond) {
+ struct damon_sysfs_context *sysfs_ctx = *sysfs_ctxs;
+
+ damos_sysfs_update_effective_quotas(sysfs_ctx->schemes, c);
+ ++sysfs_ctxs;
+ }
return 0;
}
@@ -1415,7 +1468,7 @@ static int damon_sysfs_upd_schemes_effective_quotas(
static int damon_sysfs_cmd_request_callback(struct damon_ctx *c, bool active,
bool after_aggregation)
{
- struct damon_sysfs_kdamond *kdamond;
+ struct damon_sysfs_kdamond *sys_kdamond;
bool total_bytes_only = false;
int err = 0;
@@ -1423,27 +1476,27 @@ static int damon_sysfs_cmd_request_callback(struct damon_ctx *c, bool active,
if (!damon_sysfs_schemes_regions_updating &&
!mutex_trylock(&damon_sysfs_lock))
return 0;
- kdamond = damon_sysfs_cmd_request.kdamond;
- if (!kdamond || kdamond->damon_ctx != c)
+ sys_kdamond = damon_sysfs_cmd_request.kdamond;
+ if (!sys_kdamond || !c || sys_kdamond->kdamond != c->kdamond)
goto out;
switch (damon_sysfs_cmd_request.cmd) {
case DAMON_SYSFS_CMD_UPDATE_SCHEMES_STATS:
- err = damon_sysfs_upd_schemes_stats(kdamond);
+ err = damon_sysfs_upd_schemes_stats(sys_kdamond);
break;
case DAMON_SYSFS_CMD_COMMIT:
if (!after_aggregation)
goto out;
- err = damon_sysfs_commit_input(kdamond);
+ err = damon_sysfs_commit_input(sys_kdamond);
break;
case DAMON_SYSFS_CMD_COMMIT_SCHEMES_QUOTA_GOALS:
- err = damon_sysfs_commit_schemes_quota_goals(kdamond);
+ err = damon_sysfs_commit_schemes_quota_goals(sys_kdamond);
break;
case DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_BYTES:
total_bytes_only = true;
fallthrough;
case DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_REGIONS:
if (!damon_sysfs_schemes_regions_updating) {
- err = damon_sysfs_upd_schemes_regions_start(kdamond,
+ err = damon_sysfs_upd_schemes_regions_start(sys_kdamond,
total_bytes_only);
if (!err) {
damon_sysfs_schemes_regions_updating = true;
@@ -1458,15 +1511,15 @@ static int damon_sysfs_cmd_request_callback(struct damon_ctx *c, bool active,
*/
if (active && !damos_sysfs_regions_upd_done())
goto keep_lock_out;
- err = damon_sysfs_upd_schemes_regions_stop(kdamond);
+ err = damon_sysfs_upd_schemes_regions_stop(sys_kdamond);
damon_sysfs_schemes_regions_updating = false;
}
break;
case DAMON_SYSFS_CMD_CLEAR_SCHEMES_TRIED_REGIONS:
- err = damon_sysfs_clear_schemes_regions(kdamond);
+ err = damon_sysfs_clear_schemes_regions(sys_kdamond);
break;
case DAMON_SYSFS_CMD_UPDATE_SCHEMES_EFFECTIVE_QUOTAS:
- err = damon_sysfs_upd_schemes_effective_quotas(kdamond);
+ err = damon_sysfs_upd_schemes_effective_quotas(sys_kdamond);
break;
default:
break;
@@ -1529,40 +1582,63 @@ static struct damon_ctx *damon_sysfs_build_ctx(
return ctx;
}
-static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
+static struct kdamond *damon_sysfs_build_kdamond(
+ struct damon_sysfs_context **sys_ctx, size_t nr_ctxs)
{
struct damon_ctx *ctx;
+ struct kdamond *kdamond;
+
+ kdamond = damon_new_kdamond();
+ if (!kdamond)
+ return ERR_PTR(-ENOMEM);
+
+ for (size_t i = 0; i < nr_ctxs; ++i) {
+ ctx = damon_sysfs_build_ctx(sys_ctx[i]);
+ if (IS_ERR(ctx)) {
+ damon_destroy_kdamond(kdamond);
+ return ERR_PTR(PTR_ERR(ctx));
+ }
+ ctx->kdamond = kdamond;
+ damon_add_ctx(kdamond, ctx);
+ }
+ return kdamond;
+}
+
+static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *sys_kdamond)
+{
+ struct kdamond *kdamond;
int err;
- if (damon_sysfs_kdamond_running(kdamond))
+ if (damon_sysfs_kdamond_running(sys_kdamond))
return -EBUSY;
- if (damon_sysfs_cmd_request.kdamond == kdamond)
+ if (damon_sysfs_cmd_request.kdamond == sys_kdamond)
return -EBUSY;
/* TODO: support multiple contexts per kdamond */
- if (kdamond->contexts->nr != 1)
+ if (sys_kdamond->contexts->nr != 1)
return -EINVAL;
- if (kdamond->damon_ctx)
- damon_destroy_ctx(kdamond->damon_ctx);
- kdamond->damon_ctx = NULL;
+ if (sys_kdamond->kdamond)
+ damon_destroy_kdamond(sys_kdamond->kdamond);
+ sys_kdamond->kdamond = NULL;
- ctx = damon_sysfs_build_ctx(kdamond->contexts->contexts_arr[0]);
- if (IS_ERR(ctx))
- return PTR_ERR(ctx);
- err = damon_start(&ctx, 1, false);
+ kdamond = damon_sysfs_build_kdamond(sys_kdamond->contexts->contexts_arr,
+ sys_kdamond->contexts->nr);
+ if (IS_ERR(kdamond))
+ return PTR_ERR(kdamond);
+ err = damon_start(kdamond, false);
if (err) {
- damon_destroy_ctx(ctx);
+ damon_destroy_kdamond(kdamond);
return err;
}
- kdamond->damon_ctx = ctx;
+ sys_kdamond->kdamond = kdamond;
return err;
}
-static int damon_sysfs_turn_damon_off(struct damon_sysfs_kdamond *kdamond)
+static int damon_sysfs_turn_damon_off(struct damon_sysfs_kdamond *sys_kdamond)
{
- if (!kdamond->damon_ctx)
+ if (!sys_kdamond->kdamond)
return -EINVAL;
- return damon_stop(&kdamond->damon_ctx, 1);
+ return damon_stop(sys_kdamond->kdamond);
/*
* To allow users show final monitoring results of already turned-off
* DAMON, we free kdamond->damon_ctx in next
@@ -1654,21 +1730,21 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
static ssize_t pid_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
- struct damon_sysfs_kdamond *kdamond = container_of(kobj,
+ struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj,
struct damon_sysfs_kdamond, kobj);
- struct damon_ctx *ctx;
+ struct kdamond *kdamond;
int pid = -1;
if (!mutex_trylock(&damon_sysfs_lock))
return -EBUSY;
- ctx = kdamond->damon_ctx;
- if (!ctx)
+ kdamond = sys_kdamond->kdamond;
+ if (!kdamond)
goto out;
- mutex_lock(&ctx->kdamond_lock);
- if (ctx->kdamond)
- pid = ctx->kdamond->pid;
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
+ if (kdamond->self)
+ pid = kdamond->self->pid;
+ mutex_unlock(&kdamond->lock);
out:
mutex_unlock(&damon_sysfs_lock);
return sysfs_emit(buf, "%d\n", pid);
@@ -1676,12 +1752,12 @@ static ssize_t pid_show(struct kobject *kobj,
static void damon_sysfs_kdamond_release(struct kobject *kobj)
{
- struct damon_sysfs_kdamond *kdamond = container_of(kobj,
+ struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj,
struct damon_sysfs_kdamond, kobj);
- if (kdamond->damon_ctx)
- damon_destroy_ctx(kdamond->damon_ctx);
- kfree(kdamond);
+ if (sys_kdamond->kdamond)
+ damon_destroy_kdamond(sys_kdamond->kdamond);
+ kfree(sys_kdamond);
}
static struct kobj_attribute damon_sysfs_kdamond_state_attr =
--
2.42.0
This patch actually implements support for
multi-context design for kdamond daemon.
In pseudo code previous versions worked like
the following:
while (!kdamond_should_stop()) {
/* prepare accesses for only 1 context */
prepare_accesses(damon_context);
sleep(sample_interval);
/* check accesses for only 1 context */
check_accesses(damon_context);
...
}
With this patch kdamond workflow will look
like the following:
while (!kdamond_shoule_stop()) {
/* prepare accesses for all contexts in kdamond */
damon_for_each_context(ctx, kdamond)
prepare_accesses(ctx);
sleep(sample_interval);
/* check_accesses for all contexts in kdamond */
damon_for_each_context(ctx, kdamond)
check_accesses(ctx);
...
}
Another point to note is watermarks. Previous versions
checked watermarks on each iteration for current context
and if matric's value wan't acceptable kdamond waited
for watermark's sleep interval.
Now there's no need to wait for each context, we can
just skip context if watermark's metric isn't ready,
but if there's no contexts that can run we
check for each context's watermark metric and
sleep for the lowest interval of all contexts.
Signed-off-by: Alex Rusuf <[email protected]>
---
include/linux/damon.h | 11 +-
include/trace/events/damon.h | 14 +-
mm/damon/core-test.h | 2 +-
mm/damon/core.c | 286 +++++++++++++++++------------
mm/damon/dbgfs-test.h | 4 +-
mm/damon/dbgfs.c | 342 +++++++++++++++++++++--------------
mm/damon/modules-common.c | 1 -
mm/damon/sysfs.c | 47 +++--
8 files changed, 431 insertions(+), 276 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 7cb9979a0..2facf3a5f 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -575,7 +575,6 @@ struct damon_attrs {
* @lock: Kdamond's global lock, serializes accesses to any field.
* @self: Kernel thread which is actually being executed.
* @contexts: Head of contexts (&damon_ctx) list.
- * @nr_ctxs: Number of contexts being monitored.
*
* Each DAMON's background daemon has this structure. Once
* configured, daemon can be started by calling damon_start().
@@ -589,7 +588,6 @@ struct kdamond {
struct mutex lock;
struct task_struct *self;
struct list_head contexts;
- size_t nr_ctxs;
/* private: */
/* for waiting until the execution of the kdamond_fn is started */
@@ -634,7 +632,10 @@ struct damon_ctx {
* update
*/
unsigned long next_ops_update_sis;
+ /* upper limit for each monitoring region */
unsigned long sz_limit;
+ /* marker to check if context is valid */
+ bool valid;
/* public: */
struct kdamond *kdamond;
@@ -682,6 +683,12 @@ static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
}
+static inline bool damon_is_last_ctx(struct damon_ctx *ctx,
+ struct kdamond *kdamond)
+{
+ return list_is_last(&ctx->list, &kdamond->contexts);
+}
+
#define damon_for_each_region(r, t) \
list_for_each_entry(r, &t->regions_list, list)
diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
index 23200aabc..d5287566c 100644
--- a/include/trace/events/damon.h
+++ b/include/trace/events/damon.h
@@ -50,12 +50,13 @@ TRACE_EVENT_CONDITION(damos_before_apply,
TRACE_EVENT(damon_aggregated,
- TP_PROTO(unsigned int target_id, struct damon_region *r,
- unsigned int nr_regions),
+ TP_PROTO(unsigned int context_id, unsigned int target_id,
+ struct damon_region *r, unsigned int nr_regions),
- TP_ARGS(target_id, r, nr_regions),
+ TP_ARGS(context_id, target_id, r, nr_regions),
TP_STRUCT__entry(
+ __field(unsigned long, context_id)
__field(unsigned long, target_id)
__field(unsigned int, nr_regions)
__field(unsigned long, start)
@@ -65,6 +66,7 @@ TRACE_EVENT(damon_aggregated,
),
TP_fast_assign(
+ __entry->context_id = context_id;
__entry->target_id = target_id;
__entry->nr_regions = nr_regions;
__entry->start = r->ar.start;
@@ -73,9 +75,9 @@ TRACE_EVENT(damon_aggregated,
__entry->age = r->age;
),
- TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
- __entry->target_id, __entry->nr_regions,
- __entry->start, __entry->end,
+ TP_printk("context_id=%lu target_id=%lu nr_regions=%u %lu-%lu: %u %u",
+ __entry->context_id, __entry->target_id,
+ __entry->nr_regions, __entry->start, __entry->end,
__entry->nr_accesses, __entry->age)
);
diff --git a/mm/damon/core-test.h b/mm/damon/core-test.h
index 0cee634f3..7962c9a0e 100644
--- a/mm/damon/core-test.h
+++ b/mm/damon/core-test.h
@@ -99,7 +99,7 @@ static void damon_test_aggregate(struct kunit *test)
}
it++;
}
- kdamond_reset_aggregated(ctx);
+ kdamond_reset_aggregated(ctx, 0);
it = 0;
damon_for_each_target(t, ctx) {
ir = 0;
diff --git a/mm/damon/core.c b/mm/damon/core.c
index cfc9c803d..ad73752af 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -500,6 +500,8 @@ struct damon_ctx *damon_new_ctx(void)
ctx->attrs.min_nr_regions = 10;
ctx->attrs.max_nr_regions = 1000;
+ ctx->valid = true;
+
INIT_LIST_HEAD(&ctx->adaptive_targets);
INIT_LIST_HEAD(&ctx->schemes);
INIT_LIST_HEAD(&ctx->list);
@@ -513,7 +515,7 @@ struct damon_ctx *damon_new_ctx(void)
void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
{
list_add_tail(&ctx->list, &kdamond->contexts);
- ++kdamond->nr_ctxs;
+ ctx->kdamond = kdamond;
}
struct kdamond *damon_new_kdamond(void)
@@ -567,10 +569,8 @@ void damon_destroy_ctxs(struct kdamond *kdamond)
{
struct damon_ctx *c, *next;
- damon_for_each_context_safe(c, next, kdamond) {
+ damon_for_each_context_safe(c, next, kdamond)
damon_destroy_ctx(c);
- --kdamond->nr_ctxs;
- }
}
void damon_destroy_kdamond(struct kdamond *kdamond)
@@ -735,6 +735,20 @@ bool damon_kdamond_running(struct kdamond *kdamond)
return running;
}
+/**
+ * kdamond_nr_ctxs() - Return number of contexts for this kdamond.
+ */
+static int kdamond_nr_ctxs(struct kdamond *kdamond)
+{
+ struct list_head *pos;
+ int nr_ctxs = 0;
+
+ list_for_each(pos, &kdamond->contexts)
+ ++nr_ctxs;
+
+ return nr_ctxs;
+}
+
/* Returns the size upper limit for each monitoring region */
static unsigned long damon_region_sz_limit(struct damon_ctx *ctx)
{
@@ -793,11 +807,11 @@ static int __damon_start(struct kdamond *kdamond)
* @exclusive: exclusiveness of this contexts group
*
* This function starts a group of monitoring threads for a group of monitoring
- * contexts. One thread per each context is created and run in parallel. The
- * caller should handle synchronization between the threads by itself. If
- * @exclusive is true and a group of threads that created by other
+ * contexts. If @exclusive is true and a group of contexts that created by other
* 'damon_start()' call is currently running, this function does nothing but
- * returns -EBUSY.
+ * returns -EBUSY, if @exclusive is true and a given kdamond wants to run
+ * several contexts, then this function returns -EINVAL. kdamond can run
+ * exclusively only one context.
*
* Return: 0 on success, negative error code otherwise.
*/
@@ -806,10 +820,6 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
int err = 0;
BUG_ON(!kdamond);
- BUG_ON(!kdamond->nr_ctxs);
-
- if (kdamond->nr_ctxs != 1)
- return -EINVAL;
mutex_lock(&damon_lock);
if ((exclusive && nr_running_kdamonds) ||
@@ -818,6 +828,11 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
return -EBUSY;
}
+ if (exclusive && kdamond_nr_ctxs(kdamond) > 1) {
+ mutex_unlock(&damon_lock);
+ return -EINVAL;
+ }
+
err = __damon_start(kdamond);
if (err)
return err;
@@ -857,7 +872,7 @@ int damon_stop(struct kdamond *kdamond)
/*
* Reset the aggregated monitoring results ('nr_accesses' of each region).
*/
-static void kdamond_reset_aggregated(struct damon_ctx *c)
+static void kdamond_reset_aggregated(struct damon_ctx *c, unsigned int ci)
{
struct damon_target *t;
unsigned int ti = 0; /* target's index */
@@ -866,7 +881,7 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
struct damon_region *r;
damon_for_each_region(r, t) {
- trace_damon_aggregated(ti, r, damon_nr_regions(t));
+ trace_damon_aggregated(ci, ti, r, damon_nr_regions(t));
r->last_nr_accesses = r->nr_accesses;
r->nr_accesses = 0;
}
@@ -1033,21 +1048,15 @@ static bool damos_filter_out(struct damon_ctx *ctx, struct damon_target *t,
return false;
}
-static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
- struct damon_region *r, struct damos *s)
+static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c,
+ struct damon_target *t, struct damon_region *r,
+ struct damos *s)
{
struct damos_quota *quota = &s->quota;
unsigned long sz = damon_sz_region(r);
struct timespec64 begin, end;
unsigned long sz_applied = 0;
int err = 0;
- /*
- * We plan to support multiple context per kdamond, as DAMON sysfs
- * implies with 'nr_contexts' file. Nevertheless, only single context
- * per kdamond is supported for now. So, we can simply use '0' context
- * index here.
- */
- unsigned int cidx = 0;
struct damos *siter; /* schemes iterator */
unsigned int sidx = 0;
struct damon_target *titer; /* targets iterator */
@@ -1103,7 +1112,8 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
damos_update_stat(s, sz, sz_applied);
}
-static void damon_do_apply_schemes(struct damon_ctx *c,
+static void damon_do_apply_schemes(unsigned int ctx_id,
+ struct damon_ctx *c,
struct damon_target *t,
struct damon_region *r)
{
@@ -1128,7 +1138,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
if (!damos_valid_target(c, t, r, s))
continue;
- damos_apply_scheme(c, t, r, s);
+ damos_apply_scheme(ctx_id, c, t, r, s);
}
}
@@ -1309,7 +1319,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
quota->min_score = score;
}
-static void kdamond_apply_schemes(struct damon_ctx *c)
+static void kdamond_apply_schemes(struct damon_ctx *c, unsigned int ctx_id)
{
struct damon_target *t;
struct damon_region *r, *next_r;
@@ -1335,7 +1345,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
damon_for_each_target(t, c) {
damon_for_each_region_safe(r, next_r, t)
- damon_do_apply_schemes(c, t, r);
+ damon_do_apply_schemes(ctx_id, c, t, r);
}
damon_for_each_scheme(s, c) {
@@ -1505,22 +1515,35 @@ static void kdamond_split_regions(struct damon_ctx *ctx)
*
* Returns true if need to stop current monitoring.
*/
-static bool kdamond_need_stop(struct damon_ctx *ctx)
+static bool kdamond_need_stop(void)
{
- struct damon_target *t;
-
if (kthread_should_stop())
return true;
+ return false;
+}
+
+static bool kdamond_valid_ctx(struct damon_ctx *ctx)
+{
+ struct damon_target *t;
if (!ctx->ops.target_valid)
- return false;
+ return true;
damon_for_each_target(t, ctx) {
if (ctx->ops.target_valid(t))
- return false;
+ return true;
}
- return true;
+ return false;
+}
+
+static void kdamond_usleep(unsigned long usecs)
+{
+ /* See Documentation/timers/timers-howto.rst for the thresholds */
+ if (usecs > 20 * USEC_PER_MSEC)
+ schedule_timeout_idle(usecs_to_jiffies(usecs));
+ else
+ usleep_idle_range(usecs, usecs + 1);
}
static unsigned long damos_wmark_metric_value(enum damos_wmark_metric metric)
@@ -1569,41 +1592,25 @@ static unsigned long damos_wmark_wait_us(struct damos *scheme)
return 0;
}
-static void kdamond_usleep(unsigned long usecs)
-{
- /* See Documentation/timers/timers-howto.rst for the thresholds */
- if (usecs > 20 * USEC_PER_MSEC)
- schedule_timeout_idle(usecs_to_jiffies(usecs));
- else
- usleep_idle_range(usecs, usecs + 1);
-}
-
-/* Returns negative error code if it's not activated but should return */
-static int kdamond_wait_activation(struct damon_ctx *ctx)
+/**
+ * Returns minimum wait time for monitoring context if it hits watermarks,
+ * otherwise returns 0.
+ */
+static unsigned long kdamond_wmark_wait_time(struct damon_ctx *ctx)
{
struct damos *s;
unsigned long wait_time;
unsigned long min_wait_time = 0;
bool init_wait_time = false;
- while (!kdamond_need_stop(ctx)) {
- damon_for_each_scheme(s, ctx) {
- wait_time = damos_wmark_wait_us(s);
- if (!init_wait_time || wait_time < min_wait_time) {
- init_wait_time = true;
- min_wait_time = wait_time;
- }
+ damon_for_each_scheme(s, ctx) {
+ wait_time = damos_wmark_wait_us(s);
+ if (!init_wait_time || wait_time < min_wait_time) {
+ init_wait_time = true;
+ min_wait_time = wait_time;
}
- if (!min_wait_time)
- return 0;
-
- kdamond_usleep(min_wait_time);
-
- if (ctx->callback.after_wmarks_check &&
- ctx->callback.after_wmarks_check(ctx))
- break;
}
- return -EBUSY;
+ return min_wait_time;
}
static void kdamond_init_intervals_sis(struct damon_ctx *ctx)
@@ -1672,14 +1679,41 @@ static void kdamond_finish_ctxs(struct kdamond *kdamond)
kdamond_finish_ctx(c);
}
+static bool kdamond_prepare_access_checks_ctx(struct damon_ctx *ctx,
+ unsigned long *sample_interval,
+ unsigned long *min_wait_time)
+{
+ unsigned long wait_time = 0;
+
+ if (!ctx->valid || !kdamond_valid_ctx(ctx))
+ goto invalidate_ctx;
+
+ wait_time = kdamond_wmark_wait_time(ctx);
+ if (wait_time) {
+ if (!*min_wait_time || wait_time < *min_wait_time)
+ *min_wait_time = wait_time;
+ return false;
+ }
+
+ if (ctx->ops.prepare_access_checks)
+ ctx->ops.prepare_access_checks(ctx);
+ if (ctx->callback.after_sampling &&
+ ctx->callback.after_sampling(ctx))
+ goto invalidate_ctx;
+ *sample_interval = ctx->attrs.sample_interval;
+ return true;
+invalidate_ctx:
+ ctx->valid = false;
+ return false;
+}
+
/*
* The monitoring daemon that runs as a kernel thread
*/
static int kdamond_fn(void *data)
{
+ struct damon_ctx *ctx;
struct kdamond *kdamond = data;
- struct damon_ctx *ctx = damon_first_ctx(kdamond);
- unsigned int max_nr_accesses = 0;
pr_debug("kdamond (%d) starts\n", current->pid);
@@ -1687,69 +1721,85 @@ static int kdamond_fn(void *data)
if (!kdamond_init_ctxs(kdamond))
goto done;
- while (!kdamond_need_stop(ctx)) {
- /*
- * ctx->attrs and ctx->next_{aggregation,ops_update}_sis could
- * be changed from after_wmarks_check() or after_aggregation()
- * callbacks. Read the values here, and use those for this
- * iteration. That is, damon_set_attrs() updated new values
- * are respected from next iteration.
- */
- unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
- unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
- unsigned long sample_interval = ctx->attrs.sample_interval;
- unsigned long sz_limit = ctx->sz_limit;
-
- if (kdamond_wait_activation(ctx))
- break;
+ while (!kdamond_need_stop()) {
+ unsigned int ctx_id = 0;
+ unsigned long nr_valid_ctxs = 0;
+ unsigned long min_wait_time = 0;
+ unsigned long sample_interval = 0;
- if (ctx->ops.prepare_access_checks)
- ctx->ops.prepare_access_checks(ctx);
- if (ctx->callback.after_sampling &&
- ctx->callback.after_sampling(ctx))
- break;
+ damon_for_each_context(ctx, kdamond) {
+ if (kdamond_prepare_access_checks_ctx(ctx, &sample_interval,
+ &min_wait_time))
+ nr_valid_ctxs++;
+ }
+ if (!nr_valid_ctxs) {
+ if (!min_wait_time)
+ break;
+ kdamond_usleep(min_wait_time);
+ continue;
+ }
kdamond_usleep(sample_interval);
- ctx->passed_sample_intervals++;
- if (ctx->ops.check_accesses)
- max_nr_accesses = ctx->ops.check_accesses(ctx);
+ damon_for_each_context(ctx, kdamond) {
+ /*
+ * ctx->attrs and ctx->next_{aggregation,ops_update}_sis could
+ * be changed from after_wmarks_check() or after_aggregation()
+ * callbacks. Read the values here, and use those for this
+ * iteration. That is, damon_set_attrs() updated new values
+ * are respected from next iteration.
+ */
+ unsigned int max_nr_accesses = 0;
+ unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
+ unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
+ unsigned long sz_limit = ctx->sz_limit;
+ unsigned long sample_interval = ctx->attrs.sample_interval ?
+ ctx->attrs.sample_interval : 1;
+
+ if (!ctx->valid)
+ goto next_ctx;
+
+ ctx->passed_sample_intervals++;
+
+ if (ctx->ops.check_accesses)
+ max_nr_accesses = ctx->ops.check_accesses(ctx);
+
+ if (ctx->passed_sample_intervals == next_aggregation_sis) {
+ kdamond_merge_regions(ctx,
+ max_nr_accesses / 10,
+ sz_limit);
+ if (ctx->callback.after_aggregation &&
+ ctx->callback.after_aggregation(ctx))
+ goto next_ctx;
+ }
+
+ /*
+ * do kdamond_apply_schemes() after kdamond_merge_regions() if
+ * possible, to reduce overhead
+ */
+ if (!list_empty(&ctx->schemes))
+ kdamond_apply_schemes(ctx, ctx_id);
- if (ctx->passed_sample_intervals == next_aggregation_sis) {
- kdamond_merge_regions(ctx,
- max_nr_accesses / 10,
- sz_limit);
- if (ctx->callback.after_aggregation &&
- ctx->callback.after_aggregation(ctx))
- break;
- }
+ if (ctx->passed_sample_intervals == next_aggregation_sis) {
+ ctx->next_aggregation_sis = next_aggregation_sis +
+ ctx->attrs.aggr_interval / sample_interval;
- /*
- * do kdamond_apply_schemes() after kdamond_merge_regions() if
- * possible, to reduce overhead
- */
- if (!list_empty(&ctx->schemes))
- kdamond_apply_schemes(ctx);
-
- sample_interval = ctx->attrs.sample_interval ?
- ctx->attrs.sample_interval : 1;
- if (ctx->passed_sample_intervals == next_aggregation_sis) {
- ctx->next_aggregation_sis = next_aggregation_sis +
- ctx->attrs.aggr_interval / sample_interval;
-
- kdamond_reset_aggregated(ctx);
- kdamond_split_regions(ctx);
- if (ctx->ops.reset_aggregated)
- ctx->ops.reset_aggregated(ctx);
- }
+ kdamond_reset_aggregated(ctx, ctx_id);
+ kdamond_split_regions(ctx);
+ if (ctx->ops.reset_aggregated)
+ ctx->ops.reset_aggregated(ctx);
+ }
- if (ctx->passed_sample_intervals == next_ops_update_sis) {
- ctx->next_ops_update_sis = next_ops_update_sis +
- ctx->attrs.ops_update_interval /
- sample_interval;
- if (ctx->ops.update)
- ctx->ops.update(ctx);
- ctx->sz_limit = damon_region_sz_limit(ctx);
+ if (ctx->passed_sample_intervals == next_ops_update_sis) {
+ ctx->next_ops_update_sis = next_ops_update_sis +
+ ctx->attrs.ops_update_interval /
+ sample_interval;
+ if (ctx->ops.update)
+ ctx->ops.update(ctx);
+ ctx->sz_limit = damon_region_sz_limit(ctx);
+ }
+next_ctx:
+ ++ctx_id;
}
}
done:
diff --git a/mm/damon/dbgfs-test.h b/mm/damon/dbgfs-test.h
index 2d85217f5..52745ed1d 100644
--- a/mm/damon/dbgfs-test.h
+++ b/mm/damon/dbgfs-test.h
@@ -70,7 +70,7 @@ static void damon_dbgfs_test_str_to_ints(struct kunit *test)
static void damon_dbgfs_test_set_targets(struct kunit *test)
{
- struct damon_ctx *ctx = dbgfs_new_ctx();
+ struct damon_ctx *ctx = dbgfs_new_damon_ctx();
char buf[64];
/* Make DAMON consider target has no pid */
@@ -88,7 +88,7 @@ static void damon_dbgfs_test_set_targets(struct kunit *test)
sprint_target_ids(ctx, buf, 64);
KUNIT_EXPECT_STREQ(test, (char *)buf, "\n");
- dbgfs_destroy_ctx(ctx);
+ dbgfs_destroy_damon_ctx(ctx);
}
static void damon_dbgfs_test_set_init_regions(struct kunit *test)
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index 2461cfe2e..7dff8376b 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -20,9 +20,13 @@
"to DAMON_SYSFS. If you cannot, please report your usecase to " \
"[email protected] and [email protected].\n"
-static struct damon_ctx **dbgfs_ctxs;
-static int dbgfs_nr_ctxs;
-static struct dentry **dbgfs_dirs;
+struct damon_dbgfs_ctx {
+ struct kdamond *kdamond;
+ struct dentry *dbgfs_dir;
+ struct list_head list;
+};
+
+static LIST_HEAD(damon_dbgfs_ctxs);
static DEFINE_MUTEX(damon_dbgfs_lock);
static void damon_dbgfs_warn_deprecation(void)
@@ -30,6 +34,65 @@ static void damon_dbgfs_warn_deprecation(void)
pr_warn_once(DAMON_DBGFS_DEPRECATION_NOTICE);
}
+static struct damon_dbgfs_ctx *dbgfs_root_dbgfs_ctx(void)
+{
+ return list_first_entry(&damon_dbgfs_ctxs,
+ struct damon_dbgfs_ctx, list);
+}
+
+static void dbgfs_add_dbgfs_ctx(struct damon_dbgfs_ctx *dbgfs_ctx)
+{
+ list_add_tail(&dbgfs_ctx->list, &damon_dbgfs_ctxs);
+}
+
+static struct damon_dbgfs_ctx *
+dbgfs_lookup_dbgfs_ctx(struct dentry *dbgfs_dir)
+{
+ struct damon_dbgfs_ctx *dbgfs_ctx;
+
+ list_for_each_entry(dbgfs_ctx, &damon_dbgfs_ctxs, list)
+ if (dbgfs_ctx->dbgfs_dir == dbgfs_dir)
+ return dbgfs_ctx;
+ return NULL;
+}
+
+static void dbgfs_stop_kdamonds(void)
+{
+ struct damon_dbgfs_ctx *dbgfs_ctx;
+ int ret = 0;
+
+ list_for_each_entry(dbgfs_ctx, &damon_dbgfs_ctxs, list)
+ if (damon_kdamond_running(dbgfs_ctx->kdamond))
+ ret |= damon_stop(dbgfs_ctx->kdamond);
+ if (ret)
+ pr_err("%s: some running kdamond(s) failed to stop!\n", __func__);
+}
+
+
+static int dbgfs_start_kdamonds(void)
+{
+ int ret;
+ struct damon_dbgfs_ctx *dbgfs_ctx;
+
+ list_for_each_entry(dbgfs_ctx, &damon_dbgfs_ctxs, list) {
+ ret = damon_start(dbgfs_ctx->kdamond, false);
+ if (ret)
+ goto err_stop_kdamonds;
+ }
+ return 0;
+
+err_stop_kdamonds:
+ dbgfs_stop_kdamonds();
+ return ret;
+}
+
+static bool dbgfs_targets_empty(struct damon_dbgfs_ctx *dbgfs_ctx)
+{
+ struct damon_ctx *ctx = damon_first_ctx(dbgfs_ctx->kdamond);
+
+ return damon_targets_empty(ctx);
+}
+
/*
* Returns non-empty string on success, negative error code otherwise.
*/
@@ -60,15 +123,16 @@ static ssize_t dbgfs_attrs_read(struct file *file,
char __user *buf, size_t count, loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
char kbuf[128];
int ret;
- mutex_lock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
ret = scnprintf(kbuf, ARRAY_SIZE(kbuf), "%lu %lu %lu %lu %lu\n",
ctx->attrs.sample_interval, ctx->attrs.aggr_interval,
ctx->attrs.ops_update_interval,
ctx->attrs.min_nr_regions, ctx->attrs.max_nr_regions);
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
return simple_read_from_buffer(buf, count, ppos, kbuf, ret);
}
@@ -77,6 +141,7 @@ static ssize_t dbgfs_attrs_write(struct file *file,
const char __user *buf, size_t count, loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
struct damon_attrs attrs;
char *kbuf;
ssize_t ret;
@@ -94,8 +159,8 @@ static ssize_t dbgfs_attrs_write(struct file *file,
goto out;
}
- mutex_lock(&ctx->kdamond_lock);
- if (ctx->kdamond) {
+ mutex_lock(&kdamond->lock);
+ if (kdamond->self) {
ret = -EBUSY;
goto unlock_out;
}
@@ -104,7 +169,7 @@ static ssize_t dbgfs_attrs_write(struct file *file,
if (!ret)
ret = count;
unlock_out:
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
out:
kfree(kbuf);
return ret;
@@ -173,6 +238,7 @@ static ssize_t dbgfs_schemes_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
char *kbuf;
ssize_t len;
@@ -180,9 +246,9 @@ static ssize_t dbgfs_schemes_read(struct file *file, char __user *buf,
if (!kbuf)
return -ENOMEM;
- mutex_lock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
len = sprint_schemes(ctx, kbuf, count);
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
if (len < 0)
goto out;
len = simple_read_from_buffer(buf, count, ppos, kbuf, len);
@@ -298,6 +364,7 @@ static ssize_t dbgfs_schemes_write(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
char *kbuf;
struct damos **schemes;
ssize_t nr_schemes = 0, ret;
@@ -312,8 +379,8 @@ static ssize_t dbgfs_schemes_write(struct file *file, const char __user *buf,
goto out;
}
- mutex_lock(&ctx->kdamond_lock);
- if (ctx->kdamond) {
+ mutex_lock(&kdamond->lock);
+ if (kdamond->self) {
ret = -EBUSY;
goto unlock_out;
}
@@ -323,13 +390,16 @@ static ssize_t dbgfs_schemes_write(struct file *file, const char __user *buf,
nr_schemes = 0;
unlock_out:
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
free_schemes_arr(schemes, nr_schemes);
out:
kfree(kbuf);
return ret;
}
+#pragma GCC push_options
+#pragma GCC optimize("O0")
+
static ssize_t sprint_target_ids(struct damon_ctx *ctx, char *buf, ssize_t len)
{
struct damon_target *t;
@@ -360,18 +430,21 @@ static ssize_t dbgfs_target_ids_read(struct file *file,
char __user *buf, size_t count, loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
ssize_t len;
char ids_buf[320];
- mutex_lock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
len = sprint_target_ids(ctx, ids_buf, 320);
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
if (len < 0)
return len;
return simple_read_from_buffer(buf, count, ppos, ids_buf, len);
}
+#pragma GCC pop_options
+
/*
* Converts a string into an integers array
*
@@ -491,6 +564,7 @@ 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;
+ struct kdamond *kdamond = ctx->kdamond;
bool id_is_pid = true;
char *kbuf;
struct pid **target_pids = NULL;
@@ -514,8 +588,8 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
}
}
- mutex_lock(&ctx->kdamond_lock);
- if (ctx->kdamond) {
+ mutex_lock(&kdamond->lock);
+ if (kdamond->self) {
if (id_is_pid)
dbgfs_put_pids(target_pids, nr_targets);
ret = -EBUSY;
@@ -542,7 +616,7 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
ret = count;
unlock_out:
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
kfree(target_pids);
out:
kfree(kbuf);
@@ -575,6 +649,7 @@ static ssize_t dbgfs_init_regions_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
char *kbuf;
ssize_t len;
@@ -582,15 +657,15 @@ static ssize_t dbgfs_init_regions_read(struct file *file, char __user *buf,
if (!kbuf)
return -ENOMEM;
- mutex_lock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
if (ctx->kdamond) {
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
len = -EBUSY;
goto out;
}
len = sprint_init_regions(ctx, kbuf, count);
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
if (len < 0)
goto out;
len = simple_read_from_buffer(buf, count, ppos, kbuf, len);
@@ -670,6 +745,7 @@ static ssize_t dbgfs_init_regions_write(struct file *file,
loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
char *kbuf;
ssize_t ret = count;
int err;
@@ -678,8 +754,8 @@ static ssize_t dbgfs_init_regions_write(struct file *file,
if (IS_ERR(kbuf))
return PTR_ERR(kbuf);
- mutex_lock(&ctx->kdamond_lock);
- if (ctx->kdamond) {
+ mutex_lock(&kdamond->lock);
+ if (kdamond->self) {
ret = -EBUSY;
goto unlock_out;
}
@@ -689,7 +765,7 @@ static ssize_t dbgfs_init_regions_write(struct file *file,
ret = err;
unlock_out:
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
kfree(kbuf);
return ret;
}
@@ -698,6 +774,7 @@ static ssize_t dbgfs_kdamond_pid_read(struct file *file,
char __user *buf, size_t count, loff_t *ppos)
{
struct damon_ctx *ctx = file->private_data;
+ struct kdamond *kdamond = ctx->kdamond;
char *kbuf;
ssize_t len;
@@ -705,12 +782,12 @@ static ssize_t dbgfs_kdamond_pid_read(struct file *file,
if (!kbuf)
return -ENOMEM;
- mutex_lock(&ctx->kdamond_lock);
- if (ctx->kdamond)
- len = scnprintf(kbuf, count, "%d\n", ctx->kdamond->pid);
+ mutex_lock(&kdamond->lock);
+ if (kdamond->self)
+ len = scnprintf(kbuf, count, "%d\n", ctx->kdamond->self->pid);
else
len = scnprintf(kbuf, count, "none\n");
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
if (!len)
goto out;
len = simple_read_from_buffer(buf, count, ppos, kbuf, len);
@@ -773,19 +850,30 @@ static void dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
static void dbgfs_before_terminate(struct damon_ctx *ctx)
{
struct damon_target *t, *next;
+ struct kdamond *kdamond = ctx->kdamond;
if (!damon_target_has_pid(ctx))
return;
- mutex_lock(&ctx->kdamond_lock);
+ mutex_lock(&kdamond->lock);
damon_for_each_target_safe(t, next, ctx) {
put_pid(t->pid);
damon_destroy_target(t);
}
- mutex_unlock(&ctx->kdamond_lock);
+ mutex_unlock(&kdamond->lock);
+}
+
+static struct kdamond *dbgfs_new_kdamond(void)
+{
+ struct kdamond *kdamond;
+
+ kdamond = damon_new_kdamond();
+ if (!kdamond)
+ return NULL;
+ return kdamond;
}
-static struct damon_ctx *dbgfs_new_ctx(void)
+static struct damon_ctx *dbgfs_new_damon_ctx(void)
{
struct damon_ctx *ctx;
@@ -802,11 +890,19 @@ static struct damon_ctx *dbgfs_new_ctx(void)
return ctx;
}
-static void dbgfs_destroy_ctx(struct damon_ctx *ctx)
+static void dbgfs_destroy_damon_ctx(struct damon_ctx *ctx)
{
damon_destroy_ctx(ctx);
}
+static void dbgfs_destroy_dbgfs_ctx(struct damon_dbgfs_ctx *dbgfs_ctx)
+{
+ debugfs_remove(dbgfs_ctx->dbgfs_dir);
+ damon_destroy_kdamond(dbgfs_ctx->kdamond);
+ list_del(&dbgfs_ctx->list);
+ kfree(dbgfs_ctx);
+}
+
static ssize_t damon_dbgfs_deprecated_read(struct file *file,
char __user *buf, size_t count, loff_t *ppos)
{
@@ -824,47 +920,56 @@ static ssize_t damon_dbgfs_deprecated_read(struct file *file,
*/
static int dbgfs_mk_context(char *name)
{
- struct dentry *root, **new_dirs, *new_dir;
- struct damon_ctx **new_ctxs, *new_ctx;
+ int rc;
+ struct damon_dbgfs_ctx *dbgfs_root_ctx, *new_dbgfs_ctx;
+ struct dentry *root, *new_dir;
+ struct damon_ctx *new_ctx;
+ struct kdamond *new_kdamond;
if (damon_nr_running_ctxs())
return -EBUSY;
- new_ctxs = krealloc(dbgfs_ctxs, sizeof(*dbgfs_ctxs) *
- (dbgfs_nr_ctxs + 1), GFP_KERNEL);
- if (!new_ctxs)
+ new_dbgfs_ctx = kmalloc(sizeof(*new_dbgfs_ctx), GFP_KERNEL);
+ if (!new_dbgfs_ctx)
return -ENOMEM;
- dbgfs_ctxs = new_ctxs;
- new_dirs = krealloc(dbgfs_dirs, sizeof(*dbgfs_dirs) *
- (dbgfs_nr_ctxs + 1), GFP_KERNEL);
- if (!new_dirs)
- return -ENOMEM;
- dbgfs_dirs = new_dirs;
-
- root = dbgfs_dirs[0];
- if (!root)
- return -ENOENT;
+ rc = -ENOENT;
+ dbgfs_root_ctx = dbgfs_root_dbgfs_ctx();
+ if (!dbgfs_root_ctx || !dbgfs_root_ctx->dbgfs_dir)
+ goto destroy_new_dbgfs_ctx;
+ root = dbgfs_root_ctx->dbgfs_dir;
new_dir = debugfs_create_dir(name, root);
/* Below check is required for a potential duplicated name case */
- if (IS_ERR(new_dir))
- return PTR_ERR(new_dir);
- dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
-
- new_ctx = dbgfs_new_ctx();
- if (!new_ctx) {
- debugfs_remove(new_dir);
- dbgfs_dirs[dbgfs_nr_ctxs] = NULL;
- return -ENOMEM;
+ if (IS_ERR(new_dir)) {
+ rc = PTR_ERR(new_dir);
+ goto destroy_new_dbgfs_ctx;
}
+ new_dbgfs_ctx->dbgfs_dir = new_dir;
+
+ rc = -ENOMEM;
+ new_kdamond = damon_new_kdamond();
+ if (!new_kdamond)
+ goto destroy_new_dir;
+
+ new_ctx = dbgfs_new_damon_ctx();
+ if (!new_ctx)
+ goto destroy_new_kdamond;
+ damon_add_ctx(new_kdamond, new_ctx);
+ new_dbgfs_ctx->kdamond = new_kdamond;
- dbgfs_ctxs[dbgfs_nr_ctxs] = new_ctx;
- dbgfs_fill_ctx_dir(dbgfs_dirs[dbgfs_nr_ctxs],
- dbgfs_ctxs[dbgfs_nr_ctxs]);
- dbgfs_nr_ctxs++;
+ dbgfs_fill_ctx_dir(new_dir, new_ctx);
+ dbgfs_add_dbgfs_ctx(new_dbgfs_ctx);
return 0;
+
+destroy_new_kdamond:
+ damon_destroy_kdamond(new_kdamond);
+destroy_new_dir:
+ debugfs_remove(new_dir);
+destroy_new_dbgfs_ctx:
+ kfree(new_dbgfs_ctx);
+ return rc;
}
static ssize_t dbgfs_mk_context_write(struct file *file,
@@ -910,64 +1015,35 @@ static ssize_t dbgfs_mk_context_write(struct file *file,
*/
static int dbgfs_rm_context(char *name)
{
- struct dentry *root, *dir, **new_dirs;
+ struct dentry *root, *dir;
struct inode *inode;
- struct damon_ctx **new_ctxs;
- int i, j;
+ struct damon_dbgfs_ctx *dbgfs_root_ctx;
+ struct damon_dbgfs_ctx *dbgfs_ctx;
int ret = 0;
if (damon_nr_running_ctxs())
return -EBUSY;
- root = dbgfs_dirs[0];
- if (!root)
+ dbgfs_root_ctx = dbgfs_root_dbgfs_ctx();
+ if (!dbgfs_root_ctx || !dbgfs_root_ctx->dbgfs_dir)
return -ENOENT;
+ root = dbgfs_root_ctx->dbgfs_dir;
dir = debugfs_lookup(name, root);
if (!dir)
return -ENOENT;
+ dbgfs_ctx = dbgfs_lookup_dbgfs_ctx(dir);
+ if (!dbgfs_ctx)
+ return -ENOENT;
+
inode = d_inode(dir);
if (!S_ISDIR(inode->i_mode)) {
ret = -EINVAL;
goto out_dput;
}
+ dbgfs_destroy_dbgfs_ctx(dbgfs_ctx);
- new_dirs = kmalloc_array(dbgfs_nr_ctxs - 1, sizeof(*dbgfs_dirs),
- GFP_KERNEL);
- if (!new_dirs) {
- ret = -ENOMEM;
- goto out_dput;
- }
-
- new_ctxs = kmalloc_array(dbgfs_nr_ctxs - 1, sizeof(*dbgfs_ctxs),
- GFP_KERNEL);
- if (!new_ctxs) {
- ret = -ENOMEM;
- goto out_new_dirs;
- }
-
- for (i = 0, j = 0; i < dbgfs_nr_ctxs; i++) {
- if (dbgfs_dirs[i] == dir) {
- debugfs_remove(dbgfs_dirs[i]);
- dbgfs_destroy_ctx(dbgfs_ctxs[i]);
- continue;
- }
- new_dirs[j] = dbgfs_dirs[i];
- new_ctxs[j++] = dbgfs_ctxs[i];
- }
-
- kfree(dbgfs_dirs);
- kfree(dbgfs_ctxs);
-
- dbgfs_dirs = new_dirs;
- dbgfs_ctxs = new_ctxs;
- dbgfs_nr_ctxs--;
-
- goto out_dput;
-
-out_new_dirs:
- kfree(new_dirs);
out_dput:
dput(dir);
return ret;
@@ -1024,6 +1100,7 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
{
ssize_t ret;
char *kbuf;
+ struct damon_dbgfs_ctx *dbgfs_ctx;
kbuf = user_input_str(buf, count, ppos);
if (IS_ERR(kbuf))
@@ -1037,18 +1114,16 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
mutex_lock(&damon_dbgfs_lock);
if (!strncmp(kbuf, "on", count)) {
- int i;
-
- for (i = 0; i < dbgfs_nr_ctxs; i++) {
- if (damon_targets_empty(dbgfs_ctxs[i])) {
+ list_for_each_entry(dbgfs_ctx, &damon_dbgfs_ctxs, list) {
+ if (dbgfs_targets_empty(dbgfs_ctx)) {
kfree(kbuf);
mutex_unlock(&damon_dbgfs_lock);
return -EINVAL;
}
}
- ret = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs, true);
+ ret = dbgfs_start_kdamonds();
} else if (!strncmp(kbuf, "off", count)) {
- ret = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
+ dbgfs_stop_kdamonds();
} else {
ret = -EINVAL;
}
@@ -1088,27 +1163,20 @@ static const struct file_operations monitor_on_fops = {
static int __init __damon_dbgfs_init(void)
{
- struct dentry *dbgfs_root;
+ struct dentry *dbgfs_root_dir;
+ struct damon_dbgfs_ctx *dbgfs_root_ctx = dbgfs_root_dbgfs_ctx();
+ struct damon_ctx *damon_ctx = damon_first_ctx(dbgfs_root_ctx->kdamond);
const char * const file_names[] = {"mk_contexts", "rm_contexts",
"monitor_on_DEPRECATED", "DEPRECATED"};
const struct file_operations *fops[] = {&mk_contexts_fops,
&rm_contexts_fops, &monitor_on_fops, &deprecated_fops};
- int i;
-
- dbgfs_root = debugfs_create_dir("damon", NULL);
- for (i = 0; i < ARRAY_SIZE(file_names); i++)
- debugfs_create_file(file_names[i], 0600, dbgfs_root, NULL,
+ dbgfs_root_dir = debugfs_create_dir("damon", NULL);
+ for (int i = 0; i < ARRAY_SIZE(file_names); i++)
+ debugfs_create_file(file_names[i], 0600, dbgfs_root_dir, NULL,
fops[i]);
- dbgfs_fill_ctx_dir(dbgfs_root, dbgfs_ctxs[0]);
-
- dbgfs_dirs = kmalloc(sizeof(dbgfs_root), GFP_KERNEL);
- if (!dbgfs_dirs) {
- debugfs_remove(dbgfs_root);
- return -ENOMEM;
- }
- dbgfs_dirs[0] = dbgfs_root;
-
+ dbgfs_fill_ctx_dir(dbgfs_root_dir, damon_ctx);
+ dbgfs_root_ctx->dbgfs_dir = dbgfs_root_dir;
return 0;
}
@@ -1118,26 +1186,38 @@ static int __init __damon_dbgfs_init(void)
static int __init damon_dbgfs_init(void)
{
+ struct damon_dbgfs_ctx *dbgfs_ctx;
+ struct damon_ctx *damon_ctx;
int rc = -ENOMEM;
mutex_lock(&damon_dbgfs_lock);
- dbgfs_ctxs = kmalloc(sizeof(*dbgfs_ctxs), GFP_KERNEL);
- if (!dbgfs_ctxs)
+ dbgfs_ctx = kmalloc(sizeof(*dbgfs_ctx), GFP_KERNEL);
+ if (!dbgfs_ctx)
goto out;
- dbgfs_ctxs[0] = dbgfs_new_ctx();
- if (!dbgfs_ctxs[0]) {
- kfree(dbgfs_ctxs);
- goto out;
- }
- dbgfs_nr_ctxs = 1;
+
+ dbgfs_ctx->kdamond = dbgfs_new_kdamond();
+ if (!dbgfs_ctx->kdamond)
+ goto bad_kdamond;
+
+ damon_ctx = dbgfs_new_damon_ctx();
+ if (!damon_ctx)
+ goto destroy_kdamond;
+ damon_add_ctx(dbgfs_ctx->kdamond, damon_ctx);
+
+ dbgfs_add_dbgfs_ctx(dbgfs_ctx);
rc = __damon_dbgfs_init();
if (rc) {
- kfree(dbgfs_ctxs[0]);
- kfree(dbgfs_ctxs);
pr_err("%s: dbgfs init failed\n", __func__);
+ goto destroy_kdamond;
}
+ mutex_unlock(&damon_dbgfs_lock);
+ return 0;
+destroy_kdamond:
+ damon_destroy_kdamond(dbgfs_ctx->kdamond);
+bad_kdamond:
+ kfree(dbgfs_ctx);
out:
mutex_unlock(&damon_dbgfs_lock);
return rc;
diff --git a/mm/damon/modules-common.c b/mm/damon/modules-common.c
index 436bb7948..6a7c0a085 100644
--- a/mm/damon/modules-common.c
+++ b/mm/damon/modules-common.c
@@ -53,7 +53,6 @@ int damon_modules_new_paddr_kdamond(struct kdamond **kdamondp)
damon_destroy_kdamond(kdamond);
return err;
}
- kdamond->nr_ctxs = 1;
*kdamondp = kdamond;
return 0;
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index bfdb979e6..41ade0770 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -897,8 +897,7 @@ static ssize_t nr_contexts_store(struct kobject *kobj,
err = kstrtoint(buf, 0, &nr);
if (err)
return err;
- /* TODO: support multiple contexts per kdamond */
- if (nr < 0 || 1 < nr)
+ if (nr < 0)
return -EINVAL;
contexts = container_of(kobj, struct damon_sysfs_contexts, kobj);
@@ -1381,23 +1380,48 @@ static int damon_sysfs_apply_inputs(struct damon_ctx *ctx,
*/
static int damon_sysfs_commit_input(struct damon_sysfs_kdamond *sys_kdamond)
{
+ unsigned long ctx_id = 0;
struct damon_ctx *c;
- struct damon_sysfs_context *sysfs_ctx;
+ struct damon_sysfs_context **sysfs_ctxs;
int err;
if (!damon_sysfs_kdamond_running(sys_kdamond))
return -EINVAL;
- /* TODO: Support multiple contexts per kdamond */
- if (sys_kdamond->contexts->nr != 1)
- return -EINVAL;
- sysfs_ctx = sys_kdamond->contexts->contexts_arr[0];
+ sysfs_ctxs = sys_kdamond->contexts->contexts_arr;
damon_for_each_context(c, sys_kdamond->kdamond) {
+ struct damon_sysfs_context *sysfs_ctx = *sysfs_ctxs;
+ struct damon_sysfs_intervals *sys_intervals =
+ sysfs_ctx->attrs->intervals;;
+
+ if (sys_kdamond->contexts->nr > 1 &&
+ sys_intervals->sample_us != c->attrs.sample_interval) {
+ pr_err("context_id=%lu: "
+ "multiple contexts must have equal sample_interval\n",
+ ctx_id);
+ /*
+ * since multiple contexts expect equal
+ * sample_intervals, try to fix it here
+ */
+ sys_intervals->sample_us = c->attrs.sample_interval;
+ }
+
err = damon_sysfs_apply_inputs(c, sysfs_ctx);
if (err)
return err;
- ++sysfs_ctx;
+ ++sysfs_ctxs;
+
+ /* sysfs_ctx may be NIL, so check if it is the last */
+ if (sys_kdamond->contexts->nr > 1 && sysfs_ctxs &&
+ !damon_is_last_ctx(c, sys_kdamond->kdamond)) {
+ sysfs_ctx = *sysfs_ctxs;
+ sys_intervals = sysfs_ctx->attrs->intervals;
+ /* We somehow failed in fixing sample_interval above */
+ BUG_ON(sys_intervals->sample_us != c->attrs.sample_interval);
+ }
+ ++ctx_id;
}
+
return 0;
}
@@ -1410,9 +1434,6 @@ static int damon_sysfs_commit_schemes_quota_goals(
if (!damon_sysfs_kdamond_running(sysfs_kdamond))
return -EINVAL;
- /* TODO: Support multiple contexts per kdamond */
- if (sysfs_kdamond->contexts->nr != 1)
- return -EINVAL;
sysfs_ctxs = sysfs_kdamond->contexts->contexts_arr;
damon_for_each_context(c, sysfs_kdamond->kdamond) {
@@ -1598,7 +1619,6 @@ static struct kdamond *damon_sysfs_build_kdamond(
damon_destroy_kdamond(kdamond);
return ERR_PTR(PTR_ERR(ctx));
}
- ctx->kdamond = kdamond;
damon_add_ctx(kdamond, ctx);
}
return kdamond;
@@ -1613,9 +1633,6 @@ static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *sys_kdamond)
return -EBUSY;
if (damon_sysfs_cmd_request.kdamond == sys_kdamond)
return -EBUSY;
- /* TODO: support multiple contexts per kdamond */
- if (sys_kdamond->contexts->nr != 1)
- return -EINVAL;
if (sys_kdamond->kdamond)
damon_destroy_kdamond(sys_kdamond->kdamond);
--
2.42.0
Hi Alex,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.9]
[also build test WARNING on linus/master next-20240531]
[cannot apply to sj/damon/next akpm-mm/mm-everything v6.10-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alex-Rusuf/mm-damon-core-add-struct-kdamond-abstraction-layer/20240531-202756
base: v6.9
patch link: https://lore.kernel.org/r/20240531122320.909060-2-yorha.op%40gmail.com
patch subject: [PATCH v2 1/2] mm/damon/core: add 'struct kdamond' abstraction layer
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240601/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> mm/damon/core.c:511: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Adds newly allocated and configured @ctx to @kdamond.
>> mm/damon/core.c:729: warning: Function parameter or struct member 'kdamond' not described in 'damon_kdamond_running'
vim +511 mm/damon/core.c
509
510 /**
> 511 * Adds newly allocated and configured @ctx to @kdamond.
512 */
513 void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
514 {
515 list_add_tail(&ctx->list, &kdamond->contexts);
516 ++kdamond->nr_ctxs;
517 }
518
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Alex,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.9]
[also build test WARNING on linus/master next-20240531]
[cannot apply to sj/damon/next akpm-mm/mm-everything v6.10-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alex-Rusuf/mm-damon-core-add-struct-kdamond-abstraction-layer/20240531-202756
base: v6.9
patch link: https://lore.kernel.org/r/20240531122320.909060-3-yorha.op%40gmail.com
patch subject: [PATCH v2 2/2] mm/damon/core: implement multi-context support
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240601/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from mm/damon/dbgfs.c:10:
In file included from include/linux/damon.h:11:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from mm/damon/dbgfs.c:10:
In file included from include/linux/damon.h:11:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from mm/damon/dbgfs.c:10:
In file included from include/linux/damon.h:11:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from mm/damon/dbgfs.c:10:
In file included from include/linux/damon.h:11:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2210:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> mm/damon/dbgfs.c:400:13: warning: unknown pragma ignored [-Wunknown-pragmas]
400 | #pragma GCC push_options
| ^
mm/damon/dbgfs.c:401:13: warning: unknown pragma ignored [-Wunknown-pragmas]
401 | #pragma GCC optimize("O0")
| ^
mm/damon/dbgfs.c:446:13: warning: unknown pragma ignored [-Wunknown-pragmas]
446 | #pragma GCC pop_options
| ^
>> mm/damon/dbgfs.c:1125:13: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
1125 | } else if (!strncmp(kbuf, "off", count)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
mm/damon/dbgfs.c:1132:7: note: uninitialized use occurs here
1132 | if (!ret)
| ^~~
mm/damon/dbgfs.c:1125:9: note: remove the 'if' if its condition is always false
1125 | } else if (!strncmp(kbuf, "off", count)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1126 | dbgfs_stop_kdamonds();
| ~~~~~~~~~~~~~~~~~~~~~~
1127 | } else {
| ~~~~~~
mm/damon/dbgfs.c:1101:13: note: initialize the variable 'ret' to silence this warning
1101 | ssize_t ret;
| ^
| = 0
mm/damon/dbgfs.c:893:13: warning: unused function 'dbgfs_destroy_damon_ctx' [-Wunused-function]
893 | static void dbgfs_destroy_damon_ctx(struct damon_ctx *ctx)
| ^~~~~~~~~~~~~~~~~~~~~~~
12 warnings generated.
vim +400 mm/damon/dbgfs.c
399
> 400 #pragma GCC push_options
401 #pragma GCC optimize("O0")
402
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hello Alex,
On Fri, 31 May 2024 15:23:18 +0300 Alex Rusuf <[email protected]> wrote:
> Currently kdamond uses only one context per kthread
> and most of its time it sleeps, so utilizing several
> contexts can scale kdamond and allow it to use
> another set of operations.
>
> This patch-set implements support for multiple contexts
> per kdamond.
>
[...]
>
> ---
> Changes from v1 (https://lore.kernel.org/damon/[email protected]/)
> - Compatibility for DebugFS interface is kept
> - Kunit tests build/execution issues are fixed
> - Patches from v1 are sqaushed, so that consistency between patches is
> kept
My request was to avoid unnecessary temporal changes that will be removed in
next patches. Some of those are well removed in this version, but I still show
some. E.g., nr_contexts field. Also, this resulted in two big patches.
I'd also appreciate if you can separate changes into smaller ones of logical
single change. For example, changes for lru_sort.c, reclaim.c, and sysfs.c on
first patch could be much smaller in my opinion. Traceevent change can also be
separated from patch 2. Some of multi-context support seems mixed in patch 1.
I'd suggest below patches flow.
Patch 1: Introduce new struct and control functions for the struct. Don't
really use the struct and the functions.
Patch 2: Modify core.c to use the struct and implement multiple contexts
support. Minimize changes to core.c users. Just keep those work as before.
Don't implement multi contexts support on sysfs.c or trace events at this
point.
Patch 3: Update sysfs.c to support the multiple contexts.
Patch 4: Update trace events to better support it.
> - Added/Fixed comments about data structures/functions
Also, you don't need to put version history under '---' marker if it is a cover
letter. You can put it on the body.
>
> Alex Rusuf (2):
> mm/damon/core: add 'struct kdamond' abstraction layer
> mm/damon/core: implement multi-context support
I will try to put more detailed comments on each patch.
>
> include/linux/damon.h | 80 ++++--
> include/trace/events/damon.h | 14 +-
> mm/damon/core-test.h | 2 +-
> mm/damon/core.c | 509 ++++++++++++++++++++++-------------
> mm/damon/dbgfs-test.h | 4 +-
> mm/damon/dbgfs.c | 342 ++++++++++++++---------
> mm/damon/lru_sort.c | 31 ++-
> mm/damon/modules-common.c | 35 ++-
> mm/damon/modules-common.h | 3 +-
> mm/damon/reclaim.c | 30 ++-
> mm/damon/sysfs.c | 303 +++++++++++++--------
> 11 files changed, 872 insertions(+), 481 deletions(-)
>
> --
> 2.42.0
Thanks,
SJ
Hi Alex,
On Fri, 31 May 2024 15:23:19 +0300 Alex Rusuf <[email protected]> wrote:
> In current implementation kdamond tracks only 1
> context, that is kdamond _is_ damon_ctx what makes
> it very difficult to implement multiple contexts.
>
> This patch adds another level of abstraction, that is
> 'struct kdamond' - structure which represents kdamond itself.
> It holds references to all contexts organized in list.
>
> Few fields like ->kdamond_started and ->kdamond_lock
> (just ->lock for 'struct kdamond') also has been moved
> to 'struct kdamond', because they have nothing to do
> with the context itself, but with the whole kdamond
> daemon.
>
> Signed-off-by: Alex Rusuf <[email protected]>
> ---
> include/linux/damon.h | 73 ++++++++---
> mm/damon/core.c | 249 ++++++++++++++++++++++-------------
> mm/damon/lru_sort.c | 31 +++--
> mm/damon/modules-common.c | 36 +++--
> mm/damon/modules-common.h | 3 +-
> mm/damon/reclaim.c | 30 +++--
> mm/damon/sysfs.c | 268 ++++++++++++++++++++++++--------------
> 7 files changed, 463 insertions(+), 227 deletions(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 886d07294..7cb9979a0 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -568,29 +568,49 @@ struct damon_attrs {
> unsigned long max_nr_regions;
> };
>
> +/**
> + * struct kdamond - Represents a background daemon that is responsible
> + * for executing each context.
> + *
> + * @lock: Kdamond's global lock, serializes accesses to any field.
> + * @self: Kernel thread which is actually being executed.
> + * @contexts: Head of contexts (&damon_ctx) list.
> + * @nr_ctxs: Number of contexts being monitored.
> + *
> + * Each DAMON's background daemon has this structure. Once
> + * configured, daemon can be started by calling damon_start().
> + *
> + * Monitoring can be explicitly stopped by calling damon_stop(). Once
> + * daemon is terminated @self is set to NULL, so users can know if
> + * monitoring is stopped by reading @self pointer. Access to @self
> + * must also be protected by @lock.
> + */
> +struct kdamond {
> + struct mutex lock;
> + struct task_struct *self;
I'm not sure if 'self' is a good name. I'd prefer more clear name such as
'task', but this is not a strong opinion.
> + struct list_head contexts;
> + size_t nr_ctxs;
> +
> +/* private: */
> + /* for waiting until the execution of the kdamond_fn is started */
> + struct completion kdamond_started;
> +};
> +
> /**
> * struct damon_ctx - Represents a context for each monitoring. This is the
> * main interface that allows users to set the attributes and get the results
> * of the monitoring.
> *
> * @attrs: Monitoring attributes for accuracy/overhead control.
> - * @kdamond: Kernel thread who does the monitoring.
> - * @kdamond_lock: Mutex for the synchronizations with @kdamond.
> + * @kdamond: Back reference to daemon who is the owner of this context.
Why do we need this?
> + * @list: List head of siblings.
> *
> * For each monitoring context, one kernel thread for the monitoring is
> * created. The pointer to the thread is stored in @kdamond.
> *
> * Once started, the monitoring thread runs until explicitly required to be
> * terminated or every monitoring target is invalid. The validity of the
> - * targets is checked via the &damon_operations.target_valid of @ops. The
> - * termination can also be explicitly requested by calling damon_stop().
> - * The thread sets @kdamond to NULL when it terminates. Therefore, users can
> - * know whether the monitoring is ongoing or terminated by reading @kdamond.
> - * Reads and writes to @kdamond from outside of the monitoring thread must
> - * be protected by @kdamond_lock.
> - *
> - * Note that the monitoring thread protects only @kdamond via @kdamond_lock.
> - * Accesses to other fields must be protected by themselves.
> + * targets is checked via the &damon_operations.target_valid of @ops.
> *
> * @ops: Set of monitoring operations for given use cases.
> * @callback: Set of callbacks for monitoring events notifications.
> @@ -614,12 +634,11 @@ struct damon_ctx {
> * update
> */
> unsigned long next_ops_update_sis;
> - /* for waiting until the execution of the kdamond_fn is started */
> - struct completion kdamond_started;
> + unsigned long sz_limit;
What's this?
>
> /* public: */
> - struct task_struct *kdamond;
> - struct mutex kdamond_lock;
> + struct kdamond *kdamond;
> + struct list_head list;
>
> struct damon_operations ops;
> struct damon_callback callback;
> @@ -653,6 +672,15 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
> return r->ar.end - r->ar.start;
> }
>
> +static inline struct damon_target *damon_first_target(struct damon_ctx *ctx)
> +{
> + return list_first_entry(&ctx->adaptive_targets, struct damon_target, list);
> +}
> +
> +static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
> +{
> + return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
> +}
>
> #define damon_for_each_region(r, t) \
> list_for_each_entry(r, &t->regions_list, list)
> @@ -675,6 +703,12 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
> #define damon_for_each_scheme_safe(s, next, ctx) \
> list_for_each_entry_safe(s, next, &(ctx)->schemes, list)
>
> +#define damon_for_each_context(c, kdamond) \
> + list_for_each_entry(c, &(kdamond)->contexts, list)
> +
> +#define damon_for_each_context_safe(c, next, kdamond) \
> + list_for_each_entry_safe(c, next, &(kdamond)->contexts, list)
> +
> #define damos_for_each_quota_goal(goal, quota) \
> list_for_each_entry(goal, "a->goals, list)
>
> @@ -736,7 +770,12 @@ void damon_destroy_target(struct damon_target *t);
> unsigned int damon_nr_regions(struct damon_target *t);
>
> struct damon_ctx *damon_new_ctx(void);
> +void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx);
> +struct kdamond *damon_new_kdamond(void);
> void damon_destroy_ctx(struct damon_ctx *ctx);
> +void damon_destroy_ctxs(struct kdamond *kdamond);
Why do we need this? Wouldn't damon_destroy_kdamond() do that?
> +void damon_destroy_kdamond(struct kdamond *kdamond);
> +bool damon_kdamond_running(struct kdamond *kdamond);
Let's put kdamond control functions outside of functions for damon_ctx control,
like we do for damon_target and damon_ctx.
> int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs);
> void damon_set_schemes(struct damon_ctx *ctx,
> struct damos **schemes, ssize_t nr_schemes);
> @@ -758,8 +797,8 @@ static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs
> }
>
>
> -int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive);
> -int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> +int damon_start(struct kdamond *kdamond, bool exclusive);
> +int damon_stop(struct kdamond *kdamond);
>
> int damon_set_region_biggest_system_ram_default(struct damon_target *t,
> unsigned long *start, unsigned long *end);
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 6d503c1c1..cfc9c803d 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -24,7 +24,7 @@
> #endif
>
> static DEFINE_MUTEX(damon_lock);
> -static int nr_running_ctxs;
> +static int nr_running_kdamonds;
> static bool running_exclusive_ctxs;
>
> static DEFINE_MUTEX(damon_ops_lock);
> @@ -488,8 +488,6 @@ struct damon_ctx *damon_new_ctx(void)
> if (!ctx)
> return NULL;
>
> - init_completion(&ctx->kdamond_started);
> -
> ctx->attrs.sample_interval = 5 * 1000;
> ctx->attrs.aggr_interval = 100 * 1000;
> ctx->attrs.ops_update_interval = 60 * 1000 * 1000;
> @@ -499,17 +497,41 @@ struct damon_ctx *damon_new_ctx(void)
> ctx->next_aggregation_sis = 0;
> ctx->next_ops_update_sis = 0;
>
> - mutex_init(&ctx->kdamond_lock);
> -
> ctx->attrs.min_nr_regions = 10;
> ctx->attrs.max_nr_regions = 1000;
>
> INIT_LIST_HEAD(&ctx->adaptive_targets);
> INIT_LIST_HEAD(&ctx->schemes);
> + INIT_LIST_HEAD(&ctx->list);
>
> return ctx;
> }
>
> +/**
> + * Adds newly allocated and configured @ctx to @kdamond.
There's no guarantee if @ctx is newly allocated and configured. We can simply
say "Adds a DAMON context to a given DAMON thread".
And this may cause kerneldoc compilation warning. I think the format should be
fixed, and parameters should be commented.
> + */
> +void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
> +{
> + list_add_tail(&ctx->list, &kdamond->contexts);
> + ++kdamond->nr_ctxs;
> +}
> +
> +struct kdamond *damon_new_kdamond(void)
> +{
> + struct kdamond *kdamond;
> +
> + kdamond = kzalloc(sizeof(*kdamond), GFP_KERNEL);
> + if (!kdamond)
> + return NULL;
> +
> + init_completion(&kdamond->kdamond_started);
> + mutex_init(&kdamond->lock);
> +
> + INIT_LIST_HEAD(&kdamond->contexts);
Among the four fields of the struct, three are manually set here, and the final
one is set as zero by 'kzalloc()'. I'd prefer using 'kmalloc()' and manually
set all fields.
> +
> + return kdamond;
> +}
> +
> static void damon_destroy_targets(struct damon_ctx *ctx)
> {
> struct damon_target *t, *next_t;
> @@ -523,6 +545,11 @@ static void damon_destroy_targets(struct damon_ctx *ctx)
> damon_destroy_target(t);
> }
>
> +static inline void damon_del_ctx(struct damon_ctx *ctx)
> +{
> + list_del(&ctx->list);
> +}
> +
> void damon_destroy_ctx(struct damon_ctx *ctx)
> {
> struct damos *s, *next_s;
> @@ -532,9 +559,27 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
> damon_for_each_scheme_safe(s, next_s, ctx)
> damon_destroy_scheme(s);
>
> + damon_del_ctx(ctx);
> kfree(ctx);
> }
>
> +void damon_destroy_ctxs(struct kdamond *kdamond)
> +{
> + struct damon_ctx *c, *next;
> +
> + damon_for_each_context_safe(c, next, kdamond) {
> + damon_destroy_ctx(c);
> + --kdamond->nr_ctxs;
> + }
> +}
Is there explicit user of this function? If not, I'd prefer making this
'static', or simply embed in damon_destroy_kdamond() below.
> +
> +void damon_destroy_kdamond(struct kdamond *kdamond)
> +{
> + damon_destroy_ctxs(kdamond);
> + mutex_destroy(&kdamond->lock);
> + kfree(kdamond);
> +}
> +
> static unsigned int damon_age_for_new_attrs(unsigned int age,
> struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
> {
> @@ -667,13 +712,27 @@ void damon_set_schemes(struct damon_ctx *ctx, struct damos **schemes,
> */
> int damon_nr_running_ctxs(void)
> {
> - int nr_ctxs;
> + int nr_kdamonds;
>
> mutex_lock(&damon_lock);
> - nr_ctxs = nr_running_ctxs;
> + nr_kdamonds = nr_running_kdamonds;
> mutex_unlock(&damon_lock);
>
> - return nr_ctxs;
> + return nr_kdamonds;
Shouldn't we change the name of this function, too?
> +}
> +
> +/**
> + * damon_kdamond_running() - Return true if kdamond is running
> + * false otherwise.
> + */
> +bool damon_kdamond_running(struct kdamond *kdamond)
> +{
> + bool running;
> +
> + mutex_lock(&kdamond->lock);
> + running = kdamond->self != NULL;
> + mutex_unlock(&kdamond->lock);
> + return running;
> }
Seems this function is used by only DAMON sysfs interface. Don't implement
unnecessary public function. Implement it in mm/damon/sysfs.c please.
>
> /* Returns the size upper limit for each monitoring region */
> @@ -700,38 +759,37 @@ static int kdamond_fn(void *data);
>
> /*
> * __damon_start() - Starts monitoring with given context.
s/context/kdamond/? Maybe we can simply say "starts given kdamond".
> - * @ctx: monitoring context
> + * @kdamond: daemon to be started
> *
> * 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)
> +static int __damon_start(struct kdamond *kdamond)
> {
> int err = -EBUSY;
>
> - mutex_lock(&ctx->kdamond_lock);
> - if (!ctx->kdamond) {
> + mutex_lock(&kdamond->lock);
> + if (!kdamond->self) {
> err = 0;
> - reinit_completion(&ctx->kdamond_started);
> - ctx->kdamond = kthread_run(kdamond_fn, ctx, "kdamond.%d",
> - nr_running_ctxs);
> - if (IS_ERR(ctx->kdamond)) {
> - err = PTR_ERR(ctx->kdamond);
> - ctx->kdamond = NULL;
> + reinit_completion(&kdamond->kdamond_started);
> + kdamond->self = kthread_run(kdamond_fn, kdamond, "kdamond.%d",
> + nr_running_kdamonds);
> + if (IS_ERR(kdamond->self)) {
> + err = PTR_ERR(kdamond->self);
> + kdamond->self = NULL;
> } else {
> - wait_for_completion(&ctx->kdamond_started);
> + wait_for_completion(&kdamond->kdamond_started);
> }
> }
> - mutex_unlock(&ctx->kdamond_lock);
> + mutex_unlock(&kdamond->lock);
>
> return err;
> }
>
> /**
> * damon_start() - Starts the monitorings for a given group of contexts.
> - * @ctxs: an array of the pointers for contexts to start monitoring
> - * @nr_ctxs: size of @ctxs
> + * @kdamond: a daemon that contains list of monitoring contexts
> * @exclusive: exclusiveness of this contexts group
> *
> * This function starts a group of monitoring threads for a group of monitoring
> @@ -743,74 +801,59 @@ static int __damon_start(struct damon_ctx *ctx)
> *
> * Return: 0 on success, negative error code otherwise.
> */
> -int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
> +int damon_start(struct kdamond *kdamond, bool exclusive)
> {
> - int i;
> int err = 0;
>
> + BUG_ON(!kdamond);
> + BUG_ON(!kdamond->nr_ctxs);
checkpatch complains as below.
WARNING: Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants
#432: FILE: mm/damon/core.c:809:
+ BUG_ON(!kdamond->nr_ctxs);
I don't think we really need above BUG_ON() checks.
> +
> + if (kdamond->nr_ctxs != 1)
> + return -EINVAL;
Why do we need this? To avoid DAMON sysfs interface change? I think
mm/damon/sysfs.c does this check on its own, so this may not needed?
> +
> mutex_lock(&damon_lock);
> - if ((exclusive && nr_running_ctxs) ||
> + if ((exclusive && nr_running_kdamonds) ||
> (!exclusive && running_exclusive_ctxs)) {
> mutex_unlock(&damon_lock);
> return -EBUSY;
> }
>
> - for (i = 0; i < nr_ctxs; i++) {
> - err = __damon_start(ctxs[i]);
> - if (err)
> - break;
> - nr_running_ctxs++;
> - }
> - if (exclusive && nr_running_ctxs)
> + err = __damon_start(kdamond);
> + if (err)
> + return err;
> + nr_running_kdamonds++;
> +
> + if (exclusive && nr_running_kdamonds)
> running_exclusive_ctxs = true;
> mutex_unlock(&damon_lock);
>
> return err;
> }
>
> -/*
> - * __damon_stop() - Stops monitoring of a given context.
> - * @ctx: monitoring context
> +/**
> + * damon_stop() - Stops the monitorings for a given group of contexts.
Let's simply say "Stops a given DAMON thread"
> + * @kdamond: a daemon (that contains list of monitoring contexts)
> + * to be stopped.
> *
> * Return: 0 on success, negative error code otherwise.
> */
> -static int __damon_stop(struct damon_ctx *ctx)
> +int damon_stop(struct kdamond *kdamond)
> {
> struct task_struct *tsk;
>
> - mutex_lock(&ctx->kdamond_lock);
> - tsk = ctx->kdamond;
> + mutex_lock(&kdamond->lock);
> + tsk = kdamond->self;
> if (tsk) {
> get_task_struct(tsk);
> - mutex_unlock(&ctx->kdamond_lock);
> + mutex_unlock(&kdamond->lock);
> kthread_stop_put(tsk);
> return 0;
> }
> - mutex_unlock(&ctx->kdamond_lock);
> + mutex_unlock(&kdamond->lock);
>
> return -EPERM;
> }
>
> -/**
> - * damon_stop() - Stops the monitorings for a given group of contexts.
> - * @ctxs: an array of the pointers for contexts to stop monitoring
> - * @nr_ctxs: size of @ctxs
> - *
> - * Return: 0 on success, negative error code otherwise.
> - */
> -int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
> -{
> - int i, err = 0;
> -
> - for (i = 0; i < nr_ctxs; i++) {
> - /* nr_running_ctxs is decremented in kdamond_fn */
> - err = __damon_stop(ctxs[i]);
> - if (err)
> - break;
> - }
> - return err;
> -}
> -
> /*
> * Reset the aggregated monitoring results ('nr_accesses' of each region).
> */
> @@ -1582,29 +1625,68 @@ static void kdamond_init_intervals_sis(struct damon_ctx *ctx)
> }
> }
>
> +static bool kdamond_init_ctx(struct damon_ctx *ctx)
> +{
> + if (ctx->ops.init)
> + ctx->ops.init(ctx);
> + if (ctx->callback.before_start && ctx->callback.before_start(ctx))
> + return false;
> +
> + kdamond_init_intervals_sis(ctx);
> + ctx->sz_limit = damon_region_sz_limit(ctx);
> +
> + return true;
> +}
I think 'int' return type for error code is better. We can just return
before_start() return value in case of error, otherwise zero.
> +
> +static bool kdamond_init_ctxs(struct kdamond *kdamond)
> +{
> + struct damon_ctx *c;
> +
> + damon_for_each_context(c, kdamond)
> + if (!kdamond_init_ctx(c))
> + return false;
> + return true;
> +}
> +
> +static void kdamond_finish_ctx(struct damon_ctx *ctx)
> +{
> + struct damon_target *t;
> + struct damon_region *r, *next;
> +
> + damon_for_each_target(t, ctx) {
> + damon_for_each_region_safe(r, next, t)
> + damon_destroy_region(r, t);
> + }
> +
> + if (ctx->callback.before_terminate)
> + ctx->callback.before_terminate(ctx);
> + if (ctx->ops.cleanup)
> + ctx->ops.cleanup(ctx);
> +}
> +
> +static void kdamond_finish_ctxs(struct kdamond *kdamond)
> +{
> + struct damon_ctx *c;
> +
> + damon_for_each_context(c, kdamond)
> + kdamond_finish_ctx(c);
> +}
> +
> /*
> * The monitoring daemon that runs as a kernel thread
> */
> static int kdamond_fn(void *data)
> {
> - struct damon_ctx *ctx = data;
> - struct damon_target *t;
> - struct damon_region *r, *next;
> + struct kdamond *kdamond = data;
> + struct damon_ctx *ctx = damon_first_ctx(kdamond);
> unsigned int max_nr_accesses = 0;
> - unsigned long sz_limit = 0;
>
> pr_debug("kdamond (%d) starts\n", current->pid);
>
> - complete(&ctx->kdamond_started);
> - kdamond_init_intervals_sis(ctx);
> -
> - if (ctx->ops.init)
> - ctx->ops.init(ctx);
> - if (ctx->callback.before_start && ctx->callback.before_start(ctx))
> + complete(&kdamond->kdamond_started);
> + if (!kdamond_init_ctxs(kdamond))
> goto done;
>
> - sz_limit = damon_region_sz_limit(ctx);
> -
> while (!kdamond_need_stop(ctx)) {
> /*
> * ctx->attrs and ctx->next_{aggregation,ops_update}_sis could
> @@ -1616,6 +1698,7 @@ static int kdamond_fn(void *data)
> unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
> unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
> unsigned long sample_interval = ctx->attrs.sample_interval;
> + unsigned long sz_limit = ctx->sz_limit;
>
> if (kdamond_wait_activation(ctx))
> break;
> @@ -1666,28 +1749,20 @@ static int kdamond_fn(void *data)
> sample_interval;
> if (ctx->ops.update)
> ctx->ops.update(ctx);
> - sz_limit = damon_region_sz_limit(ctx);
> + ctx->sz_limit = damon_region_sz_limit(ctx);
> }
> }
> done:
> - damon_for_each_target(t, ctx) {
> - damon_for_each_region_safe(r, next, t)
> - damon_destroy_region(r, t);
> - }
> -
> - if (ctx->callback.before_terminate)
> - ctx->callback.before_terminate(ctx);
> - if (ctx->ops.cleanup)
> - ctx->ops.cleanup(ctx);
> + kdamond_finish_ctxs(kdamond);
>
> pr_debug("kdamond (%d) finishes\n", current->pid);
> - mutex_lock(&ctx->kdamond_lock);
> - ctx->kdamond = NULL;
> - mutex_unlock(&ctx->kdamond_lock);
> + mutex_lock(&kdamond->lock);
> + kdamond->self = NULL;
> + mutex_unlock(&kdamond->lock);
>
> mutex_lock(&damon_lock);
> - nr_running_ctxs--;
> - if (!nr_running_ctxs && running_exclusive_ctxs)
> + nr_running_kdamonds--;
> + if (!nr_running_kdamonds && running_exclusive_ctxs)
> running_exclusive_ctxs = false;
> mutex_unlock(&damon_lock);
>
> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 3de2916a6..76c20098a 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -142,8 +142,18 @@ static struct damos_access_pattern damon_lru_sort_stub_pattern = {
> .max_age_region = UINT_MAX,
> };
>
> -static struct damon_ctx *ctx;
> -static struct damon_target *target;
> +static struct kdamond *kdamond;
> +
> +static inline struct damon_ctx *damon_lru_sort_ctx(void)
> +{
> + return damon_first_ctx(kdamond);
> +}
> +
> +static inline struct damon_target *damon_lru_sort_target(void)
> +{
> + return damon_first_target(
> + damon_lru_sort_ctx());
> +}
Do we really need above two functions? Can't we simply add 'kdamond' global
variable but still uses 'ctx' and 'target' as before, except interface changed
function calls?
>
> static struct damos *damon_lru_sort_new_scheme(
> struct damos_access_pattern *pattern, enum damos_action action)
> @@ -201,6 +211,7 @@ static int damon_lru_sort_apply_parameters(void)
> struct damos *scheme, *hot_scheme, *cold_scheme;
> struct damos *old_hot_scheme = NULL, *old_cold_scheme = NULL;
> unsigned int hot_thres, cold_thres;
> + struct damon_ctx *ctx = damon_lru_sort_ctx();
> int err = 0;
>
> err = damon_set_attrs(ctx, &damon_lru_sort_mon_attrs);
> @@ -237,7 +248,8 @@ static int damon_lru_sort_apply_parameters(void)
> damon_set_schemes(ctx, &hot_scheme, 1);
> damon_add_scheme(ctx, cold_scheme);
>
> - return damon_set_region_biggest_system_ram_default(target,
> + return damon_set_region_biggest_system_ram_default(
> + damon_lru_sort_target(),
> &monitor_region_start,
> &monitor_region_end);
> }
> @@ -247,7 +259,7 @@ static int damon_lru_sort_turn(bool on)
> int err;
>
> if (!on) {
> - err = damon_stop(&ctx, 1);
> + err = damon_stop(kdamond);
> if (!err)
> kdamond_pid = -1;
> return err;
> @@ -257,10 +269,11 @@ static int damon_lru_sort_turn(bool on)
> if (err)
> return err;
>
> - err = damon_start(&ctx, 1, true);
> + err = damon_start(kdamond, true);
> if (err)
> return err;
> - kdamond_pid = ctx->kdamond->pid;
> +
> + kdamond_pid = kdamond->self->pid;
> return 0;
> }
>
> @@ -279,7 +292,7 @@ static int damon_lru_sort_enabled_store(const char *val,
> return 0;
>
> /* Called before init function. The function will handle this. */
> - if (!ctx)
> + if (!kdamond)
> goto set_param_out;
>
> err = damon_lru_sort_turn(enable);
> @@ -334,11 +347,13 @@ static int damon_lru_sort_after_wmarks_check(struct damon_ctx *c)
>
> static int __init damon_lru_sort_init(void)
> {
> - int err = damon_modules_new_paddr_ctx_target(&ctx, &target);
> + struct damon_ctx *ctx;
> + int err = damon_modules_new_paddr_kdamond(&kdamond);
I think we can allocate kdamond struct and set ctx/target here, and remove
unnecessary functions and changes.
>
> if (err)
> return err;
>
> + ctx = damon_lru_sort_ctx();
> ctx->callback.after_wmarks_check = damon_lru_sort_after_wmarks_check;
> ctx->callback.after_aggregation = damon_lru_sort_after_aggregation;
>
> diff --git a/mm/damon/modules-common.c b/mm/damon/modules-common.c
> index 7cf96574c..436bb7948 100644
> --- a/mm/damon/modules-common.c
> +++ b/mm/damon/modules-common.c
> @@ -9,13 +9,7 @@
>
> #include "modules-common.h"
>
> -/*
> - * Allocate, set, and return a DAMON context for the physical address space.
> - * @ctxp: Pointer to save the point to the newly created context
> - * @targetp: Pointer to save the point to the newly created target
> - */
> -int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
> - struct damon_target **targetp)
> +static int __damon_modules_new_paddr_kdamond(struct kdamond *kdamond)
> {
> struct damon_ctx *ctx;
> struct damon_target *target;
> @@ -34,9 +28,33 @@ int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
> damon_destroy_ctx(ctx);
> return -ENOMEM;
> }
> +
> damon_add_target(ctx, target);
> + damon_add_ctx(kdamond, ctx);
> +
> + return 0;
> +}
> +
> +/*
> + * Allocate, set, and return a DAMON daemon for the physical address space.
> + * @kdamondp: Pointer to save the point to the newly created kdamond
> + */
> +int damon_modules_new_paddr_kdamond(struct kdamond **kdamondp)
> +{
> + int err;
> + struct kdamond *kdamond;
> +
> + kdamond = damon_new_kdamond();
> + if (!kdamond)
> + return -ENOMEM;
> +
> + err = __damon_modules_new_paddr_kdamond(kdamond);
> + if (err) {
> + damon_destroy_kdamond(kdamond);
> + return err;
> + }
> + kdamond->nr_ctxs = 1;
>
> - *ctxp = ctx;
> - *targetp = target;
> + *kdamondp = kdamond;
> return 0;
> }
> diff --git a/mm/damon/modules-common.h b/mm/damon/modules-common.h
> index f49cdb417..5fc5b8ae3 100644
> --- a/mm/damon/modules-common.h
> +++ b/mm/damon/modules-common.h
> @@ -45,5 +45,4 @@
> module_param_named(nr_##qt_exceed_name, stat.qt_exceeds, ulong, \
> 0400);
>
> -int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
> - struct damon_target **targetp);
> +int damon_modules_new_paddr_kdamond(struct kdamond **kdamondp);
> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 9bd341d62..f6540ef1a 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c
> @@ -150,8 +150,18 @@ static struct damos_stat damon_reclaim_stat;
> DEFINE_DAMON_MODULES_DAMOS_STATS_PARAMS(damon_reclaim_stat,
> reclaim_tried_regions, reclaimed_regions, quota_exceeds);
>
> -static struct damon_ctx *ctx;
> -static struct damon_target *target;
> +static struct kdamond *kdamond;
> +
> +static inline struct damon_ctx *damon_reclaim_ctx(void)
> +{
> + return damon_first_ctx(kdamond);
> +}
> +
> +static inline struct damon_target *damon_reclaim_target(void)
> +{
> + return damon_first_target(
> + damon_reclaim_ctx());
> +}
Ditto.
>
> static struct damos *damon_reclaim_new_scheme(void)
> {
> @@ -197,6 +207,7 @@ static int damon_reclaim_apply_parameters(void)
> struct damos *scheme, *old_scheme;
> struct damos_quota_goal *goal;
> struct damos_filter *filter;
> + struct damon_ctx *ctx = damon_reclaim_ctx();
> int err = 0;
>
> err = damon_set_attrs(ctx, &damon_reclaim_mon_attrs);
> @@ -244,7 +255,8 @@ static int damon_reclaim_apply_parameters(void)
> }
> damon_set_schemes(ctx, &scheme, 1);
>
> - return damon_set_region_biggest_system_ram_default(target,
> + return damon_set_region_biggest_system_ram_default(
> + damon_reclaim_target(),
> &monitor_region_start,
> &monitor_region_end);
> }
> @@ -254,7 +266,7 @@ static int damon_reclaim_turn(bool on)
> int err;
>
> if (!on) {
> - err = damon_stop(&ctx, 1);
> + err = damon_stop(kdamond);
> if (!err)
> kdamond_pid = -1;
> return err;
> @@ -264,10 +276,10 @@ static int damon_reclaim_turn(bool on)
> if (err)
> return err;
>
> - err = damon_start(&ctx, 1, true);
> + err = damon_start(kdamond, true);
> if (err)
> return err;
> - kdamond_pid = ctx->kdamond->pid;
> + kdamond_pid = kdamond->self->pid;
> return 0;
> }
>
> @@ -286,7 +298,7 @@ static int damon_reclaim_enabled_store(const char *val,
> return 0;
>
> /* Called before init function. The function will handle this. */
> - if (!ctx)
> + if (!kdamond)
> goto set_param_out;
>
> err = damon_reclaim_turn(enable);
> @@ -337,11 +349,13 @@ static int damon_reclaim_after_wmarks_check(struct damon_ctx *c)
>
> static int __init damon_reclaim_init(void)
> {
> - int err = damon_modules_new_paddr_ctx_target(&ctx, &target);
> + struct damon_ctx *ctx;
> + int err = damon_modules_new_paddr_kdamond(&kdamond);
>
> if (err)
> return err;
>
> + ctx = damon_reclaim_ctx();
> ctx->callback.after_wmarks_check = damon_reclaim_after_wmarks_check;
> ctx->callback.after_aggregation = damon_reclaim_after_aggregation;
>
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 6fee383bc..bfdb979e6 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
Ditto. Many of below changes could be separated into another patch in my
opinion.
> @@ -939,7 +939,7 @@ static const struct kobj_type damon_sysfs_contexts_ktype = {
> struct damon_sysfs_kdamond {
> struct kobject kobj;
> struct damon_sysfs_contexts *contexts;
> - struct damon_ctx *damon_ctx;
> + struct kdamond *kdamond;
> };
>
> static struct damon_sysfs_kdamond *damon_sysfs_kdamond_alloc(void)
> @@ -974,16 +974,6 @@ static void damon_sysfs_kdamond_rm_dirs(struct damon_sysfs_kdamond *kdamond)
> kobject_put(&kdamond->contexts->kobj);
> }
>
> -static bool damon_sysfs_ctx_running(struct damon_ctx *ctx)
> -{
> - bool running;
> -
> - mutex_lock(&ctx->kdamond_lock);
> - running = ctx->kdamond != NULL;
> - mutex_unlock(&ctx->kdamond_lock);
> - return running;
> -}
Don't add replacement of this in mm/damon/core.c. Modify this here.
> -
> /*
> * enum damon_sysfs_cmd - Commands for a specific kdamond.
> */
> @@ -1065,15 +1055,15 @@ static struct damon_sysfs_cmd_request damon_sysfs_cmd_request;
> static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
> char *buf)
> {
> - struct damon_sysfs_kdamond *kdamond = container_of(kobj,
> + struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj,
> struct damon_sysfs_kdamond, kobj);
This makes reviewing bit difficult. Let's keep current name for now.
> - struct damon_ctx *ctx = kdamond->damon_ctx;
> + struct kdamond *kdamond = sys_kdamond->kdamond;
Keep 'kdamond' variable name as is, and use shorter name for this variable,
say, kd, or do kdamond->kdamond.
> bool running;
>
> - if (!ctx)
> + if (!kdamond)
"if (!kd)" or "if (!kdamond->kdamond)"
> running = false;
> else
> - running = damon_sysfs_ctx_running(ctx);
> + running = damon_kdamond_running(kdamond);
>
> return sysfs_emit(buf, "%s\n", running ?
> damon_sysfs_cmd_strs[DAMON_SYSFS_CMD_ON] :
> @@ -1242,13 +1232,15 @@ static bool damon_sysfs_schemes_regions_updating;
> static void damon_sysfs_before_terminate(struct damon_ctx *ctx)
> {
> struct damon_target *t, *next;
> - struct damon_sysfs_kdamond *kdamond;
> + struct damon_sysfs_kdamond *sys_kdamond;
> + struct kdamond *kdamond;
> enum damon_sysfs_cmd cmd;
>
> /* damon_sysfs_schemes_update_regions_stop() might not yet called */
> - kdamond = damon_sysfs_cmd_request.kdamond;
> + kdamond = ctx->kdamond;
> + sys_kdamond = damon_sysfs_cmd_request.kdamond;
> cmd = damon_sysfs_cmd_request.cmd;
> - if (kdamond && ctx == kdamond->damon_ctx &&
> + if (sys_kdamond && kdamond == sys_kdamond->kdamond &&
> (cmd == DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_REGIONS ||
> cmd == DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_BYTES) &&
> damon_sysfs_schemes_regions_updating) {
Again, changed names makes me reviewing difficult.
From this point, I think below changes could be significantly changed in next
version of this patchset if you agree to above comments. So I'm stop reviewing
this patch from here for now. Please let me know if you have different opinion
about it.
Thanks,
SJ
[...]
Hi Alex,
On Fri, 31 May 2024 15:23:20 +0300 Alex Rusuf <[email protected]> wrote:
> This patch actually implements support for
> multi-context design for kdamond daemon.
>
> In pseudo code previous versions worked like
> the following:
>
> while (!kdamond_should_stop()) {
>
> /* prepare accesses for only 1 context */
> prepare_accesses(damon_context);
>
> sleep(sample_interval);
>
> /* check accesses for only 1 context */
> check_accesses(damon_context);
>
> ...
> }
>
> With this patch kdamond workflow will look
> like the following:
>
> while (!kdamond_shoule_stop()) {
>
> /* prepare accesses for all contexts in kdamond */
> damon_for_each_context(ctx, kdamond)
> prepare_accesses(ctx);
>
> sleep(sample_interval);
>
> /* check_accesses for all contexts in kdamond */
> damon_for_each_context(ctx, kdamond)
> check_accesses(ctx);
>
> ...
> }
This is not what you do now, but on previous patch, right? Let's modify the
mesage appropriately.
>
> Another point to note is watermarks. Previous versions
> checked watermarks on each iteration for current context
> and if matric's value wan't acceptable kdamond waited
> for watermark's sleep interval.
Mention changes of versions on cover letter.
>
> Now there's no need to wait for each context, we can
> just skip context if watermark's metric isn't ready,
> but if there's no contexts that can run we
> check for each context's watermark metric and
> sleep for the lowest interval of all contexts.
>
> Signed-off-by: Alex Rusuf <[email protected]>
> ---
> include/linux/damon.h | 11 +-
> include/trace/events/damon.h | 14 +-
> mm/damon/core-test.h | 2 +-
> mm/damon/core.c | 286 +++++++++++++++++------------
> mm/damon/dbgfs-test.h | 4 +-
> mm/damon/dbgfs.c | 342 +++++++++++++++++++++--------------
> mm/damon/modules-common.c | 1 -
> mm/damon/sysfs.c | 47 +++--
> 8 files changed, 431 insertions(+), 276 deletions(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 7cb9979a0..2facf3a5f 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -575,7 +575,6 @@ struct damon_attrs {
> * @lock: Kdamond's global lock, serializes accesses to any field.
> * @self: Kernel thread which is actually being executed.
> * @contexts: Head of contexts (&damon_ctx) list.
> - * @nr_ctxs: Number of contexts being monitored.
> *
> * Each DAMON's background daemon has this structure. Once
> * configured, daemon can be started by calling damon_start().
> @@ -589,7 +588,6 @@ struct kdamond {
> struct mutex lock;
> struct task_struct *self;
> struct list_head contexts;
> - size_t nr_ctxs;
Why we add this on first patch, and then remove here? Let's not make
unnecessary temporal change. It only increase review time.
>
> /* private: */
> /* for waiting until the execution of the kdamond_fn is started */
> @@ -634,7 +632,10 @@ struct damon_ctx {
> * update
> */
> unsigned long next_ops_update_sis;
> + /* upper limit for each monitoring region */
> unsigned long sz_limit;
> + /* marker to check if context is valid */
> + bool valid;
What is the validity?
>
> /* public: */
> struct kdamond *kdamond;
> @@ -682,6 +683,12 @@ static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
> return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
> }
>
> +static inline bool damon_is_last_ctx(struct damon_ctx *ctx,
> + struct kdamond *kdamond)
> +{
> + return list_is_last(&ctx->list, &kdamond->contexts);
> +}
> +
> #define damon_for_each_region(r, t) \
> list_for_each_entry(r, &t->regions_list, list)
>
> diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> index 23200aabc..d5287566c 100644
> --- a/include/trace/events/damon.h
> +++ b/include/trace/events/damon.h
Let's separate this change to another patch.
> @@ -50,12 +50,13 @@ TRACE_EVENT_CONDITION(damos_before_apply,
>
> TRACE_EVENT(damon_aggregated,
>
> - TP_PROTO(unsigned int target_id, struct damon_region *r,
> - unsigned int nr_regions),
> + TP_PROTO(unsigned int context_id, unsigned int target_id,
> + struct damon_region *r, unsigned int nr_regions),
>
> - TP_ARGS(target_id, r, nr_regions),
> + TP_ARGS(context_id, target_id, r, nr_regions),
>
> TP_STRUCT__entry(
> + __field(unsigned long, context_id)
> __field(unsigned long, target_id)
> __field(unsigned int, nr_regions)
> __field(unsigned long, start)
> @@ -65,6 +66,7 @@ TRACE_EVENT(damon_aggregated,
> ),
>
> TP_fast_assign(
> + __entry->context_id = context_id;
> __entry->target_id = target_id;
> __entry->nr_regions = nr_regions;
> __entry->start = r->ar.start;
> @@ -73,9 +75,9 @@ TRACE_EVENT(damon_aggregated,
> __entry->age = r->age;
> ),
>
> - TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
> - __entry->target_id, __entry->nr_regions,
> - __entry->start, __entry->end,
> + TP_printk("context_id=%lu target_id=%lu nr_regions=%u %lu-%lu: %u %u",
> + __entry->context_id, __entry->target_id,
> + __entry->nr_regions, __entry->start, __entry->end,
> __entry->nr_accesses, __entry->age)
> );
>
> diff --git a/mm/damon/core-test.h b/mm/damon/core-test.h
> index 0cee634f3..7962c9a0e 100644
> --- a/mm/damon/core-test.h
> +++ b/mm/damon/core-test.h
> @@ -99,7 +99,7 @@ static void damon_test_aggregate(struct kunit *test)
> }
> it++;
> }
> - kdamond_reset_aggregated(ctx);
> + kdamond_reset_aggregated(ctx, 0);
> it = 0;
> damon_for_each_target(t, ctx) {
> ir = 0;
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index cfc9c803d..ad73752af 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -500,6 +500,8 @@ struct damon_ctx *damon_new_ctx(void)
> ctx->attrs.min_nr_regions = 10;
> ctx->attrs.max_nr_regions = 1000;
>
> + ctx->valid = true;
> +
> INIT_LIST_HEAD(&ctx->adaptive_targets);
> INIT_LIST_HEAD(&ctx->schemes);
> INIT_LIST_HEAD(&ctx->list);
> @@ -513,7 +515,7 @@ struct damon_ctx *damon_new_ctx(void)
> void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
> {
> list_add_tail(&ctx->list, &kdamond->contexts);
> - ++kdamond->nr_ctxs;
> + ctx->kdamond = kdamond;
> }
>
> struct kdamond *damon_new_kdamond(void)
> @@ -567,10 +569,8 @@ void damon_destroy_ctxs(struct kdamond *kdamond)
> {
> struct damon_ctx *c, *next;
>
> - damon_for_each_context_safe(c, next, kdamond) {
> + damon_for_each_context_safe(c, next, kdamond)
> damon_destroy_ctx(c);
> - --kdamond->nr_ctxs;
> - }
> }
>
> void damon_destroy_kdamond(struct kdamond *kdamond)
> @@ -735,6 +735,20 @@ bool damon_kdamond_running(struct kdamond *kdamond)
> return running;
> }
>
> +/**
> + * kdamond_nr_ctxs() - Return number of contexts for this kdamond.
> + */
> +static int kdamond_nr_ctxs(struct kdamond *kdamond)
> +{
> + struct list_head *pos;
> + int nr_ctxs = 0;
> +
> + list_for_each(pos, &kdamond->contexts)
> + ++nr_ctxs;
> +
> + return nr_ctxs;
> +}
> +
> /* Returns the size upper limit for each monitoring region */
> static unsigned long damon_region_sz_limit(struct damon_ctx *ctx)
> {
> @@ -793,11 +807,11 @@ static int __damon_start(struct kdamond *kdamond)
> * @exclusive: exclusiveness of this contexts group
> *
> * This function starts a group of monitoring threads for a group of monitoring
> - * contexts. One thread per each context is created and run in parallel. The
> - * caller should handle synchronization between the threads by itself. If
> - * @exclusive is true and a group of threads that created by other
> + * contexts. If @exclusive is true and a group of contexts that created by other
> * 'damon_start()' call is currently running, this function does nothing but
> - * returns -EBUSY.
> + * returns -EBUSY, if @exclusive is true and a given kdamond wants to run
> + * several contexts, then this function returns -EINVAL. kdamond can run
> + * exclusively only one context.
> *
> * Return: 0 on success, negative error code otherwise.
> */
> @@ -806,10 +820,6 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
> int err = 0;
>
> BUG_ON(!kdamond);
> - BUG_ON(!kdamond->nr_ctxs);
> -
> - if (kdamond->nr_ctxs != 1)
> - return -EINVAL;
>
> mutex_lock(&damon_lock);
> if ((exclusive && nr_running_kdamonds) ||
> @@ -818,6 +828,11 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
> return -EBUSY;
> }
>
> + if (exclusive && kdamond_nr_ctxs(kdamond) > 1) {
> + mutex_unlock(&damon_lock);
> + return -EINVAL;
> + }
> +
> err = __damon_start(kdamond);
> if (err)
> return err;
> @@ -857,7 +872,7 @@ int damon_stop(struct kdamond *kdamond)
> /*
> * Reset the aggregated monitoring results ('nr_accesses' of each region).
> */
> -static void kdamond_reset_aggregated(struct damon_ctx *c)
> +static void kdamond_reset_aggregated(struct damon_ctx *c, unsigned int ci)
> {
> struct damon_target *t;
> unsigned int ti = 0; /* target's index */
> @@ -866,7 +881,7 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
> struct damon_region *r;
>
> damon_for_each_region(r, t) {
> - trace_damon_aggregated(ti, r, damon_nr_regions(t));
> + trace_damon_aggregated(ci, ti, r, damon_nr_regions(t));
Separate traceevent change into another patch.
> r->last_nr_accesses = r->nr_accesses;
> r->nr_accesses = 0;
> }
> @@ -1033,21 +1048,15 @@ static bool damos_filter_out(struct damon_ctx *ctx, struct damon_target *t,
> return false;
> }
>
> -static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> - struct damon_region *r, struct damos *s)
> +static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c,
> + struct damon_target *t, struct damon_region *r,
> + struct damos *s)
Unnecesary change.
As also mentioned on the reply to the first patch, I think this patch can
significantly changed if you agree to my opinion about the flow of patches that
I mentioned on the reply to the cover letter. Hence, stopping review from here
for now. Please let me know if you have a different opinion.
Thanks,
SJ
[...]
Hi Alex,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.9]
[also build test WARNING on linus/master next-20240531]
[cannot apply to sj/damon/next akpm-mm/mm-everything v6.10-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alex-Rusuf/mm-damon-core-add-struct-kdamond-abstraction-layer/20240531-202756
base: v6.9
patch link: https://lore.kernel.org/r/20240531122320.909060-3-yorha.op%40gmail.com
patch subject: [PATCH v2 2/2] mm/damon/core: implement multi-context support
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240601/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
mm/damon/core.c:513: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Adds newly allocated and configured @ctx to @kdamond.
mm/damon/core.c:729: warning: Function parameter or struct member 'kdamond' not described in 'damon_kdamond_running'
>> mm/damon/core.c:742: warning: Function parameter or struct member 'kdamond' not described in 'kdamond_nr_ctxs'
mm/damon/core.c:1596: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Returns minimum wait time for monitoring context if it hits watermarks,
vim +742 mm/damon/core.c
737
738 /**
739 * kdamond_nr_ctxs() - Return number of contexts for this kdamond.
740 */
741 static int kdamond_nr_ctxs(struct kdamond *kdamond)
> 742 {
743 struct list_head *pos;
744 int nr_ctxs = 0;
745
746 list_for_each(pos, &kdamond->contexts)
747 ++nr_ctxs;
748
749 return nr_ctxs;
750 }
751
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi SJ,
> Hi Alex,
>
> On Fri, 31 May 2024 15:23:20 +0300 Alex Rusuf <[email protected]> wrote:
>
> > This patch actually implements support for
> > multi-context design for kdamond daemon.
> >
> > In pseudo code previous versions worked like
> > the following:
> >
> > while (!kdamond_should_stop()) {
> >
> > /* prepare accesses for only 1 context */
> > prepare_accesses(damon_context);
> >
> > sleep(sample_interval);
> >
> > /* check accesses for only 1 context */
> > check_accesses(damon_context);
> >
> > ...
> > }
> >
> > With this patch kdamond workflow will look
> > like the following:
> >
> > while (!kdamond_shoule_stop()) {
> >
> > /* prepare accesses for all contexts in kdamond */
> > damon_for_each_context(ctx, kdamond)
> > prepare_accesses(ctx);
> >
> > sleep(sample_interval);
> >
> > /* check_accesses for all contexts in kdamond */
> > damon_for_each_context(ctx, kdamond)
> > check_accesses(ctx);
> >
> > ...
> > }
>
> This is not what you do now, but on previous patch, right? Let's modify the
> mesage appropriately.
No, this is exactly what this patch is for, previous one only introduced
'struct kdamond' and made all interfaces (sysfs/dbgfs/core) to use it.
I mean previous patch doesn't include support for multiple contexts,
functionality was the same as before.
>
> >
> > Another point to note is watermarks. Previous versions
> > checked watermarks on each iteration for current context
> > and if matric's value wan't acceptable kdamond waited
> > for watermark's sleep interval.
>
> Mention changes of versions on cover letter.
>
> >
> > Now there's no need to wait for each context, we can
> > just skip context if watermark's metric isn't ready,
> > but if there's no contexts that can run we
> > check for each context's watermark metric and
> > sleep for the lowest interval of all contexts.
> >
> > Signed-off-by: Alex Rusuf <[email protected]>
> > ---
> > include/linux/damon.h | 11 +-
> > include/trace/events/damon.h | 14 +-
> > mm/damon/core-test.h | 2 +-
> > mm/damon/core.c | 286 +++++++++++++++++------------
> > mm/damon/dbgfs-test.h | 4 +-
> > mm/damon/dbgfs.c | 342 +++++++++++++++++++++--------------
> > mm/damon/modules-common.c | 1 -
> > mm/damon/sysfs.c | 47 +++--
> > 8 files changed, 431 insertions(+), 276 deletions(-)
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 7cb9979a0..2facf3a5f 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -575,7 +575,6 @@ struct damon_attrs {
> > * @lock: Kdamond's global lock, serializes accesses to any field.
> > * @self: Kernel thread which is actually being executed.
> > * @contexts: Head of contexts (&damon_ctx) list.
> > - * @nr_ctxs: Number of contexts being monitored.
> > *
> > * Each DAMON's background daemon has this structure. Once
> > * configured, daemon can be started by calling damon_start().
> > @@ -589,7 +588,6 @@ struct kdamond {
> > struct mutex lock;
> > struct task_struct *self;
> > struct list_head contexts;
> > - size_t nr_ctxs;
>
> Why we add this on first patch, and then remove here? Let's not make
> unnecessary temporal change. It only increase review time.
This temporal change is needed only to not break DAMON functionality
once first patch is applied (i.e. only 1st patch is applied). I mean
to have constistancy between patches.
I think, if I change the patch-history (the one you suggested in another
reply) this could be avoided.
>
> >
> > /* private: */
> > /* for waiting until the execution of the kdamond_fn is started */
> > @@ -634,7 +632,10 @@ struct damon_ctx {
> > * update
> > */
> > unsigned long next_ops_update_sis;
> > + /* upper limit for each monitoring region */
> > unsigned long sz_limit;
> > + /* marker to check if context is valid */
> > + bool valid;
>
> What is the validity?
This is a "flag" which indicates that the context is "valid" for kdamond
to be able to call ->check_accesses() on it. Because we have many contexts
we need a way to identify which context is valid while iterating over
all of them (please, see kdamond_prepare_access_checks_ctx() function).
Maybe name should be changed, but now I don't see a way how we could merge
this into kdamond_valid_ctx() or so, because the main cause of this "valid"
bit is that there's no more "valid" targets for this context, but also we could
have ->after_sampling() returned something bad.
>
> >
> > /* public: */
> > struct kdamond *kdamond;
> > @@ -682,6 +683,12 @@ static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
> > return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
> > }
> >
> > +static inline bool damon_is_last_ctx(struct damon_ctx *ctx,
> > + struct kdamond *kdamond)
> > +{
> > + return list_is_last(&ctx->list, &kdamond->contexts);
> > +}
> > +
> > #define damon_for_each_region(r, t) \
> > list_for_each_entry(r, &t->regions_list, list)
> >
> > diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> > index 23200aabc..d5287566c 100644
> > --- a/include/trace/events/damon.h
> > +++ b/include/trace/events/damon.h
>
> Let's separate this change to another patch.
Separating patches we hardly be able to reach at least build
consistency between patches. Moreover DAMON won't be able
to function corretly in between.
>
> > @@ -50,12 +50,13 @@ TRACE_EVENT_CONDITION(damos_before_apply,
> >
> > TRACE_EVENT(damon_aggregated,
> >
> > - TP_PROTO(unsigned int target_id, struct damon_region *r,
> > - unsigned int nr_regions),
> > + TP_PROTO(unsigned int context_id, unsigned int target_id,
> > + struct damon_region *r, unsigned int nr_regions),
> >
> > - TP_ARGS(target_id, r, nr_regions),
> > + TP_ARGS(context_id, target_id, r, nr_regions),
> >
> > TP_STRUCT__entry(
> > + __field(unsigned long, context_id)
> > __field(unsigned long, target_id)
> > __field(unsigned int, nr_regions)
> > __field(unsigned long, start)
> > @@ -65,6 +66,7 @@ TRACE_EVENT(damon_aggregated,
> > ),
> >
> > TP_fast_assign(
> > + __entry->context_id = context_id;
> > __entry->target_id = target_id;
> > __entry->nr_regions = nr_regions;
> > __entry->start = r->ar.start;
> > @@ -73,9 +75,9 @@ TRACE_EVENT(damon_aggregated,
> > __entry->age = r->age;
> > ),
> >
> > - TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
> > - __entry->target_id, __entry->nr_regions,
> > - __entry->start, __entry->end,
> > + TP_printk("context_id=%lu target_id=%lu nr_regions=%u %lu-%lu: %u %u",
> > + __entry->context_id, __entry->target_id,
> > + __entry->nr_regions, __entry->start, __entry->end,
> > __entry->nr_accesses, __entry->age)
> > );
> >
> > diff --git a/mm/damon/core-test.h b/mm/damon/core-test.h
> > index 0cee634f3..7962c9a0e 100644
> > --- a/mm/damon/core-test.h
> > +++ b/mm/damon/core-test.h
> > @@ -99,7 +99,7 @@ static void damon_test_aggregate(struct kunit *test)
> > }
> > it++;
> > }
> > - kdamond_reset_aggregated(ctx);
> > + kdamond_reset_aggregated(ctx, 0);
> > it = 0;
> > damon_for_each_target(t, ctx) {
> > ir = 0;
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index cfc9c803d..ad73752af 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -500,6 +500,8 @@ struct damon_ctx *damon_new_ctx(void)
> > ctx->attrs.min_nr_regions = 10;
> > ctx->attrs.max_nr_regions = 1000;
> >
> > + ctx->valid = true;
> > +
> > INIT_LIST_HEAD(&ctx->adaptive_targets);
> > INIT_LIST_HEAD(&ctx->schemes);
> > INIT_LIST_HEAD(&ctx->list);
> > @@ -513,7 +515,7 @@ struct damon_ctx *damon_new_ctx(void)
> > void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
> > {
> > list_add_tail(&ctx->list, &kdamond->contexts);
> > - ++kdamond->nr_ctxs;
> > + ctx->kdamond = kdamond;
> > }
> >
> > struct kdamond *damon_new_kdamond(void)
> > @@ -567,10 +569,8 @@ void damon_destroy_ctxs(struct kdamond *kdamond)
> > {
> > struct damon_ctx *c, *next;
> >
> > - damon_for_each_context_safe(c, next, kdamond) {
> > + damon_for_each_context_safe(c, next, kdamond)
> > damon_destroy_ctx(c);
> > - --kdamond->nr_ctxs;
> > - }
> > }
> >
> > void damon_destroy_kdamond(struct kdamond *kdamond)
> > @@ -735,6 +735,20 @@ bool damon_kdamond_running(struct kdamond *kdamond)
> > return running;
> > }
> >
> > +/**
> > + * kdamond_nr_ctxs() - Return number of contexts for this kdamond.
> > + */
> > +static int kdamond_nr_ctxs(struct kdamond *kdamond)
> > +{
> > + struct list_head *pos;
> > + int nr_ctxs = 0;
> > +
> > + list_for_each(pos, &kdamond->contexts)
> > + ++nr_ctxs;
> > +
> > + return nr_ctxs;
> > +}
> > +
> > /* Returns the size upper limit for each monitoring region */
> > static unsigned long damon_region_sz_limit(struct damon_ctx *ctx)
> > {
> > @@ -793,11 +807,11 @@ static int __damon_start(struct kdamond *kdamond)
> > * @exclusive: exclusiveness of this contexts group
> > *
> > * This function starts a group of monitoring threads for a group of monitoring
> > - * contexts. One thread per each context is created and run in parallel. The
> > - * caller should handle synchronization between the threads by itself. If
> > - * @exclusive is true and a group of threads that created by other
> > + * contexts. If @exclusive is true and a group of contexts that created by other
> > * 'damon_start()' call is currently running, this function does nothing but
> > - * returns -EBUSY.
> > + * returns -EBUSY, if @exclusive is true and a given kdamond wants to run
> > + * several contexts, then this function returns -EINVAL. kdamond can run
> > + * exclusively only one context.
> > *
> > * Return: 0 on success, negative error code otherwise.
> > */
> > @@ -806,10 +820,6 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
> > int err = 0;
> >
> > BUG_ON(!kdamond);
> > - BUG_ON(!kdamond->nr_ctxs);
> > -
> > - if (kdamond->nr_ctxs != 1)
> > - return -EINVAL;
> >
> > mutex_lock(&damon_lock);
> > if ((exclusive && nr_running_kdamonds) ||
> > @@ -818,6 +828,11 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
> > return -EBUSY;
> > }
> >
> > + if (exclusive && kdamond_nr_ctxs(kdamond) > 1) {
> > + mutex_unlock(&damon_lock);
> > + return -EINVAL;
> > + }
> > +
> > err = __damon_start(kdamond);
> > if (err)
> > return err;
> > @@ -857,7 +872,7 @@ int damon_stop(struct kdamond *kdamond)
> > /*
> > * Reset the aggregated monitoring results ('nr_accesses' of each region).
> > */
> > -static void kdamond_reset_aggregated(struct damon_ctx *c)
> > +static void kdamond_reset_aggregated(struct damon_ctx *c, unsigned int ci)
> > {
> > struct damon_target *t;
> > unsigned int ti = 0; /* target's index */
> > @@ -866,7 +881,7 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
> > struct damon_region *r;
> >
> > damon_for_each_region(r, t) {
> > - trace_damon_aggregated(ti, r, damon_nr_regions(t));
> > + trace_damon_aggregated(ci, ti, r, damon_nr_regions(t));
>
> Separate traceevent change into another patch.
>
> > r->last_nr_accesses = r->nr_accesses;
> > r->nr_accesses = 0;
> > }
> > @@ -1033,21 +1048,15 @@ static bool damos_filter_out(struct damon_ctx *ctx, struct damon_target *t,
> > return false;
> > }
> >
> > -static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> > - struct damon_region *r, struct damos *s)
> > +static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c,
> > + struct damon_target *t, struct damon_region *r,
> > + struct damos *s)
>
> Unnecesary change.
What do you mean? Why is this unnecessary? Now DAMON iterates over
contexts and calls kdamond_apply_schemes(ctx), so how can we know
which context we print trace for? Sorry, maybe I misunderstood
how DAMON does it, but I'm a bit confused.
Or maybe you mean indentation? The actual change is adding "cidx":
-static void damos_apply_scheme(struct damon_ctx *c, ...
+static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c,
>
> As also mentioned on the reply to the first patch, I think this patch can
> significantly changed if you agree to my opinion about the flow of patches that
> I mentioned on the reply to the cover letter. Hence, stopping review from here
> for now. Please let me know if you have a different opinion.
I see. Anyway, thank you for your comments, I'll try my best to improve the patch.
>
>
> Thanks,
> SJ
>
> [...]
BR,
Alex
Hi SJ,
> Hello Alex,
>
> On Fri, 31 May 2024 15:23:18 +0300 Alex Rusuf <[email protected]> wrote:
>
> > Currently kdamond uses only one context per kthread
> > and most of its time it sleeps, so utilizing several
> > contexts can scale kdamond and allow it to use
> > another set of operations.
> >
> > This patch-set implements support for multiple contexts
> > per kdamond.
> >
> [...]
> >
> > ---
> > Changes from v1 (https://lore.kernel.org/damon/[email protected]/)
> > - Compatibility for DebugFS interface is kept
> > - Kunit tests build/execution issues are fixed
> > - Patches from v1 are sqaushed, so that consistency between patches is
> > kept
>
> My request was to avoid unnecessary temporal changes that will be removed in
> next patches. Some of those are well removed in this version, but I still show
> some. E.g., nr_contexts field. Also, this resulted in two big patches.
This makes sense and I actually wanted that as well, so I tried to separate
them in previous version, looks like I misunderstood your request.
Anyway, don't you mind if lru_sort/traceevents/etc. will not function
correctly without applying the whole patch-set? I mean if we use the
approach below, once core.c is modified at least lru_sort and reclaim
will not work correctly, they even will not be built.
>
> I'd also appreciate if you can separate changes into smaller ones of logical
> single change. For example, changes for lru_sort.c, reclaim.c, and sysfs.c on
> first patch could be much smaller in my opinion. Traceevent change can also be
> separated from patch 2. Some of multi-context support seems mixed in patch 1.
>
> I'd suggest below patches flow.
>
> Patch 1: Introduce new struct and control functions for the struct. Don't
> really use the struct and the functions.
>
> Patch 2: Modify core.c to use the struct and implement multiple contexts
> support. Minimize changes to core.c users. Just keep those work as before.
> Don't implement multi contexts support on sysfs.c or trace events at this
> point.
>
> Patch 3: Update sysfs.c to support the multiple contexts.
>
> Patch 4: Update trace events to better support it.
>
> > - Added/Fixed comments about data structures/functions
>
> Also, you don't need to put version history under '---' marker if it is a cover
> letter. You can put it on the body.
>
> >
> > Alex Rusuf (2):
> > mm/damon/core: add 'struct kdamond' abstraction layer
> > mm/damon/core: implement multi-context support
>
> I will try to put more detailed comments on each patch.
>
> >
> > include/linux/damon.h | 80 ++++--
> > include/trace/events/damon.h | 14 +-
> > mm/damon/core-test.h | 2 +-
> > mm/damon/core.c | 509 ++++++++++++++++++++++-------------
> > mm/damon/dbgfs-test.h | 4 +-
> > mm/damon/dbgfs.c | 342 ++++++++++++++---------
> > mm/damon/lru_sort.c | 31 ++-
> > mm/damon/modules-common.c | 35 ++-
> > mm/damon/modules-common.h | 3 +-
> > mm/damon/reclaim.c | 30 ++-
> > mm/damon/sysfs.c | 303 +++++++++++++--------
> > 11 files changed, 872 insertions(+), 481 deletions(-)
> >
> > --
> > 2.42.0
>
>
> Thanks,
> SJ
BR,
Alex
On Sun, 2 Jun 2024 18:08:16 +0300 Alex Rusuf <[email protected]> wrote:
> Hi SJ,
>
> > Hi Alex,
> >
> > On Fri, 31 May 2024 15:23:20 +0300 Alex Rusuf <[email protected]> wrote:
> >
> > > This patch actually implements support for
> > > multi-context design for kdamond daemon.
> > >
> > > In pseudo code previous versions worked like
> > > the following:
> > >
> > > while (!kdamond_should_stop()) {
> > >
> > > /* prepare accesses for only 1 context */
> > > prepare_accesses(damon_context);
> > >
> > > sleep(sample_interval);
> > >
> > > /* check accesses for only 1 context */
> > > check_accesses(damon_context);
> > >
> > > ...
> > > }
> > >
> > > With this patch kdamond workflow will look
> > > like the following:
> > >
> > > while (!kdamond_shoule_stop()) {
> > >
> > > /* prepare accesses for all contexts in kdamond */
> > > damon_for_each_context(ctx, kdamond)
> > > prepare_accesses(ctx);
> > >
> > > sleep(sample_interval);
> > >
> > > /* check_accesses for all contexts in kdamond */
> > > damon_for_each_context(ctx, kdamond)
> > > check_accesses(ctx);
> > >
> > > ...
> > > }
> >
> > This is not what you do now, but on previous patch, right? Let's modify the
> > mesage appropriately.
>
> No, this is exactly what this patch is for, previous one only introduced
> 'struct kdamond' and made all interfaces (sysfs/dbgfs/core) to use it.
> I mean previous patch doesn't include support for multiple contexts,
> functionality was the same as before.
Thank you for clarifying. Indeed the first patch didn't update access check
part, but did update contexts initializations (kdamond_init_ctx()) and
termination (kdamond_finish_ctxs()) logics to support multiple contexts. Let's
do such things in one separate patch, as I suggested on the reply to the cover
letter.
>
> >
> > >
> > > Another point to note is watermarks. Previous versions
> > > checked watermarks on each iteration for current context
> > > and if matric's value wan't acceptable kdamond waited
> > > for watermark's sleep interval.
> >
> > Mention changes of versions on cover letter.
> >
> > >
> > > Now there's no need to wait for each context, we can
> > > just skip context if watermark's metric isn't ready,
> > > but if there's no contexts that can run we
> > > check for each context's watermark metric and
> > > sleep for the lowest interval of all contexts.
> > >
> > > Signed-off-by: Alex Rusuf <[email protected]>
> > > ---
> > > include/linux/damon.h | 11 +-
> > > include/trace/events/damon.h | 14 +-
> > > mm/damon/core-test.h | 2 +-
> > > mm/damon/core.c | 286 +++++++++++++++++------------
> > > mm/damon/dbgfs-test.h | 4 +-
> > > mm/damon/dbgfs.c | 342 +++++++++++++++++++++--------------
> > > mm/damon/modules-common.c | 1 -
> > > mm/damon/sysfs.c | 47 +++--
> > > 8 files changed, 431 insertions(+), 276 deletions(-)
> > >
> > > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > > index 7cb9979a0..2facf3a5f 100644
> > > --- a/include/linux/damon.h
> > > +++ b/include/linux/damon.h
> > > @@ -575,7 +575,6 @@ struct damon_attrs {
> > > * @lock: Kdamond's global lock, serializes accesses to any field.
> > > * @self: Kernel thread which is actually being executed.
> > > * @contexts: Head of contexts (&damon_ctx) list.
> > > - * @nr_ctxs: Number of contexts being monitored.
> > > *
> > > * Each DAMON's background daemon has this structure. Once
> > > * configured, daemon can be started by calling damon_start().
> > > @@ -589,7 +588,6 @@ struct kdamond {
> > > struct mutex lock;
> > > struct task_struct *self;
> > > struct list_head contexts;
> > > - size_t nr_ctxs;
> >
> > Why we add this on first patch, and then remove here? Let's not make
> > unnecessary temporal change. It only increase review time.
>
> This temporal change is needed only to not break DAMON functionality
> once first patch is applied (i.e. only 1st patch is applied). I mean
> to have constistancy between patches.
>
> I think, if I change the patch-history (the one you suggested in another
> reply) this could be avoided.
Yes, I think that would be helpful for reviewing :)
>
> >
> > >
> > > /* private: */
> > > /* for waiting until the execution of the kdamond_fn is started */
> > > @@ -634,7 +632,10 @@ struct damon_ctx {
> > > * update
> > > */
> > > unsigned long next_ops_update_sis;
> > > + /* upper limit for each monitoring region */
> > > unsigned long sz_limit;
> > > + /* marker to check if context is valid */
> > > + bool valid;
> >
> > What is the validity?
>
> This is a "flag" which indicates that the context is "valid" for kdamond
> to be able to call ->check_accesses() on it. Because we have many contexts
> we need a way to identify which context is valid while iterating over
> all of them (please, see kdamond_prepare_access_checks_ctx() function).
It's still not very clear to me. When it is "valid" or not for kdamond to be
able to call ->check_accesses()? I assume you mean it is valid if the
monitoring operations set's ->target_valid() returns zero?
The callback is not for preventing unnecessary ->check_accesses(), but for
knowing if continuing the monitoring makes sense or not. For example, the
callback of 'vaddr' operation set checks if the virtual address space still
exists (whether the target process is still running) Calling
->check_accesses() for ->target_valid() returned non-zero target is totally ok,
though it is meaningless. And therefore kdamond stops if it returns non-zero.
>
> Maybe name should be changed,
At least some more detailed comment would be appreciated, imo.
> but now I don't see a way how we could merge
> this into kdamond_valid_ctx() or so, because the main cause of this "valid"
> bit is that there's no more "valid" targets for this context, but also we could
> have ->after_sampling() returned something bad.
As mentioned above, calling ->check_accesses() or others towards invalidated
targets (e.g., terminated processes's virtual address spaces) should be ok, if
any of the targets are still valid. So I don't show special technical
difficulties here. Please let me know if I'm missing something.
>
> >
> > >
> > > /* public: */
> > > struct kdamond *kdamond;
> > > @@ -682,6 +683,12 @@ static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
> > > return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
> > > }
> > >
> > > +static inline bool damon_is_last_ctx(struct damon_ctx *ctx,
> > > + struct kdamond *kdamond)
> > > +{
> > > + return list_is_last(&ctx->list, &kdamond->contexts);
> > > +}
> > > +
> > > #define damon_for_each_region(r, t) \
> > > list_for_each_entry(r, &t->regions_list, list)
> > >
> > > diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> > > index 23200aabc..d5287566c 100644
> > > --- a/include/trace/events/damon.h
> > > +++ b/include/trace/events/damon.h
> >
> > Let's separate this change to another patch.
>
> Separating patches we hardly be able to reach at least build
> consistency between patches. Moreover DAMON won't be able
> to function corretly in between.
I agree that it's not very easy, indeed. But let's try. In terms of
functionality, we need to keep the old behavior that visible to users. For
example, this change tries to make the traceevent changed for the multi
contexts support. It is for making the behavior _better_, not for keeping old
behavior. Rather than that, this is introducing a new change to the tracepoint
output. Just make no change here. Users may get confused when they use
multiple contexts, but what they see is not changed.
Further, you can delay letting users (user-space) using the multiple contexts
(allowing >1 input to nr_contexts of DAMON sysfs interface) after making this
change in a separate patch.
>
> >
> > > @@ -50,12 +50,13 @@ TRACE_EVENT_CONDITION(damos_before_apply,
> > >
> > > TRACE_EVENT(damon_aggregated,
> > >
> > > - TP_PROTO(unsigned int target_id, struct damon_region *r,
> > > - unsigned int nr_regions),
> > > + TP_PROTO(unsigned int context_id, unsigned int target_id,
> > > + struct damon_region *r, unsigned int nr_regions),
> > >
> > > - TP_ARGS(target_id, r, nr_regions),
> > > + TP_ARGS(context_id, target_id, r, nr_regions),
> > >
> > > TP_STRUCT__entry(
> > > + __field(unsigned long, context_id)
> > > __field(unsigned long, target_id)
> > > __field(unsigned int, nr_regions)
> > > __field(unsigned long, start)
> > > @@ -65,6 +66,7 @@ TRACE_EVENT(damon_aggregated,
> > > ),
> > >
> > > TP_fast_assign(
> > > + __entry->context_id = context_id;
> > > __entry->target_id = target_id;
> > > __entry->nr_regions = nr_regions;
> > > __entry->start = r->ar.start;
> > > @@ -73,9 +75,9 @@ TRACE_EVENT(damon_aggregated,
> > > __entry->age = r->age;
> > > ),
> > >
> > > - TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
> > > - __entry->target_id, __entry->nr_regions,
> > > - __entry->start, __entry->end,
> > > + TP_printk("context_id=%lu target_id=%lu nr_regions=%u %lu-%lu: %u %u",
> > > + __entry->context_id, __entry->target_id,
> > > + __entry->nr_regions, __entry->start, __entry->end,
> > > __entry->nr_accesses, __entry->age)
> > > );
> > >
> > > diff --git a/mm/damon/core-test.h b/mm/damon/core-test.h
> > > index 0cee634f3..7962c9a0e 100644
> > > --- a/mm/damon/core-test.h
> > > +++ b/mm/damon/core-test.h
> > > @@ -99,7 +99,7 @@ static void damon_test_aggregate(struct kunit *test)
> > > }
> > > it++;
> > > }
> > > - kdamond_reset_aggregated(ctx);
> > > + kdamond_reset_aggregated(ctx, 0);
> > > it = 0;
> > > damon_for_each_target(t, ctx) {
> > > ir = 0;
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index cfc9c803d..ad73752af 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > > @@ -500,6 +500,8 @@ struct damon_ctx *damon_new_ctx(void)
> > > ctx->attrs.min_nr_regions = 10;
> > > ctx->attrs.max_nr_regions = 1000;
> > >
> > > + ctx->valid = true;
> > > +
> > > INIT_LIST_HEAD(&ctx->adaptive_targets);
> > > INIT_LIST_HEAD(&ctx->schemes);
> > > INIT_LIST_HEAD(&ctx->list);
> > > @@ -513,7 +515,7 @@ struct damon_ctx *damon_new_ctx(void)
> > > void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
> > > {
> > > list_add_tail(&ctx->list, &kdamond->contexts);
> > > - ++kdamond->nr_ctxs;
> > > + ctx->kdamond = kdamond;
> > > }
> > >
> > > struct kdamond *damon_new_kdamond(void)
> > > @@ -567,10 +569,8 @@ void damon_destroy_ctxs(struct kdamond *kdamond)
> > > {
> > > struct damon_ctx *c, *next;
> > >
> > > - damon_for_each_context_safe(c, next, kdamond) {
> > > + damon_for_each_context_safe(c, next, kdamond)
> > > damon_destroy_ctx(c);
> > > - --kdamond->nr_ctxs;
> > > - }
> > > }
> > >
> > > void damon_destroy_kdamond(struct kdamond *kdamond)
> > > @@ -735,6 +735,20 @@ bool damon_kdamond_running(struct kdamond *kdamond)
> > > return running;
> > > }
> > >
> > > +/**
> > > + * kdamond_nr_ctxs() - Return number of contexts for this kdamond.
> > > + */
> > > +static int kdamond_nr_ctxs(struct kdamond *kdamond)
> > > +{
> > > + struct list_head *pos;
> > > + int nr_ctxs = 0;
> > > +
> > > + list_for_each(pos, &kdamond->contexts)
> > > + ++nr_ctxs;
> > > +
> > > + return nr_ctxs;
> > > +}
> > > +
> > > /* Returns the size upper limit for each monitoring region */
> > > static unsigned long damon_region_sz_limit(struct damon_ctx *ctx)
> > > {
> > > @@ -793,11 +807,11 @@ static int __damon_start(struct kdamond *kdamond)
> > > * @exclusive: exclusiveness of this contexts group
> > > *
> > > * This function starts a group of monitoring threads for a group of monitoring
> > > - * contexts. One thread per each context is created and run in parallel. The
> > > - * caller should handle synchronization between the threads by itself. If
> > > - * @exclusive is true and a group of threads that created by other
> > > + * contexts. If @exclusive is true and a group of contexts that created by other
> > > * 'damon_start()' call is currently running, this function does nothing but
> > > - * returns -EBUSY.
> > > + * returns -EBUSY, if @exclusive is true and a given kdamond wants to run
> > > + * several contexts, then this function returns -EINVAL. kdamond can run
> > > + * exclusively only one context.
> > > *
> > > * Return: 0 on success, negative error code otherwise.
> > > */
> > > @@ -806,10 +820,6 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
> > > int err = 0;
> > >
> > > BUG_ON(!kdamond);
> > > - BUG_ON(!kdamond->nr_ctxs);
> > > -
> > > - if (kdamond->nr_ctxs != 1)
> > > - return -EINVAL;
> > >
> > > mutex_lock(&damon_lock);
> > > if ((exclusive && nr_running_kdamonds) ||
> > > @@ -818,6 +828,11 @@ int damon_start(struct kdamond *kdamond, bool exclusive)
> > > return -EBUSY;
> > > }
> > >
> > > + if (exclusive && kdamond_nr_ctxs(kdamond) > 1) {
> > > + mutex_unlock(&damon_lock);
> > > + return -EINVAL;
> > > + }
> > > +
> > > err = __damon_start(kdamond);
> > > if (err)
> > > return err;
> > > @@ -857,7 +872,7 @@ int damon_stop(struct kdamond *kdamond)
> > > /*
> > > * Reset the aggregated monitoring results ('nr_accesses' of each region).
> > > */
> > > -static void kdamond_reset_aggregated(struct damon_ctx *c)
> > > +static void kdamond_reset_aggregated(struct damon_ctx *c, unsigned int ci)
> > > {
> > > struct damon_target *t;
> > > unsigned int ti = 0; /* target's index */
> > > @@ -866,7 +881,7 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
> > > struct damon_region *r;
> > >
> > > damon_for_each_region(r, t) {
> > > - trace_damon_aggregated(ti, r, damon_nr_regions(t));
> > > + trace_damon_aggregated(ci, ti, r, damon_nr_regions(t));
> >
> > Separate traceevent change into another patch.
> >
> > > r->last_nr_accesses = r->nr_accesses;
> > > r->nr_accesses = 0;
> > > }
> > > @@ -1033,21 +1048,15 @@ static bool damos_filter_out(struct damon_ctx *ctx, struct damon_target *t,
> > > return false;
> > > }
> > >
> > > -static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> > > - struct damon_region *r, struct damos *s)
> > > +static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c,
> > > + struct damon_target *t, struct damon_region *r,
> > > + struct damos *s)
> >
> > Unnecesary change.
>
> What do you mean? Why is this unnecessary? Now DAMON iterates over
> contexts and calls kdamond_apply_schemes(ctx), so how can we know
> which context we print trace for? Sorry, maybe I misunderstood
> how DAMON does it, but I'm a bit confused.
I believe the above comment to tracevent change explains this.
>
> Or maybe you mean indentation? The actual change is adding "cidx":
>
> -static void damos_apply_scheme(struct damon_ctx *c, ...
> +static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c,
>
> >
> > As also mentioned on the reply to the first patch, I think this patch can
> > significantly changed if you agree to my opinion about the flow of patches that
> > I mentioned on the reply to the cover letter. Hence, stopping review from here
> > for now. Please let me know if you have a different opinion.
>
> I see. Anyway, thank you for your comments, I'll try my best to improve the patch.
No problem, appreciate your patience and great works on this patchset!
Thanks,
SJ
[...]
On Sun, 2 Jun 2024 18:31:02 +0300 Alex Rusuf <[email protected]> wrote:
> Hi SJ,
>
> > Hello Alex,
> >
> > On Fri, 31 May 2024 15:23:18 +0300 Alex Rusuf <[email protected]> wrote:
> >
> > > Currently kdamond uses only one context per kthread
> > > and most of its time it sleeps, so utilizing several
> > > contexts can scale kdamond and allow it to use
> > > another set of operations.
> > >
> > > This patch-set implements support for multiple contexts
> > > per kdamond.
> > >
> > [...]
> > >
> > > ---
> > > Changes from v1 (https://lore.kernel.org/damon/[email protected]/)
> > > - Compatibility for DebugFS interface is kept
> > > - Kunit tests build/execution issues are fixed
> > > - Patches from v1 are sqaushed, so that consistency between patches is
> > > kept
> >
> > My request was to avoid unnecessary temporal changes that will be removed in
> > next patches. Some of those are well removed in this version, but I still show
> > some. E.g., nr_contexts field. Also, this resulted in two big patches.
>
> This makes sense and I actually wanted that as well, so I tried to separate
> them in previous version, looks like I misunderstood your request.
No problem, I think I should also be more clear about the point. Happy to have
a chance to develop my humble communication skill with the conversaions with
you.
>
> Anyway, don't you mind if lru_sort/traceevents/etc. will not function
> correctly without applying the whole patch-set? I mean if we use the
> approach below, once core.c is modified at least lru_sort and reclaim
> will not work correctly, they even will not be built.
I mind those. Everything should work without regression in the middle of the
patchset. Nonetheless, we should avoid only regression. We don't need to make
everything perfect. Let's minimize changes to the other modules in the way.
I believe below suggested patches flow and my second reply to the second patch
of this patchset can clarify the point. Please let me know if not.
>
> >
> > I'd also appreciate if you can separate changes into smaller ones of logical
> > single change. For example, changes for lru_sort.c, reclaim.c, and sysfs.c on
> > first patch could be much smaller in my opinion. Traceevent change can also be
> > separated from patch 2. Some of multi-context support seems mixed in patch 1.
> >
> > I'd suggest below patches flow.
> >
> > Patch 1: Introduce new struct and control functions for the struct. Don't
> > really use the struct and the functions.
> >
> > Patch 2: Modify core.c to use the struct and implement multiple contexts
> > support. Minimize changes to core.c users. Just keep those work as before.
> > Don't implement multi contexts support on sysfs.c or trace events at this
> > point.
> >
> > Patch 3: Update sysfs.c to support the multiple contexts.
> >
> > Patch 4: Update trace events to better support it.
As I mentioned on my second reply to the second patch, you could swich patches
3 and 4 if you want trace events to work perfect from the beginning of
user-visible multi contexts support.
Thanks,
SJ
[...]
Hi SJ,
> Hi Alex,
>
> On Fri, 31 May 2024 15:23:19 +0300 Alex Rusuf <[email protected]> wrote:
>
> > In current implementation kdamond tracks only 1
> > context, that is kdamond _is_ damon_ctx what makes
> > it very difficult to implement multiple contexts.
> >
> > This patch adds another level of abstraction, that is
> > 'struct kdamond' - structure which represents kdamond itself.
> > It holds references to all contexts organized in list.
> >
> > Few fields like ->kdamond_started and ->kdamond_lock
> > (just ->lock for 'struct kdamond') also has been moved
> > to 'struct kdamond', because they have nothing to do
> > with the context itself, but with the whole kdamond
> > daemon.
> >
> > Signed-off-by: Alex Rusuf <[email protected]>
> > ---
> > include/linux/damon.h | 73 ++++++++---
> > mm/damon/core.c | 249 ++++++++++++++++++++++-------------
> > mm/damon/lru_sort.c | 31 +++--
> > mm/damon/modules-common.c | 36 +++--
> > mm/damon/modules-common.h | 3 +-
> > mm/damon/reclaim.c | 30 +++--
> > mm/damon/sysfs.c | 268 ++++++++++++++++++++++++--------------
> > 7 files changed, 463 insertions(+), 227 deletions(-)
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 886d07294..7cb9979a0 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -568,29 +568,49 @@ struct damon_attrs {
> > unsigned long max_nr_regions;
> > };
> >
> > +/**
> > + * struct kdamond - Represents a background daemon that is responsible
> > + * for executing each context.
> > + *
> > + * @lock: Kdamond's global lock, serializes accesses to any field.
> > + * @self: Kernel thread which is actually being executed.
> > + * @contexts: Head of contexts (&damon_ctx) list.
> > + * @nr_ctxs: Number of contexts being monitored.
> > + *
> > + * Each DAMON's background daemon has this structure. Once
> > + * configured, daemon can be started by calling damon_start().
> > + *
> > + * Monitoring can be explicitly stopped by calling damon_stop(). Once
> > + * daemon is terminated @self is set to NULL, so users can know if
> > + * monitoring is stopped by reading @self pointer. Access to @self
> > + * must also be protected by @lock.
> > + */
> > +struct kdamond {
> > + struct mutex lock;
> > + struct task_struct *self;
>
> I'm not sure if 'self' is a good name. I'd prefer more clear name such as
> 'task', but this is not a strong opinion.
>
> > + struct list_head contexts;
> > + size_t nr_ctxs;
> > +
> > +/* private: */
> > + /* for waiting until the execution of the kdamond_fn is started */
> > + struct completion kdamond_started;
> > +};
> > +
> > /**
> > * struct damon_ctx - Represents a context for each monitoring. This is the
> > * main interface that allows users to set the attributes and get the results
> > * of the monitoring.
> > *
> > * @attrs: Monitoring attributes for accuracy/overhead control.
> > - * @kdamond: Kernel thread who does the monitoring.
> > - * @kdamond_lock: Mutex for the synchronizations with @kdamond.
> > + * @kdamond: Back reference to daemon who is the owner of this context.
>
> Why do we need this?
This is mostly for DebugFS, but I guess this can be removed. This has been
brought to keep compatibility with previous interfaces while introducing
kdamonds abstraction.
>
> > + * @list: List head of siblings.
> > *
> > * For each monitoring context, one kernel thread for the monitoring is
> > * created. The pointer to the thread is stored in @kdamond.
> > *
> > * Once started, the monitoring thread runs until explicitly required to be
> > * terminated or every monitoring target is invalid. The validity of the
> > - * targets is checked via the &damon_operations.target_valid of @ops. The
> > - * termination can also be explicitly requested by calling damon_stop().
> > - * The thread sets @kdamond to NULL when it terminates. Therefore, users can
> > - * know whether the monitoring is ongoing or terminated by reading @kdamond.
> > - * Reads and writes to @kdamond from outside of the monitoring thread must
> > - * be protected by @kdamond_lock.
> > - *
> > - * Note that the monitoring thread protects only @kdamond via @kdamond_lock.
> > - * Accesses to other fields must be protected by themselves.
> > + * targets is checked via the &damon_operations.target_valid of @ops.
> > *
> > * @ops: Set of monitoring operations for given use cases.
> > * @callback: Set of callbacks for monitoring events notifications.
> > @@ -614,12 +634,11 @@ struct damon_ctx {
> > * update
> > */
> > unsigned long next_ops_update_sis;
> > - /* for waiting until the execution of the kdamond_fn is started */
> > - struct completion kdamond_started;
> > + unsigned long sz_limit;
>
> What's this?
I forgot to add comments there, this field is just saved like "next_ops_update_sis",
which is used in kdamond main loop.
>
> >
> > /* public: */
> > - struct task_struct *kdamond;
> > - struct mutex kdamond_lock;
> > + struct kdamond *kdamond;
> > + struct list_head list;
> >
> > struct damon_operations ops;
> > struct damon_callback callback;
> > @@ -653,6 +672,15 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
> > return r->ar.end - r->ar.start;
> > }
> >
> > +static inline struct damon_target *damon_first_target(struct damon_ctx *ctx)
> > +{
> > + return list_first_entry(&ctx->adaptive_targets, struct damon_target, list);
> > +}
> > +
> > +static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
> > +{
> > + return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
> > +}
> >
> > #define damon_for_each_region(r, t) \
> > list_for_each_entry(r, &t->regions_list, list)
> > @@ -675,6 +703,12 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
> > #define damon_for_each_scheme_safe(s, next, ctx) \
> > list_for_each_entry_safe(s, next, &(ctx)->schemes, list)
> >
> > +#define damon_for_each_context(c, kdamond) \
> > + list_for_each_entry(c, &(kdamond)->contexts, list)
> > +
> > +#define damon_for_each_context_safe(c, next, kdamond) \
> > + list_for_each_entry_safe(c, next, &(kdamond)->contexts, list)
> > +
> > #define damos_for_each_quota_goal(goal, quota) \
> > list_for_each_entry(goal, "a->goals, list)
> >
> > @@ -736,7 +770,12 @@ void damon_destroy_target(struct damon_target *t);
> > unsigned int damon_nr_regions(struct damon_target *t);
> >
> > struct damon_ctx *damon_new_ctx(void);
> > +void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx);
> > +struct kdamond *damon_new_kdamond(void);
> > void damon_destroy_ctx(struct damon_ctx *ctx);
> > +void damon_destroy_ctxs(struct kdamond *kdamond);
>
> Why do we need this? Wouldn't damon_destroy_kdamond() do that?
You're right, looks like only damon_destroy_kdamond() uses this
function, will be embeded, thank you!
>
> > +void damon_destroy_kdamond(struct kdamond *kdamond);
> > +bool damon_kdamond_running(struct kdamond *kdamond);
>
> Let's put kdamond control functions outside of functions for damon_ctx control,
> like we do for damon_target and damon_ctx.
Yeah, I didn't notice this, thank you!
>
> > int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs);
> > void damon_set_schemes(struct damon_ctx *ctx,
> > struct damos **schemes, ssize_t nr_schemes);
> > @@ -758,8 +797,8 @@ static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs
> > }
> >
> >
> > -int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive);
> > -int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> > +int damon_start(struct kdamond *kdamond, bool exclusive);
> > +int damon_stop(struct kdamond *kdamond);
> >
> > int damon_set_region_biggest_system_ram_default(struct damon_target *t,
> > unsigned long *start, unsigned long *end);
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 6d503c1c1..cfc9c803d 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -24,7 +24,7 @@
> > #endif
> >
> > static DEFINE_MUTEX(damon_lock);
> > -static int nr_running_ctxs;
> > +static int nr_running_kdamonds;
> > static bool running_exclusive_ctxs;
> >
> > static DEFINE_MUTEX(damon_ops_lock);
> > @@ -488,8 +488,6 @@ struct damon_ctx *damon_new_ctx(void)
> > if (!ctx)
> > return NULL;
> >
> > - init_completion(&ctx->kdamond_started);
> > -
> > ctx->attrs.sample_interval = 5 * 1000;
> > ctx->attrs.aggr_interval = 100 * 1000;
> > ctx->attrs.ops_update_interval = 60 * 1000 * 1000;
> > @@ -499,17 +497,41 @@ struct damon_ctx *damon_new_ctx(void)
> > ctx->next_aggregation_sis = 0;
> > ctx->next_ops_update_sis = 0;
> >
> > - mutex_init(&ctx->kdamond_lock);
> > -
> > ctx->attrs.min_nr_regions = 10;
> > ctx->attrs.max_nr_regions = 1000;
> >
> > INIT_LIST_HEAD(&ctx->adaptive_targets);
> > INIT_LIST_HEAD(&ctx->schemes);
> > + INIT_LIST_HEAD(&ctx->list);
> >
> > return ctx;
> > }
> >
> > +/**
> > + * Adds newly allocated and configured @ctx to @kdamond.
>
> There's no guarantee if @ctx is newly allocated and configured. We can simply
> say "Adds a DAMON context to a given DAMON thread".
>
> And this may cause kerneldoc compilation warning. I think the format should be
> fixed, and parameters should be commented.
True, I'll fix that, thank you!
>
> > + */
> > +void damon_add_ctx(struct kdamond *kdamond, struct damon_ctx *ctx)
> > +{
> > + list_add_tail(&ctx->list, &kdamond->contexts);
> > + ++kdamond->nr_ctxs;
> > +}
> > +
> > +struct kdamond *damon_new_kdamond(void)
> > +{
> > + struct kdamond *kdamond;
> > +
> > + kdamond = kzalloc(sizeof(*kdamond), GFP_KERNEL);
> > + if (!kdamond)
> > + return NULL;
> > +
> > + init_completion(&kdamond->kdamond_started);
> > + mutex_init(&kdamond->lock);
> > +
> > + INIT_LIST_HEAD(&kdamond->contexts);
>
> Among the four fields of the struct, three are manually set here, and the final
> one is set as zero by 'kzalloc()'. I'd prefer using 'kmalloc()' and manually
> set all fields.
No problem, I'll change this, thank you!
>
> > +
> > + return kdamond;
> > +}
> > +
> > static void damon_destroy_targets(struct damon_ctx *ctx)
> > {
> > struct damon_target *t, *next_t;
> > @@ -523,6 +545,11 @@ static void damon_destroy_targets(struct damon_ctx *ctx)
> > damon_destroy_target(t);
> > }
> >
> > +static inline void damon_del_ctx(struct damon_ctx *ctx)
> > +{
> > + list_del(&ctx->list);
> > +}
> > +
> > void damon_destroy_ctx(struct damon_ctx *ctx)
> > {
> > struct damos *s, *next_s;
> > @@ -532,9 +559,27 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
> > damon_for_each_scheme_safe(s, next_s, ctx)
> > damon_destroy_scheme(s);
> >
> > + damon_del_ctx(ctx);
> > kfree(ctx);
> > }
> >
> > +void damon_destroy_ctxs(struct kdamond *kdamond)
> > +{
> > + struct damon_ctx *c, *next;
> > +
> > + damon_for_each_context_safe(c, next, kdamond) {
> > + damon_destroy_ctx(c);
> > + --kdamond->nr_ctxs;
> > + }
> > +}
>
> Is there explicit user of this function? If not, I'd prefer making this
> 'static', or simply embed in damon_destroy_kdamond() below.
As I mentioned above there's no explicit users, so I'll embed it
into damon_destroy_kdamond().
>
> > +
> > +void damon_destroy_kdamond(struct kdamond *kdamond)
> > +{
> > + damon_destroy_ctxs(kdamond);
> > + mutex_destroy(&kdamond->lock);
> > + kfree(kdamond);
> > +}
> > +
> > static unsigned int damon_age_for_new_attrs(unsigned int age,
> > struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
> > {
> > @@ -667,13 +712,27 @@ void damon_set_schemes(struct damon_ctx *ctx, struct damos **schemes,
> > */
> > int damon_nr_running_ctxs(void)
> > {
> > - int nr_ctxs;
> > + int nr_kdamonds;
> >
> > mutex_lock(&damon_lock);
> > - nr_ctxs = nr_running_ctxs;
> > + nr_kdamonds = nr_running_kdamonds;
> > mutex_unlock(&damon_lock);
> >
> > - return nr_ctxs;
> > + return nr_kdamonds;
>
> Shouldn't we change the name of this function, too?
Yeah, this makes sense, thank you!
>
> > +}
> > +
> > +/**
> > + * damon_kdamond_running() - Return true if kdamond is running
> > + * false otherwise.
> > + */
> > +bool damon_kdamond_running(struct kdamond *kdamond)
> > +{
> > + bool running;
> > +
> > + mutex_lock(&kdamond->lock);
> > + running = kdamond->self != NULL;
> > + mutex_unlock(&kdamond->lock);
> > + return running;
> > }
>
> Seems this function is used by only DAMON sysfs interface. Don't implement
> unnecessary public function. Implement it in mm/damon/sysfs.c please.
DebugFS interface also uses this.
>
> >
> > /* Returns the size upper limit for each monitoring region */
> > @@ -700,38 +759,37 @@ static int kdamond_fn(void *data);
> >
> > /*
> > * __damon_start() - Starts monitoring with given context.
>
> s/context/kdamond/? Maybe we can simply say "starts given kdamond".
You're right, thank you!
>
> > - * @ctx: monitoring context
> > + * @kdamond: daemon to be started
> > *
> > * 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)
> > +static int __damon_start(struct kdamond *kdamond)
> > {
> > int err = -EBUSY;
> >
> > - mutex_lock(&ctx->kdamond_lock);
> > - if (!ctx->kdamond) {
> > + mutex_lock(&kdamond->lock);
> > + if (!kdamond->self) {
> > err = 0;
> > - reinit_completion(&ctx->kdamond_started);
> > - ctx->kdamond = kthread_run(kdamond_fn, ctx, "kdamond.%d",
> > - nr_running_ctxs);
> > - if (IS_ERR(ctx->kdamond)) {
> > - err = PTR_ERR(ctx->kdamond);
> > - ctx->kdamond = NULL;
> > + reinit_completion(&kdamond->kdamond_started);
> > + kdamond->self = kthread_run(kdamond_fn, kdamond, "kdamond.%d",
> > + nr_running_kdamonds);
> > + if (IS_ERR(kdamond->self)) {
> > + err = PTR_ERR(kdamond->self);
> > + kdamond->self = NULL;
> > } else {
> > - wait_for_completion(&ctx->kdamond_started);
> > + wait_for_completion(&kdamond->kdamond_started);
> > }
> > }
> > - mutex_unlock(&ctx->kdamond_lock);
> > + mutex_unlock(&kdamond->lock);
> >
> > return err;
> > }
> >
> > /**
> > * damon_start() - Starts the monitorings for a given group of contexts.
> > - * @ctxs: an array of the pointers for contexts to start monitoring
> > - * @nr_ctxs: size of @ctxs
> > + * @kdamond: a daemon that contains list of monitoring contexts
> > * @exclusive: exclusiveness of this contexts group
> > *
> > * This function starts a group of monitoring threads for a group of monitoring
> > @@ -743,74 +801,59 @@ static int __damon_start(struct damon_ctx *ctx)
> > *
> > * Return: 0 on success, negative error code otherwise.
> > */
> > -int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
> > +int damon_start(struct kdamond *kdamond, bool exclusive)
> > {
> > - int i;
> > int err = 0;
> >
> > + BUG_ON(!kdamond);
> > + BUG_ON(!kdamond->nr_ctxs);
>
> checkpatch complains as below.
>
> WARNING: Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants
> #432: FILE: mm/damon/core.c:809:
> + BUG_ON(!kdamond->nr_ctxs);
>
> I don't think we really need above BUG_ON() checks.
Actually we can just return an error here. I guess I did this
for better debugging, so it can be removed.
>
> > +
> > + if (kdamond->nr_ctxs != 1)
> > + return -EINVAL;
>
> Why do we need this? To avoid DAMON sysfs interface change? I think
> mm/damon/sysfs.c does this check on its own, so this may not needed?
You're right, I didn't notice this, thank you!
>
> > +
> > mutex_lock(&damon_lock);
> > - if ((exclusive && nr_running_ctxs) ||
> > + if ((exclusive && nr_running_kdamonds) ||
> > (!exclusive && running_exclusive_ctxs)) {
> > mutex_unlock(&damon_lock);
> > return -EBUSY;
> > }
> >
> > - for (i = 0; i < nr_ctxs; i++) {
> > - err = __damon_start(ctxs[i]);
> > - if (err)
> > - break;
> > - nr_running_ctxs++;
> > - }
> > - if (exclusive && nr_running_ctxs)
> > + err = __damon_start(kdamond);
> > + if (err)
> > + return err;
> > + nr_running_kdamonds++;
> > +
> > + if (exclusive && nr_running_kdamonds)
> > running_exclusive_ctxs = true;
> > mutex_unlock(&damon_lock);
> >
> > return err;
> > }
> >
> > -/*
> > - * __damon_stop() - Stops monitoring of a given context.
> > - * @ctx: monitoring context
> > +/**
> > + * damon_stop() - Stops the monitorings for a given group of contexts.
>
> Let's simply say "Stops a given DAMON thread"
>
> > + * @kdamond: a daemon (that contains list of monitoring contexts)
> > + * to be stopped.
> > *
> > * Return: 0 on success, negative error code otherwise.
> > */
> > -static int __damon_stop(struct damon_ctx *ctx)
> > +int damon_stop(struct kdamond *kdamond)
> > {
> > struct task_struct *tsk;
> >
> > - mutex_lock(&ctx->kdamond_lock);
> > - tsk = ctx->kdamond;
> > + mutex_lock(&kdamond->lock);
> > + tsk = kdamond->self;
> > if (tsk) {
> > get_task_struct(tsk);
> > - mutex_unlock(&ctx->kdamond_lock);
> > + mutex_unlock(&kdamond->lock);
> > kthread_stop_put(tsk);
> > return 0;
> > }
> > - mutex_unlock(&ctx->kdamond_lock);
> > + mutex_unlock(&kdamond->lock);
> >
> > return -EPERM;
> > }
> >
> > -/**
> > - * damon_stop() - Stops the monitorings for a given group of contexts.
> > - * @ctxs: an array of the pointers for contexts to stop monitoring
> > - * @nr_ctxs: size of @ctxs
> > - *
> > - * Return: 0 on success, negative error code otherwise.
> > - */
> > -int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
> > -{
> > - int i, err = 0;
> > -
> > - for (i = 0; i < nr_ctxs; i++) {
> > - /* nr_running_ctxs is decremented in kdamond_fn */
> > - err = __damon_stop(ctxs[i]);
> > - if (err)
> > - break;
> > - }
> > - return err;
> > -}
> > -
> > /*
> > * Reset the aggregated monitoring results ('nr_accesses' of each region).
> > */
> > @@ -1582,29 +1625,68 @@ static void kdamond_init_intervals_sis(struct damon_ctx *ctx)
> > }
> > }
> >
> > +static bool kdamond_init_ctx(struct damon_ctx *ctx)
> > +{
> > + if (ctx->ops.init)
> > + ctx->ops.init(ctx);
> > + if (ctx->callback.before_start && ctx->callback.before_start(ctx))
> > + return false;
> > +
> > + kdamond_init_intervals_sis(ctx);
> > + ctx->sz_limit = damon_region_sz_limit(ctx);
> > +
> > + return true;
> > +}
>
> I think 'int' return type for error code is better. We can just return
> before_start() return value in case of error, otherwise zero.
That's true, before_start() also returns 'int', so this looks better,
thanks!
>
> > +
> > +static bool kdamond_init_ctxs(struct kdamond *kdamond)
> > +{
> > + struct damon_ctx *c;
> > +
> > + damon_for_each_context(c, kdamond)
> > + if (!kdamond_init_ctx(c))
> > + return false;
> > + return true;
> > +}
> > +
> > +static void kdamond_finish_ctx(struct damon_ctx *ctx)
> > +{
> > + struct damon_target *t;
> > + struct damon_region *r, *next;
> > +
> > + damon_for_each_target(t, ctx) {
> > + damon_for_each_region_safe(r, next, t)
> > + damon_destroy_region(r, t);
> > + }
> > +
> > + if (ctx->callback.before_terminate)
> > + ctx->callback.before_terminate(ctx);
> > + if (ctx->ops.cleanup)
> > + ctx->ops.cleanup(ctx);
> > +}
> > +
> > +static void kdamond_finish_ctxs(struct kdamond *kdamond)
> > +{
> > + struct damon_ctx *c;
> > +
> > + damon_for_each_context(c, kdamond)
> > + kdamond_finish_ctx(c);
> > +}
> > +
> > /*
> > * The monitoring daemon that runs as a kernel thread
> > */
> > static int kdamond_fn(void *data)
> > {
> > - struct damon_ctx *ctx = data;
> > - struct damon_target *t;
> > - struct damon_region *r, *next;
> > + struct kdamond *kdamond = data;
> > + struct damon_ctx *ctx = damon_first_ctx(kdamond);
> > unsigned int max_nr_accesses = 0;
> > - unsigned long sz_limit = 0;
> >
> > pr_debug("kdamond (%d) starts\n", current->pid);
> >
> > - complete(&ctx->kdamond_started);
> > - kdamond_init_intervals_sis(ctx);
> > -
> > - if (ctx->ops.init)
> > - ctx->ops.init(ctx);
> > - if (ctx->callback.before_start && ctx->callback.before_start(ctx))
> > + complete(&kdamond->kdamond_started);
> > + if (!kdamond_init_ctxs(kdamond))
> > goto done;
> >
> > - sz_limit = damon_region_sz_limit(ctx);
> > -
> > while (!kdamond_need_stop(ctx)) {
> > /*
> > * ctx->attrs and ctx->next_{aggregation,ops_update}_sis could
> > @@ -1616,6 +1698,7 @@ static int kdamond_fn(void *data)
> > unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
> > unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
> > unsigned long sample_interval = ctx->attrs.sample_interval;
> > + unsigned long sz_limit = ctx->sz_limit;
> >
> > if (kdamond_wait_activation(ctx))
> > break;
> > @@ -1666,28 +1749,20 @@ static int kdamond_fn(void *data)
> > sample_interval;
> > if (ctx->ops.update)
> > ctx->ops.update(ctx);
> > - sz_limit = damon_region_sz_limit(ctx);
> > + ctx->sz_limit = damon_region_sz_limit(ctx);
> > }
> > }
> > done:
> > - damon_for_each_target(t, ctx) {
> > - damon_for_each_region_safe(r, next, t)
> > - damon_destroy_region(r, t);
> > - }
> > -
> > - if (ctx->callback.before_terminate)
> > - ctx->callback.before_terminate(ctx);
> > - if (ctx->ops.cleanup)
> > - ctx->ops.cleanup(ctx);
> > + kdamond_finish_ctxs(kdamond);
> >
> > pr_debug("kdamond (%d) finishes\n", current->pid);
> > - mutex_lock(&ctx->kdamond_lock);
> > - ctx->kdamond = NULL;
> > - mutex_unlock(&ctx->kdamond_lock);
> > + mutex_lock(&kdamond->lock);
> > + kdamond->self = NULL;
> > + mutex_unlock(&kdamond->lock);
> >
> > mutex_lock(&damon_lock);
> > - nr_running_ctxs--;
> > - if (!nr_running_ctxs && running_exclusive_ctxs)
> > + nr_running_kdamonds--;
> > + if (!nr_running_kdamonds && running_exclusive_ctxs)
> > running_exclusive_ctxs = false;
> > mutex_unlock(&damon_lock);
> >
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 3de2916a6..76c20098a 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> > @@ -142,8 +142,18 @@ static struct damos_access_pattern damon_lru_sort_stub_pattern = {
> > .max_age_region = UINT_MAX,
> > };
> >
> > -static struct damon_ctx *ctx;
> > -static struct damon_target *target;
> > +static struct kdamond *kdamond;
> > +
> > +static inline struct damon_ctx *damon_lru_sort_ctx(void)
> > +{
> > + return damon_first_ctx(kdamond);
> > +}
> > +
> > +static inline struct damon_target *damon_lru_sort_target(void)
> > +{
> > + return damon_first_target(
> > + damon_lru_sort_ctx());
> > +}
>
> Do we really need above two functions? Can't we simply add 'kdamond' global
> variable but still uses 'ctx' and 'target' as before, except interface changed
> function calls?
We can, I just tried to use 'readable' interface, I thought
that using raw pointers from kdamond will be less readable.
>
> >
> > static struct damos *damon_lru_sort_new_scheme(
> > struct damos_access_pattern *pattern, enum damos_action action)
> > @@ -201,6 +211,7 @@ static int damon_lru_sort_apply_parameters(void)
> > struct damos *scheme, *hot_scheme, *cold_scheme;
> > struct damos *old_hot_scheme = NULL, *old_cold_scheme = NULL;
> > unsigned int hot_thres, cold_thres;
> > + struct damon_ctx *ctx = damon_lru_sort_ctx();
> > int err = 0;
> >
> > err = damon_set_attrs(ctx, &damon_lru_sort_mon_attrs);
> > @@ -237,7 +248,8 @@ static int damon_lru_sort_apply_parameters(void)
> > damon_set_schemes(ctx, &hot_scheme, 1);
> > damon_add_scheme(ctx, cold_scheme);
> >
> > - return damon_set_region_biggest_system_ram_default(target,
> > + return damon_set_region_biggest_system_ram_default(
> > + damon_lru_sort_target(),
> > &monitor_region_start,
> > &monitor_region_end);
> > }
> > @@ -247,7 +259,7 @@ static int damon_lru_sort_turn(bool on)
> > int err;
> >
> > if (!on) {
> > - err = damon_stop(&ctx, 1);
> > + err = damon_stop(kdamond);
> > if (!err)
> > kdamond_pid = -1;
> > return err;
> > @@ -257,10 +269,11 @@ static int damon_lru_sort_turn(bool on)
> > if (err)
> > return err;
> >
> > - err = damon_start(&ctx, 1, true);
> > + err = damon_start(kdamond, true);
> > if (err)
> > return err;
> > - kdamond_pid = ctx->kdamond->pid;
> > +
> > + kdamond_pid = kdamond->self->pid;
> > return 0;
> > }
> >
> > @@ -279,7 +292,7 @@ static int damon_lru_sort_enabled_store(const char *val,
> > return 0;
> >
> > /* Called before init function. The function will handle this. */
> > - if (!ctx)
> > + if (!kdamond)
> > goto set_param_out;
> >
> > err = damon_lru_sort_turn(enable);
> > @@ -334,11 +347,13 @@ static int damon_lru_sort_after_wmarks_check(struct damon_ctx *c)
> >
> > static int __init damon_lru_sort_init(void)
> > {
> > - int err = damon_modules_new_paddr_ctx_target(&ctx, &target);
> > + struct damon_ctx *ctx;
> > + int err = damon_modules_new_paddr_kdamond(&kdamond);
>
> I think we can allocate kdamond struct and set ctx/target here, and remove
> unnecessary functions and changes.
I see, will change that, thank you!
>
> >
> > if (err)
> > return err;
> >
> > + ctx = damon_lru_sort_ctx();
> > ctx->callback.after_wmarks_check = damon_lru_sort_after_wmarks_check;
> > ctx->callback.after_aggregation = damon_lru_sort_after_aggregation;
> >
> > diff --git a/mm/damon/modules-common.c b/mm/damon/modules-common.c
> > index 7cf96574c..436bb7948 100644
> > --- a/mm/damon/modules-common.c
> > +++ b/mm/damon/modules-common.c
> > @@ -9,13 +9,7 @@
> >
> > #include "modules-common.h"
> >
> > -/*
> > - * Allocate, set, and return a DAMON context for the physical address space.
> > - * @ctxp: Pointer to save the point to the newly created context
> > - * @targetp: Pointer to save the point to the newly created target
> > - */
> > -int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
> > - struct damon_target **targetp)
> > +static int __damon_modules_new_paddr_kdamond(struct kdamond *kdamond)
> > {
> > struct damon_ctx *ctx;
> > struct damon_target *target;
> > @@ -34,9 +28,33 @@ int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
> > damon_destroy_ctx(ctx);
> > return -ENOMEM;
> > }
> > +
> > damon_add_target(ctx, target);
> > + damon_add_ctx(kdamond, ctx);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Allocate, set, and return a DAMON daemon for the physical address space.
> > + * @kdamondp: Pointer to save the point to the newly created kdamond
> > + */
> > +int damon_modules_new_paddr_kdamond(struct kdamond **kdamondp)
> > +{
> > + int err;
> > + struct kdamond *kdamond;
> > +
> > + kdamond = damon_new_kdamond();
> > + if (!kdamond)
> > + return -ENOMEM;
> > +
> > + err = __damon_modules_new_paddr_kdamond(kdamond);
> > + if (err) {
> > + damon_destroy_kdamond(kdamond);
> > + return err;
> > + }
> > + kdamond->nr_ctxs = 1;
> >
> > - *ctxp = ctx;
> > - *targetp = target;
> > + *kdamondp = kdamond;
> > return 0;
> > }
> > diff --git a/mm/damon/modules-common.h b/mm/damon/modules-common.h
> > index f49cdb417..5fc5b8ae3 100644
> > --- a/mm/damon/modules-common.h
> > +++ b/mm/damon/modules-common.h
> > @@ -45,5 +45,4 @@
> > module_param_named(nr_##qt_exceed_name, stat.qt_exceeds, ulong, \
> > 0400);
> >
> > -int damon_modules_new_paddr_ctx_target(struct damon_ctx **ctxp,
> > - struct damon_target **targetp);
> > +int damon_modules_new_paddr_kdamond(struct kdamond **kdamondp);
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 9bd341d62..f6540ef1a 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> > @@ -150,8 +150,18 @@ static struct damos_stat damon_reclaim_stat;
> > DEFINE_DAMON_MODULES_DAMOS_STATS_PARAMS(damon_reclaim_stat,
> > reclaim_tried_regions, reclaimed_regions, quota_exceeds);
> >
> > -static struct damon_ctx *ctx;
> > -static struct damon_target *target;
> > +static struct kdamond *kdamond;
> > +
> > +static inline struct damon_ctx *damon_reclaim_ctx(void)
> > +{
> > + return damon_first_ctx(kdamond);
> > +}
> > +
> > +static inline struct damon_target *damon_reclaim_target(void)
> > +{
> > + return damon_first_target(
> > + damon_reclaim_ctx());
> > +}
>
> Ditto.
>
> >
> > static struct damos *damon_reclaim_new_scheme(void)
> > {
> > @@ -197,6 +207,7 @@ static int damon_reclaim_apply_parameters(void)
> > struct damos *scheme, *old_scheme;
> > struct damos_quota_goal *goal;
> > struct damos_filter *filter;
> > + struct damon_ctx *ctx = damon_reclaim_ctx();
> > int err = 0;
> >
> > err = damon_set_attrs(ctx, &damon_reclaim_mon_attrs);
> > @@ -244,7 +255,8 @@ static int damon_reclaim_apply_parameters(void)
> > }
> > damon_set_schemes(ctx, &scheme, 1);
> >
> > - return damon_set_region_biggest_system_ram_default(target,
> > + return damon_set_region_biggest_system_ram_default(
> > + damon_reclaim_target(),
> > &monitor_region_start,
> > &monitor_region_end);
> > }
> > @@ -254,7 +266,7 @@ static int damon_reclaim_turn(bool on)
> > int err;
> >
> > if (!on) {
> > - err = damon_stop(&ctx, 1);
> > + err = damon_stop(kdamond);
> > if (!err)
> > kdamond_pid = -1;
> > return err;
> > @@ -264,10 +276,10 @@ static int damon_reclaim_turn(bool on)
> > if (err)
> > return err;
> >
> > - err = damon_start(&ctx, 1, true);
> > + err = damon_start(kdamond, true);
> > if (err)
> > return err;
> > - kdamond_pid = ctx->kdamond->pid;
> > + kdamond_pid = kdamond->self->pid;
> > return 0;
> > }
> >
> > @@ -286,7 +298,7 @@ static int damon_reclaim_enabled_store(const char *val,
> > return 0;
> >
> > /* Called before init function. The function will handle this. */
> > - if (!ctx)
> > + if (!kdamond)
> > goto set_param_out;
> >
> > err = damon_reclaim_turn(enable);
> > @@ -337,11 +349,13 @@ static int damon_reclaim_after_wmarks_check(struct damon_ctx *c)
> >
> > static int __init damon_reclaim_init(void)
> > {
> > - int err = damon_modules_new_paddr_ctx_target(&ctx, &target);
> > + struct damon_ctx *ctx;
> > + int err = damon_modules_new_paddr_kdamond(&kdamond);
> >
> > if (err)
> > return err;
> >
> > + ctx = damon_reclaim_ctx();
> > ctx->callback.after_wmarks_check = damon_reclaim_after_wmarks_check;
> > ctx->callback.after_aggregation = damon_reclaim_after_aggregation;
> >
> > diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> > index 6fee383bc..bfdb979e6 100644
> > --- a/mm/damon/sysfs.c
> > +++ b/mm/damon/sysfs.c
>
> Ditto. Many of below changes could be separated into another patch in my
> opinion.
>
> > @@ -939,7 +939,7 @@ static const struct kobj_type damon_sysfs_contexts_ktype = {
> > struct damon_sysfs_kdamond {
> > struct kobject kobj;
> > struct damon_sysfs_contexts *contexts;
> > - struct damon_ctx *damon_ctx;
> > + struct kdamond *kdamond;
> > };
> >
> > static struct damon_sysfs_kdamond *damon_sysfs_kdamond_alloc(void)
> > @@ -974,16 +974,6 @@ static void damon_sysfs_kdamond_rm_dirs(struct damon_sysfs_kdamond *kdamond)
> > kobject_put(&kdamond->contexts->kobj);
> > }
> >
> > -static bool damon_sysfs_ctx_running(struct damon_ctx *ctx)
> > -{
> > - bool running;
> > -
> > - mutex_lock(&ctx->kdamond_lock);
> > - running = ctx->kdamond != NULL;
> > - mutex_unlock(&ctx->kdamond_lock);
> > - return running;
> > -}
>
> Don't add replacement of this in mm/damon/core.c. Modify this here.
DebugFS also uses this.
>
> > -
> > /*
> > * enum damon_sysfs_cmd - Commands for a specific kdamond.
> > */
> > @@ -1065,15 +1055,15 @@ static struct damon_sysfs_cmd_request damon_sysfs_cmd_request;
> > static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
> > char *buf)
> > {
> > - struct damon_sysfs_kdamond *kdamond = container_of(kobj,
> > + struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj,
> > struct damon_sysfs_kdamond, kobj);
>
> This makes reviewing bit difficult. Let's keep current name for now.
>
> > - struct damon_ctx *ctx = kdamond->damon_ctx;
> > + struct kdamond *kdamond = sys_kdamond->kdamond;
>
> Keep 'kdamond' variable name as is, and use shorter name for this variable,
> say, kd, or do kdamond->kdamond.
It usually makes reading code difficult, these are two different abstraction of
kdamond, but the name will almost be the same, I don't think this is a good idea.
>
> > bool running;
> >
> > - if (!ctx)
> > + if (!kdamond)
>
> "if (!kd)" or "if (!kdamond->kdamond)"
And this is an example, I can't say which one belong to sysfs's abstraction
and which represents core's kdamond. With "sys_" prefix or "sysfs_" sometimes,
it is much easier to read.
>
> > running = false;
> > else
> > - running = damon_sysfs_ctx_running(ctx);
> > + running = damon_kdamond_running(kdamond);
> >
> > return sysfs_emit(buf, "%s\n", running ?
> > damon_sysfs_cmd_strs[DAMON_SYSFS_CMD_ON] :
> > @@ -1242,13 +1232,15 @@ static bool damon_sysfs_schemes_regions_updating;
> > static void damon_sysfs_before_terminate(struct damon_ctx *ctx)
> > {
> > struct damon_target *t, *next;
> > - struct damon_sysfs_kdamond *kdamond;
> > + struct damon_sysfs_kdamond *sys_kdamond;
> > + struct kdamond *kdamond;
> > enum damon_sysfs_cmd cmd;
> >
> > /* damon_sysfs_schemes_update_regions_stop() might not yet called */
> > - kdamond = damon_sysfs_cmd_request.kdamond;
> > + kdamond = ctx->kdamond;
> > + sys_kdamond = damon_sysfs_cmd_request.kdamond;
> > cmd = damon_sysfs_cmd_request.cmd;
> > - if (kdamond && ctx == kdamond->damon_ctx &&
> > + if (sys_kdamond && kdamond == sys_kdamond->kdamond &&
> > (cmd == DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_REGIONS ||
> > cmd == DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_BYTES) &&
> > damon_sysfs_schemes_regions_updating) {
>
> Again, changed names makes me reviewing difficult.
>
> >From this point, I think below changes could be significantly changed in next
> version of this patchset if you agree to above comments. So I'm stop reviewing
> this patch from here for now. Please let me know if you have different opinion
> about it.
Thank you very much for your comments, I really appreciate that you spent time to
review this. I'll try to improve the patch-set in next versions!
>
>
> Thanks,
> SJ
>
> [...]
BR,
Alex
On Sun, 2 Jun 2024 19:15:57 +0300 Alex Rusuf <[email protected]> wrote:
> Hi SJ,
>
> > Hi Alex,
> >
> > On Fri, 31 May 2024 15:23:19 +0300 Alex Rusuf <[email protected]> wrote:
> >
> > > In current implementation kdamond tracks only 1
> > > context, that is kdamond _is_ damon_ctx what makes
> > > it very difficult to implement multiple contexts.
> > >
> > > This patch adds another level of abstraction, that is
> > > 'struct kdamond' - structure which represents kdamond itself.
> > > It holds references to all contexts organized in list.
> > >
> > > Few fields like ->kdamond_started and ->kdamond_lock
> > > (just ->lock for 'struct kdamond') also has been moved
> > > to 'struct kdamond', because they have nothing to do
> > > with the context itself, but with the whole kdamond
> > > daemon.
> > >
> > > Signed-off-by: Alex Rusuf <[email protected]>
> > > ---
> > > include/linux/damon.h | 73 ++++++++---
> > > mm/damon/core.c | 249 ++++++++++++++++++++++-------------
> > > mm/damon/lru_sort.c | 31 +++--
> > > mm/damon/modules-common.c | 36 +++--
> > > mm/damon/modules-common.h | 3 +-
> > > mm/damon/reclaim.c | 30 +++--
> > > mm/damon/sysfs.c | 268 ++++++++++++++++++++++++--------------
> > > 7 files changed, 463 insertions(+), 227 deletions(-)
> > >
[...]
> > > +/**
> > > + * damon_kdamond_running() - Return true if kdamond is running
> > > + * false otherwise.
> > > + */
> > > +bool damon_kdamond_running(struct kdamond *kdamond)
> > > +{
> > > + bool running;
> > > +
> > > + mutex_lock(&kdamond->lock);
> > > + running = kdamond->self != NULL;
> > > + mutex_unlock(&kdamond->lock);
> > > + return running;
> > > }
> >
> > Seems this function is used by only DAMON sysfs interface. Don't implement
> > unnecessary public function. Implement it in mm/damon/sysfs.c please.
>
> DebugFS interface also uses this.
I think dbgfs.c is using damon_nr_running_ctxs() now? De-duplicating code is
nice, but I think that can be done in a separate patch. Moreover, DAMON
debugfs interface is deprecated now. I'd like to keep changes to the file as
minimum as possible.
[...]
> > > +static inline struct damon_ctx *damon_lru_sort_ctx(void)
> > > +{
> > > + return damon_first_ctx(kdamond);
> > > +}
> > > +
> > > +static inline struct damon_target *damon_lru_sort_target(void)
> > > +{
> > > + return damon_first_target(
> > > + damon_lru_sort_ctx());
> > > +}
> >
> > Do we really need above two functions? Can't we simply add 'kdamond' global
> > variable but still uses 'ctx' and 'target' as before, except interface changed
> > function calls?
>
> We can, I just tried to use 'readable' interface, I thought
> that using raw pointers from kdamond will be less readable.
I agree this will eventually be better code. But let's do such things in
separte patches. Keeping individual patch simple and small would help poor
reviewers :)
[...]
> > > @@ -1065,15 +1055,15 @@ static struct damon_sysfs_cmd_request damon_sysfs_cmd_request;
> > > static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
> > > char *buf)
> > > {
> > > - struct damon_sysfs_kdamond *kdamond = container_of(kobj,
> > > + struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj,
> > > struct damon_sysfs_kdamond, kobj);
> >
> > This makes reviewing bit difficult. Let's keep current name for now.
> >
> > > - struct damon_ctx *ctx = kdamond->damon_ctx;
> > > + struct kdamond *kdamond = sys_kdamond->kdamond;
> >
> > Keep 'kdamond' variable name as is, and use shorter name for this variable,
> > say, kd, or do kdamond->kdamond.
>
> It usually makes reading code difficult, these are two different abstraction of
> kdamond, but the name will almost be the same, I don't think this is a good idea.
>
> >
> > > bool running;
> > >
> > > - if (!ctx)
> > > + if (!kdamond)
> >
> > "if (!kd)" or "if (!kdamond->kdamond)"
>
> And this is an example, I can't say which one belong to sysfs's abstraction
> and which represents core's kdamond. With "sys_" prefix or "sysfs_" sometimes,
> it is much easier to read.
I agree that. But from my perspective, these name changes are not essential
for the purpose of this patch but only make review time consuming. Do one
thing with minimum change, please. You could do the code cleanup in other
patches.
[...]
> >
> > >From this point, I think below changes could be significantly changed in next
> > version of this patchset if you agree to above comments. So I'm stop reviewing
> > this patch from here for now. Please let me know if you have different opinion
> > about it.
>
> Thank you very much for your comments, I really appreciate that you spent time to
> review this. I'll try to improve the patch-set in next versions!
No problem, and looking forward to the next version!
Thanks,
SJ
[...]
[...]
> >
> > >
> > > >
> > > > /* private: */
> > > > /* for waiting until the execution of the kdamond_fn is started */
> > > > @@ -634,7 +632,10 @@ struct damon_ctx {
> > > > * update
> > > > */
> > > > unsigned long next_ops_update_sis;
> > > > + /* upper limit for each monitoring region */
> > > > unsigned long sz_limit;
> > > > + /* marker to check if context is valid */
> > > > + bool valid;
> > >
> > > What is the validity?
> >
> > This is a "flag" which indicates that the context is "valid" for kdamond
> > to be able to call ->check_accesses() on it. Because we have many contexts
> > we need a way to identify which context is valid while iterating over
> > all of them (please, see kdamond_prepare_access_checks_ctx() function).
>
> It's still not very clear to me. When it is "valid" or not for kdamond to be
> able to call ->check_accesses()? I assume you mean it is valid if the
> monitoring operations set's ->target_valid() returns zero?
>
> The callback is not for preventing unnecessary ->check_accesses(), but for
> knowing if continuing the monitoring makes sense or not. For example, the
> callback of 'vaddr' operation set checks if the virtual address space still
> exists (whether the target process is still running) Calling
> ->check_accesses() for ->target_valid() returned non-zero target is totally ok,
> though it is meaningless. And therefore kdamond stops if it returns non-zero.
>
> >
> > Maybe name should be changed,
>
> At least some more detailed comment would be appreciated, imo.
>
> > but now I don't see a way how we could merge
> > this into kdamond_valid_ctx() or so, because the main cause of this "valid"
> > bit is that there's no more "valid" targets for this context, but also we could
> > have ->after_sampling() returned something bad.
>
> As mentioned above, calling ->check_accesses() or others towards invalidated
> targets (e.g., terminated processes's virtual address spaces) should be ok, if
> any of the targets are still valid. So I don't show special technical
> difficulties here. Please let me know if I'm missing something.
This is true in case of only 1 context. kdamond can be stopped if there's
no valid targets for this context (e.g. no address space exists anymore),
but when there are many contexts we need to check if any of contexts has
valid target(s). For example, we have 3 contexts per kdamond, at some
point of time we have 1st context having no valid targets (process has been
finished), but other 2 contexts do have valid targets, so we don't need
to call ->prepare_access_checks() and ->check_accesses() as well for 1st
context, but we do need to call them for other contexts.
We can call ->kdamond_valid_ctx() before calling ->check_accesses(),
but then we also need to check if nothing bad has been returned from
->after_sampling() call, so that we're allowed to further call
->check_accesses().
I decided to use a "valid" flag inside damon_ctx structure, so
that if this context isn't valid, we will just skip it while
iterating over all contexts. In case of just 1 context
kdamond_prepare_access_checks() will return false, so that
nr_valid_ctxs will remain zero, so we will break "while"
loop, otherwise we'll see that there's at least one valid
context to call ->check_accesses() for.
The last point is that we need to understand for which
context we can call ->check_accesses(), this is done
by checking ctx->valid bit, if it is "false" (we already called
kdamond_valid_ctx() and ->after_sampling(), and one of this calls
failed) we just skip this context by doing "goto next_ctx;"
in main kdamond loop.
>
> >
> > >
> > > >
> > > > /* public: */
> > > > struct kdamond *kdamond;
> > > > @@ -682,6 +683,12 @@ static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond)
> > > > return list_first_entry(&kdamond->contexts, struct damon_ctx, list);
> > > > }
> > > >
> > > > +static inline bool damon_is_last_ctx(struct damon_ctx *ctx,
> > > > + struct kdamond *kdamond)
> > > > +{
> > > > + return list_is_last(&ctx->list, &kdamond->contexts);
> > > > +}
> > > > +
> > > > #define damon_for_each_region(r, t) \
> > > > list_for_each_entry(r, &t->regions_list, list)
> > > >
> > > > diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> > > > index 23200aabc..d5287566c 100644
> > > > --- a/include/trace/events/damon.h
> > > > +++ b/include/trace/events/damon.h
> > >
> > > Let's separate this change to another patch.
> >
> > Separating patches we hardly be able to reach at least build
> > consistency between patches. Moreover DAMON won't be able
> > to function corretly in between.
>
> I agree that it's not very easy, indeed. But let's try. In terms of
> functionality, we need to keep the old behavior that visible to users. For
> example, this change tries to make the traceevent changed for the multi
> contexts support. It is for making the behavior _better_, not for keeping old
> behavior. Rather than that, this is introducing a new change to the tracepoint
> output. Just make no change here. Users may get confused when they use
> multiple contexts, but what they see is not changed.
>
> Further, you can delay letting users (user-space) using the multiple contexts
> (allowing >1 input to nr_contexts of DAMON sysfs interface) after making this
> change in a separate patch.
>
> >
> > >
[...]
> > > >
> > > > -static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> > > > - struct damon_region *r, struct damos *s)
> > > > +static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c,
> > > > + struct damon_target *t, struct damon_region *r,
> > > > + struct damos *s)
> > >
> > > Unnecesary change.
> >
> > What do you mean? Why is this unnecessary? Now DAMON iterates over
> > contexts and calls kdamond_apply_schemes(ctx), so how can we know
> > which context we print trace for? Sorry, maybe I misunderstood
> > how DAMON does it, but I'm a bit confused.
>
> I believe the above comment to tracevent change explains this.
Just to make it clear, you mean separating this change into different
smaller patch?
[...]
BR,
Alex
On Sun, 2 Jun 2024 22:48:14 +0300 Alex Rusuf <[email protected]> wrote:
> [...]
>
> > >
> > > >
> > > > >
> > > > > /* private: */
> > > > > /* for waiting until the execution of the kdamond_fn is started */
> > > > > @@ -634,7 +632,10 @@ struct damon_ctx {
> > > > > * update
> > > > > */
> > > > > unsigned long next_ops_update_sis;
> > > > > + /* upper limit for each monitoring region */
> > > > > unsigned long sz_limit;
> > > > > + /* marker to check if context is valid */
> > > > > + bool valid;
> > > >
> > > > What is the validity?
> > >
> > > This is a "flag" which indicates that the context is "valid" for kdamond
> > > to be able to call ->check_accesses() on it. Because we have many contexts
> > > we need a way to identify which context is valid while iterating over
> > > all of them (please, see kdamond_prepare_access_checks_ctx() function).
> >
> > It's still not very clear to me. When it is "valid" or not for kdamond to be
> > able to call ->check_accesses()? I assume you mean it is valid if the
> > monitoring operations set's ->target_valid() returns zero?
> >
> > The callback is not for preventing unnecessary ->check_accesses(), but for
> > knowing if continuing the monitoring makes sense or not. For example, the
> > callback of 'vaddr' operation set checks if the virtual address space still
> > exists (whether the target process is still running) Calling
> > ->check_accesses() for ->target_valid() returned non-zero target is totally ok,
> > though it is meaningless. And therefore kdamond stops if it returns non-zero.
> >
> > >
> > > Maybe name should be changed,
> >
> > At least some more detailed comment would be appreciated, imo.
> >
> > > but now I don't see a way how we could merge
> > > this into kdamond_valid_ctx() or so, because the main cause of this "valid"
> > > bit is that there's no more "valid" targets for this context, but also we could
> > > have ->after_sampling() returned something bad.
> >
> > As mentioned above, calling ->check_accesses() or others towards invalidated
> > targets (e.g., terminated processes's virtual address spaces) should be ok, if
> > any of the targets are still valid. So I don't show special technical
> > difficulties here. Please let me know if I'm missing something.
>
> This is true in case of only 1 context. kdamond can be stopped if there's
> no valid targets for this context (e.g. no address space exists anymore),
> but when there are many contexts we need to check if any of contexts has
> valid target(s). For example, we have 3 contexts per kdamond, at some
> point of time we have 1st context having no valid targets (process has been
> finished), but other 2 contexts do have valid targets, so we don't need
> to call ->prepare_access_checks() and ->check_accesses() as well for 1st
> context, but we do need to call them for other contexts.
Yes, we don't need to. Nonetheless, calling ->prepare_access_checks() is also
not a big problem, right? If I'm not wrong, I don't want to add more
complexity for unclear benefit. In other words, I think simply letting a
kdamond continues access monitoring for virtual address space targets even
after the processes are terminated while there are other contexts that need to
continue access monitoring is ok, unless it has clear and significant problem.
>
> We can call ->kdamond_valid_ctx() before calling ->check_accesses(),
> but then we also need to check if nothing bad has been returned from
> ->after_sampling() call, so that we're allowed to further call
> ->check_accesses().
>
> I decided to use a "valid" flag inside damon_ctx structure, so
> that if this context isn't valid, we will just skip it while
> iterating over all contexts.
If this is really needed, why don't you simply call ->target_valid() for each
target for whenever we need to know if the target is valid?
Thanks,
SJ
[...]