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 <yorha.op@gmail.com>
---
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 " \
"damon@lists.linux.dev and linux-mm@kvack.org.\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
Hello,
kernel test robot noticed "kernel-selftests.damon.sysfs.sh.fail" on:
commit: c1c2595bf89a145e63d08dbfa297951caaa91544 ("[PATCH v2 2/2] mm/damon/core: implement multi-context support")
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/all/20240531122320.909060-3-yorha.op@gmail.com/
patch subject: [PATCH v2 2/2] mm/damon/core: implement multi-context support
in testcase: kernel-selftests
version: kernel-selftests-x86_64-977d51cf-1_20240508
with following parameters:
group: group-01
compiler: gcc-13
test machine: 36 threads 1 sockets Intel(R) Core(TM) i9-10980XE CPU @ 3.00GHz (Cascade Lake) with 32G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
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 <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202406171435.2c3aa362-oliver.sang@intel.com
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240617/202406171435.2c3aa362-oliver.sang@intel.com
(as in attached kernel-selftests.xz)
# timeout set to 300
# selftests: damon: sysfs.sh
# writing 2 to /sys/kernel/mm/damon/admin/kdamonds/0/contexts/nr_contexts succeed ()
# expected failure because only 0/1 are supported
not ok 10 selftests: damon: sysfs.sh # exit=1
--
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: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240601/202406010349.tixi9sK3-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/202406010349.tixi9sK3-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406010349.tixi9sK3-lkp@intel.com/
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 Alex,
On Fri, 31 May 2024 15:23:20 +0300 Alex Rusuf <yorha.op@gmail.com> 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 <yorha.op@gmail.com>
> ---
> 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 SJ,
> Hi Alex,
>
> On Fri, 31 May 2024 15:23:20 +0300 Alex Rusuf <yorha.op@gmail.com> 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 <yorha.op@gmail.com>
> > ---
> > 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
On Sun, 2 Jun 2024 18:08:16 +0300 Alex Rusuf <yorha.op@gmail.com> wrote:
> Hi SJ,
>
> > Hi Alex,
> >
> > On Fri, 31 May 2024 15:23:20 +0300 Alex Rusuf <yorha.op@gmail.com> 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 <yorha.op@gmail.com>
> > > ---
> > > 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
[...]
[...]
> >
> > >
> > > >
> > > > /* 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 <yorha.op@gmail.com> 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
[...]
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/202406010337.eZEQ5XmV-lkp@intel.com/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/202406010337.eZEQ5XmV-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406010337.eZEQ5XmV-lkp@intel.com/
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
© 2016 - 2026 Red Hat, Inc.