On Tue, 25 Feb 2025 22:36:41 -0800 SeongJae Park <sj@xxxxxxxxxx> wrote:
> Currently all DAMON kernel API callers do online DAMON parameters commit
> from damon_callback->after_aggregation because only those are safe place
> to call the DAMON monitoring attributes update function, namely
> damon_set_attrs().
>
> Because damon_callback hooks provides no synchronization, the callers
> works in asynchronous ways or implement their own inefficient and
> complicated synchronization mechanisms. It also means online DAMON
> parameters commit can take up to one aggregation interval. On large
> systems having long aggregation intervals, that can be too slow. The
> synchronization can be done in more efficient and simple way while
> removing the latency constraint if it can be done using damon_call().
>
> The fact that damon_call() can be executed in the middle of the
> aggregation makes damon_set_attrs() unsafe to be called from it, though.
> Two real problems can occur in the case. First, converting the not yet
> completely aggregated nr_accesses for new user-set intervals can
> arguably degrade the accuracy or at least make the logic complicated.
> Second, kdamond_reset_aggregated() will not be called after the
> monitoring results update, so next aggregation starts from unclean
> state. This can result in inconsistent and unexpected nr_accesses.
>
> Make it safe as follows. Catch the middle-of-the-aggregation case from
> damon_set_attrs() and pass the information to nr_accesses conversion
> logic. The logic works as before if this is not the case (called after
> the current aggregation is completed). If not, it drops the nr_accesses
> information that so far aggregated, and make the status same to the
> beginning of this aggregation, but as if the last aggregation was ran
> with the updated sampling/aggregation intervals.
This itself has no problem. But this can cause a problem when this is applied
on top of DAMON monitoring intervals auto-tune patch[1], which makes
damon_set_attrs() can also be called from if-caluse for aggregated information
sharing and reset.
The problematic case happens when damon_set_attrs() from kdamond_call() and
kdamond_tune_intervals() in same iteration. It means it is the last iteration
of the current aggregation. damon_set_attrs() that called from kdamond_call()
understands the aggregation is finished and updates aggregation information
correctly. But, it also updates damon_ctx->next_aggregation_sis.
When damon_set_attrs() is called again from kdamond_tune_intervals(), it shows
the prior damon_set_attrs() invocation updated ->next_aggregation_sis and think
this is in the middle of the aggregation. Hence it further resets the
aggregated information. Now, kdamond_reset_aggregated() is called after the
second invocation of damon_set_attrs() in the same kdamond main loop iteration,
and corrupts the aggregated information of regions. Particularly, this can mad
the pseudo-moving-sum access frequency information broken.
Simplest fix would be resetting the prior damon_set_attrs() updated
->next_intervals_tune_sis, like below diff.
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2538,6 +2538,24 @@ static int kdamond_fn(void *data)
if (ctx->attrs.intervals_goal.aggrs &&
ctx->passed_sample_intervals >=
ctx->next_intervals_tune_sis) {
+ /*
+ * ctx->next_aggregation_sis might be updated
+ * from kdamond_call(). In the case,
+ * damon_set_attrs() which will be called from
+ * kdamond_tune_interval() may wrongly think
+ * this is in the middle of the current
+ * aggregation, and make aggregation
+ * information reset for all regions. Then,
+ * following kdamond_reset_aggregated() call
+ * will make the region information invalid,
+ * particularly for ->nr_accesses_bp.
+ *
+ * Reset ->next_aggregation_sis to avoid that.
+ * It will anyway correctly updated after this
+ * if caluse.
+ */
+ ctx->next_aggregation_sis =
+ next_aggregation_sis;
ctx->next_intervals_tune_sis +=
ctx->attrs.aggr_samples *
ctx->attrs.intervals_goal.aggrs;
I will add the change to this patch or the autotune patch, depending on their
submission order.
[1] https://lore.kernel.org/20250228220328.49438-3-sj@xxxxxxxxxx
Thanks,
SJ
>
> Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
> ---
> mm/damon/core.c | 38 ++++++++++++++++++++++++++-----------
> mm/damon/tests/core-kunit.h | 6 +++---
> 2 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 0578e89dff13..5b807caaec95 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -602,11 +602,25 @@ static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
> }
>
> static void damon_update_monitoring_result(struct damon_region *r,
> - struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
> + struct damon_attrs *old_attrs, struct damon_attrs *new_attrs,
> + bool aggregating)
> {
> - r->nr_accesses = damon_nr_accesses_for_new_attrs(r->nr_accesses,
> - old_attrs, new_attrs);
> - r->nr_accesses_bp = r->nr_accesses * 10000;
> + if (!aggregating) {
> + r->nr_accesses = damon_nr_accesses_for_new_attrs(
> + r->nr_accesses, old_attrs, new_attrs);
> + r->nr_accesses_bp = r->nr_accesses * 10000;
> + } else {
> + /*
> + * if this is called in the middle of the aggregation, reset
> + * the aggregations we made so far for this aggregation
> + * interval. In other words, make the status like
> + * kdamond_reset_aggregated() is called.
> + */
> + r->last_nr_accesses = damon_nr_accesses_for_new_attrs(
> + r->last_nr_accesses, old_attrs, new_attrs);
> + r->nr_accesses_bp = r->last_nr_accesses * 10000;
> + r->nr_accesses = 0;
> + }
> r->age = damon_age_for_new_attrs(r->age, old_attrs, new_attrs);
> }
>
> @@ -619,7 +633,7 @@ static void damon_update_monitoring_result(struct damon_region *r,
> * ->nr_accesses and ->age of given damon_ctx's regions for new damon_attrs.
> */
> static void damon_update_monitoring_results(struct damon_ctx *ctx,
> - struct damon_attrs *new_attrs)
> + struct damon_attrs *new_attrs, bool aggregating)
> {
> struct damon_attrs *old_attrs = &ctx->attrs;
> struct damon_target *t;
> @@ -634,7 +648,7 @@ static void damon_update_monitoring_results(struct damon_ctx *ctx,
> damon_for_each_target(t, ctx)
> damon_for_each_region(r, t)
> damon_update_monitoring_result(
> - r, old_attrs, new_attrs);
> + r, old_attrs, new_attrs, aggregating);
> }
>
> /*
> @@ -661,10 +675,10 @@ static bool damon_valid_intervals_goal(struct damon_attrs *attrs)
> * @ctx: monitoring context
> * @attrs: monitoring attributes
> *
> - * This function should be called while the kdamond is not running, or an
> - * access check results aggregation is not ongoing (e.g., from
> - * &struct damon_callback->after_aggregation or
> - * &struct damon_callback->after_wmarks_check callbacks).
> + * This function should be called while the kdamond is not running, an access
> + * check results aggregation is not ongoing (e.g., from &struct
> + * damon_callback->after_aggregation or &struct
> + * damon_callback->after_wmarks_check callbacks), or from damon_call().
> *
> * Every time interval is in micro-seconds.
> *
> @@ -675,6 +689,8 @@ int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs)
> unsigned long sample_interval = attrs->sample_interval ?
> attrs->sample_interval : 1;
> struct damos *s;
> + bool aggregating = ctx->passed_sample_intervals <
> + ctx->next_aggregation_sis;
>
> if (!damon_valid_intervals_goal(attrs))
> return -EINVAL;
> @@ -695,7 +711,7 @@ int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs)
> ctx->next_ops_update_sis = ctx->passed_sample_intervals +
> attrs->ops_update_interval / sample_interval;
>
> - damon_update_monitoring_results(ctx, attrs);
> + damon_update_monitoring_results(ctx, attrs, aggregating);
> ctx->attrs = *attrs;
>
> damon_for_each_scheme(s, ctx)
> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 532c6a6f21f9..be0fea9ee5fc 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -348,19 +348,19 @@ static void damon_test_update_monitoring_result(struct kunit *test)
>
> new_attrs = (struct damon_attrs){
> .sample_interval = 100, .aggr_interval = 10000,};
> - damon_update_monitoring_result(r, &old_attrs, &new_attrs);
> + damon_update_monitoring_result(r, &old_attrs, &new_attrs, false);
> KUNIT_EXPECT_EQ(test, r->nr_accesses, 15);
> KUNIT_EXPECT_EQ(test, r->age, 2);
>
> new_attrs = (struct damon_attrs){
> .sample_interval = 1, .aggr_interval = 1000};
> - damon_update_monitoring_result(r, &old_attrs, &new_attrs);
> + damon_update_monitoring_result(r, &old_attrs, &new_attrs, false);
> KUNIT_EXPECT_EQ(test, r->nr_accesses, 150);
> KUNIT_EXPECT_EQ(test, r->age, 2);
>
> new_attrs = (struct damon_attrs){
> .sample_interval = 1, .aggr_interval = 100};
> - damon_update_monitoring_result(r, &old_attrs, &new_attrs);
> + damon_update_monitoring_result(r, &old_attrs, &new_attrs, false);
> KUNIT_EXPECT_EQ(test, r->nr_accesses, 150);
> KUNIT_EXPECT_EQ(test, r->age, 20);
>
> --
> 2.39.5
