* [RFC PATCH 01/13] mm/damon/sysfs: validate user inputs from damon_sysfs_commit_input()
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 02/13] mm/damon/core: invoke kdamond_call() after merging is done if possible SeongJae Park
` (11 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
Online DAMON parameters commit via DAMON sysfs interface can make
kdamond stop. This behavior was made because it can make the
implementation simpler. The implementation simply tries committing the
parameter without validation. If it finds something wrong, it returns
error without reverting partially committed parameters back. It is safe
though, since it breaks kdamond main loop in the case of the error
return.
Users can make the wrong parameters by mistake, though. Validating the
input parameters first and returning the error when some parameters
wrong, while letting kdamond continues running with the old parameters
would be the better behavior. This behavior can also make damon_call()
carrying the online commit instead of the damon_callback hook in future
easier, because damon_call() cannot directly break kdamond main loop.
Implement the better behavior.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/sysfs.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index ccd435d234b9..87e4c6e3614e 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1449,11 +1449,11 @@ static struct damon_ctx *damon_sysfs_build_ctx(
* damon_sysfs_commit_input() - Commit user inputs to a running kdamond.
* @kdamond: The kobject wrapper for the associated kdamond.
*
- * If the sysfs input is wrong, the kdamond will be terminated.
+ * Returns error if the sysfs input is wrong.
*/
static int damon_sysfs_commit_input(struct damon_sysfs_kdamond *kdamond)
{
- struct damon_ctx *param_ctx;
+ struct damon_ctx *param_ctx, *test_ctx;
int err;
if (!damon_sysfs_kdamond_running(kdamond))
@@ -1465,7 +1465,15 @@ static int damon_sysfs_commit_input(struct damon_sysfs_kdamond *kdamond)
param_ctx = damon_sysfs_build_ctx(kdamond->contexts->contexts_arr[0]);
if (IS_ERR(param_ctx))
return PTR_ERR(param_ctx);
+ test_ctx = damon_new_ctx();
+ err = damon_commit_ctx(test_ctx, param_ctx);
+ if (err) {
+ damon_sysfs_destroy_targets(test_ctx);
+ damon_destroy_ctx(test_ctx);
+ goto out;
+ }
err = damon_commit_ctx(kdamond->damon_ctx, param_ctx);
+out:
damon_sysfs_destroy_targets(param_ctx);
damon_destroy_ctx(param_ctx);
return err;
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 02/13] mm/damon/core: invoke kdamond_call() after merging is done if possible
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 01/13] mm/damon/sysfs: validate user inputs from damon_sysfs_commit_input() SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 03/13] mm/damon/core: make damon_set_attrs() be safe to be called from damon_call() SeongJae Park
` (10 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
kdamond_call() callers may iterate the regions, so better to call it
when the number of regions is as small as possible. It is when
kdamond_merge_regions() is finished. Invoke it on the point.
This change is also for making future change for carrying online
parameters commit with damon_call() easier. The commit work should be
able to make sequence between other aggregation interval based
operations including regioins merging and aggregation reset. Placing
damon_call() invocation after the regions merging makes the sequence
handling simpler.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 310cdc87d5f4..0578e89dff13 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2412,7 +2412,6 @@ static int kdamond_fn(void *data)
if (ctx->callback.after_sampling &&
ctx->callback.after_sampling(ctx))
break;
- kdamond_call(ctx, false);
kdamond_usleep(sample_interval);
ctx->passed_sample_intervals++;
@@ -2430,9 +2429,10 @@ static int kdamond_fn(void *data)
}
/*
- * do kdamond_apply_schemes() after kdamond_merge_regions() if
- * possible, to reduce overhead
+ * do kdamond_call() and kdamond_apply_schemes() after
+ * kdamond_merge_regions() if possible, to reduce overhead
*/
+ kdamond_call(ctx, false);
if (!list_empty(&ctx->schemes))
kdamond_apply_schemes(ctx);
else
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 03/13] mm/damon/core: make damon_set_attrs() be safe to be called from damon_call()
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 01/13] mm/damon/sysfs: validate user inputs from damon_sysfs_commit_input() SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 02/13] mm/damon/core: invoke kdamond_call() after merging is done if possible SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-03-02 21:41 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 04/13] mm/damon/sysfs: handle commit command using damon_call() SeongJae Park
` (9 subsequent siblings)
12 siblings, 1 reply; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
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.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC PATCH 03/13] mm/damon/core: make damon_set_attrs() be safe to be called from damon_call()
2025-02-26 6:36 ` [RFC PATCH 03/13] mm/damon/core: make damon_set_attrs() be safe to be called from damon_call() SeongJae Park
@ 2025-03-02 21:41 ` SeongJae Park
0 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-03-02 21:41 UTC (permalink / raw)
To: SeongJae Park; +Cc: Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
On Tue, 25 Feb 2025 22:36:41 -0800 SeongJae Park <sj@kernel.org> 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@kernel.org
Thanks,
SJ
>
> Signed-off-by: SeongJae Park <sj@kernel.org>
> ---
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH 04/13] mm/damon/sysfs: handle commit command using damon_call()
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (2 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 03/13] mm/damon/core: make damon_set_attrs() be safe to be called from damon_call() SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 05/13] mm/damon/sysfs: remove damon_sysfs_cmd_request code from damon_sysfs_handle_cmd() SeongJae Park
` (8 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
DAMON sysfs interface is using damon_callback->after_aggregation hook
with its self-implemented synchronization mechanism for the hook. It is
inefficient, complicated, and take up to one aggregation interval to
complete, which can be long on some configs.
Instead use damon_call(). It provides a synchronization mechanism that
built inside DAMON's core layer, so more efficient than DAMON sysfs
interface's own one. Also it isolates the implementation inside the
core layer, and hence makes it easier to be maintained. Finally, it
takes up to one sampling interval, which is much shorter than the
aggregation interval in common setups.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/sysfs.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 87e4c6e3614e..c55a2cee4b74 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1451,8 +1451,9 @@ static struct damon_ctx *damon_sysfs_build_ctx(
*
* Returns error if the sysfs input is wrong.
*/
-static int damon_sysfs_commit_input(struct damon_sysfs_kdamond *kdamond)
+static int damon_sysfs_commit_input(void *data)
{
+ struct damon_sysfs_kdamond *kdamond = data;
struct damon_ctx *param_ctx, *test_ctx;
int err;
@@ -1550,11 +1551,6 @@ static int damon_sysfs_cmd_request_callback(struct damon_ctx *c, bool active,
if (!kdamond || kdamond->damon_ctx != c)
goto out;
switch (damon_sysfs_cmd_request.cmd) {
- case DAMON_SYSFS_CMD_COMMIT:
- if (!after_aggregation)
- goto out;
- err = damon_sysfs_commit_input(kdamond);
- break;
default:
break;
}
@@ -1712,11 +1708,7 @@ static int damon_sysfs_update_schemes_tried_regions(
* @cmd: The command to handle.
* @kdamond: The kobject wrapper for the associated kdamond.
*
- * This function handles a DAMON sysfs command for a kdamond. For commands
- * that need to access running DAMON context-internal data, it requests
- * handling of the command to the DAMON callback
- * (@damon_sysfs_cmd_request_callback()) and wait until it is properly handled,
- * or the context is completed.
+ * This function handles a DAMON sysfs command for a kdamond.
*
* Return: 0 on success, negative error code otherwise.
*/
@@ -1730,6 +1722,9 @@ static int damon_sysfs_handle_cmd(enum damon_sysfs_cmd cmd,
return damon_sysfs_turn_damon_on(kdamond);
case DAMON_SYSFS_CMD_OFF:
return damon_sysfs_turn_damon_off(kdamond);
+ case DAMON_SYSFS_CMD_COMMIT:
+ return damon_sysfs_damon_call(
+ damon_sysfs_commit_input, kdamond);
case DAMON_SYSFS_CMD_COMMIT_SCHEMES_QUOTA_GOALS:
return damon_sysfs_damon_call(
damon_sysfs_commit_schemes_quota_goals,
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 05/13] mm/damon/sysfs: remove damon_sysfs_cmd_request code from damon_sysfs_handle_cmd()
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (3 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 04/13] mm/damon/sysfs: handle commit command using damon_call() SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 06/13] mm/damon/sysfs: remove damon_sysfs_cmd_request_callback() and its callers SeongJae Park
` (7 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
damon_sysfs_handle_cmd() handles user requests that it can directly
handle on its own. For requests that need to be handled from
damon_callback hooks, it uses DAMON sysfs interface's own synchronous
damon_callback hooks management mechanism, namely
damon_sysfs_cmd_request. Now all user requests are handled without
damon_callback hooks, so damon_sysfs_cmd_request use code in
damon_sysfs_andle_cmd() does nothing in real. Remove the unnecessary
code.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/sysfs.c | 32 --------------------------------
1 file changed, 32 deletions(-)
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index c55a2cee4b74..166161f12c26 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1715,8 +1715,6 @@ static int damon_sysfs_update_schemes_tried_regions(
static int damon_sysfs_handle_cmd(enum damon_sysfs_cmd cmd,
struct damon_sysfs_kdamond *kdamond)
{
- bool need_wait = true;
-
switch (cmd) {
case DAMON_SYSFS_CMD_ON:
return damon_sysfs_turn_damon_on(kdamond);
@@ -1747,38 +1745,8 @@ static int damon_sysfs_handle_cmd(enum damon_sysfs_cmd cmd,
return damon_sysfs_damon_call(
damon_sysfs_upd_tuned_intervals, kdamond);
default:
- break;
- }
-
- /* Pass the command to DAMON callback for safe DAMON context access */
- if (damon_sysfs_cmd_request.kdamond)
- return -EBUSY;
- if (!damon_sysfs_kdamond_running(kdamond))
return -EINVAL;
- damon_sysfs_cmd_request.cmd = cmd;
- damon_sysfs_cmd_request.kdamond = kdamond;
-
- /*
- * wait until damon_sysfs_cmd_request_callback() handles the request
- * from kdamond context
- */
- mutex_unlock(&damon_sysfs_lock);
- while (need_wait) {
- schedule_timeout_idle(msecs_to_jiffies(100));
- if (!mutex_trylock(&damon_sysfs_lock))
- continue;
- if (!damon_sysfs_cmd_request.kdamond) {
- /* damon_sysfs_cmd_request_callback() handled */
- need_wait = false;
- } else if (!damon_sysfs_kdamond_running(kdamond)) {
- /* kdamond has already finished */
- need_wait = false;
- damon_sysfs_cmd_request.kdamond = NULL;
- }
- mutex_unlock(&damon_sysfs_lock);
}
- mutex_lock(&damon_sysfs_lock);
- return 0;
}
static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 06/13] mm/damon/sysfs: remove damon_sysfs_cmd_request_callback() and its callers
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (4 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 05/13] mm/damon/sysfs: remove damon_sysfs_cmd_request code from damon_sysfs_handle_cmd() SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 07/13] mm/damon/sysfs: remove damon_sysfs_cmd_request and its readers SeongJae Park
` (6 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
damon_sysfs_cmd_request_callback() is the damon_callback hook functions
that were used to handle user requests that need to read and/or write
DAMON internal data. All the usages are now updated to use damon_call()
or damos_walk(), though. Remove it and its callers.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/sysfs.c | 62 ------------------------------------------------
1 file changed, 62 deletions(-)
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 166161f12c26..e5bcf019086f 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1529,65 +1529,6 @@ static int damon_sysfs_upd_tuned_intervals(void *data)
return 0;
}
-/*
- * damon_sysfs_cmd_request_callback() - DAMON callback for handling requests.
- * @c: The DAMON context of the callback.
- * @active: Whether @c is not deactivated due to watermarks.
- * @after_aggr: Whether this is called from after_aggregation() callback.
- *
- * This function is periodically called back from the kdamond thread for @c.
- * Then, it checks if there is a waiting DAMON sysfs request and handles it.
- */
-static int damon_sysfs_cmd_request_callback(struct damon_ctx *c, bool active,
- bool after_aggregation)
-{
- struct damon_sysfs_kdamond *kdamond;
- int err = 0;
-
- /* avoid deadlock due to concurrent state_store('off') */
- if (!mutex_trylock(&damon_sysfs_lock))
- return 0;
- kdamond = damon_sysfs_cmd_request.kdamond;
- if (!kdamond || kdamond->damon_ctx != c)
- goto out;
- switch (damon_sysfs_cmd_request.cmd) {
- default:
- break;
- }
- /* Mark the request as invalid now. */
- damon_sysfs_cmd_request.kdamond = NULL;
-out:
- mutex_unlock(&damon_sysfs_lock);
- return err;
-}
-
-static int damon_sysfs_after_wmarks_check(struct damon_ctx *c)
-{
- /*
- * after_wmarks_check() is called back while the context is deactivated
- * by watermarks.
- */
- return damon_sysfs_cmd_request_callback(c, false, false);
-}
-
-static int damon_sysfs_after_sampling(struct damon_ctx *c)
-{
- /*
- * after_sampling() is called back only while the context is not
- * deactivated by watermarks.
- */
- return damon_sysfs_cmd_request_callback(c, true, false);
-}
-
-static int damon_sysfs_after_aggregation(struct damon_ctx *c)
-{
- /*
- * after_aggregation() is called back only while the context is not
- * deactivated by watermarks.
- */
- return damon_sysfs_cmd_request_callback(c, true, true);
-}
-
static struct damon_ctx *damon_sysfs_build_ctx(
struct damon_sysfs_context *sys_ctx)
{
@@ -1603,9 +1544,6 @@ static struct damon_ctx *damon_sysfs_build_ctx(
return ERR_PTR(err);
}
- ctx->callback.after_wmarks_check = damon_sysfs_after_wmarks_check;
- ctx->callback.after_sampling = damon_sysfs_after_sampling;
- ctx->callback.after_aggregation = damon_sysfs_after_aggregation;
ctx->callback.before_terminate = damon_sysfs_before_terminate;
return ctx;
}
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 07/13] mm/damon/sysfs: remove damon_sysfs_cmd_request and its readers
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (5 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 06/13] mm/damon/sysfs: remove damon_sysfs_cmd_request_callback() and its callers SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 08/13] mm/damon/sysfs-schemes: remove obsolete comment for damon_sysfs_schemes_clear_regions() SeongJae Park
` (5 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
damon_sysfs_cmd_request is DAMON sysfs interface's own synchronization
mechanism for accessing DAMON internal data via damon_callback hooks.
All the users are now migrated to damon_call() and damos_walk(), so
nobody really uses it. No one writes to the data structure but reading
code is remained. Remove the reading code and the entire data
structure.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/sysfs.c | 24 +-----------------------
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index e5bcf019086f..1af6aff35d84 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1238,25 +1238,6 @@ static const char * const damon_sysfs_cmd_strs[] = {
"update_tuned_intervals",
};
-/*
- * struct damon_sysfs_cmd_request - A request to the DAMON callback.
- * @cmd: The command that needs to be handled by the callback.
- * @kdamond: The kobject wrapper that associated to the kdamond thread.
- *
- * This structure represents a sysfs command request that need to access some
- * DAMON context-internal data. Because DAMON context-internal data can be
- * safely accessed from DAMON callbacks without additional synchronization, the
- * request will be handled by the DAMON callback. None-``NULL`` @kdamond means
- * the request is valid.
- */
-struct damon_sysfs_cmd_request {
- enum damon_sysfs_cmd cmd;
- struct damon_sysfs_kdamond *kdamond;
-};
-
-/* Current DAMON callback request. Protected by damon_sysfs_lock. */
-static struct damon_sysfs_cmd_request damon_sysfs_cmd_request;
-
static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
{
@@ -1555,8 +1536,6 @@ static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
if (damon_sysfs_kdamond_running(kdamond))
return -EBUSY;
- if (damon_sysfs_cmd_request.kdamond == kdamond)
- return -EBUSY;
/* TODO: support multiple contexts per kdamond */
if (kdamond->contexts->nr != 1)
return -EINVAL;
@@ -1796,8 +1775,7 @@ static bool damon_sysfs_kdamonds_busy(struct damon_sysfs_kdamond **kdamonds,
int i;
for (i = 0; i < nr_kdamonds; i++) {
- if (damon_sysfs_kdamond_running(kdamonds[i]) ||
- damon_sysfs_cmd_request.kdamond == kdamonds[i])
+ if (damon_sysfs_kdamond_running(kdamonds[i]))
return true;
}
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 08/13] mm/damon/sysfs-schemes: remove obsolete comment for damon_sysfs_schemes_clear_regions()
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (6 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 07/13] mm/damon/sysfs: remove damon_sysfs_cmd_request and its readers SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 09/13] mm/damon: remove damon_callback->private SeongJae Park
` (4 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
The comment on damon_sysfs_schemes_clear_regions() function is obsolete,
since it has updated to directly called from DAMON sysfs interface code.
Remove the outdated comment.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/sysfs-schemes.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index ed834622df2a..d50f37ef4eb8 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -2341,7 +2341,6 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
}
}
-/* Called from damon_sysfs_cmd_request_callback under damon_sysfs_lock */
int damon_sysfs_schemes_clear_regions(
struct damon_sysfs_schemes *sysfs_schemes)
{
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 09/13] mm/damon: remove damon_callback->private
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (7 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 08/13] mm/damon/sysfs-schemes: remove obsolete comment for damon_sysfs_schemes_clear_regions() SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 10/13] mm/damon: remove ->before_start of damon_callback SeongJae Park
` (3 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
The field was added to let users keep their personal data to use inside
of the callbacks. However, no one is actively using that now. Remove
it.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
include/linux/damon.h | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 636dcdc49b22..2b783c91cbb5 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -604,12 +604,10 @@ struct damon_operations {
* @after_aggregation: Called after each aggregation.
* @before_damos_apply: Called before applying DAMOS action.
* @before_terminate: Called before terminating the monitoring.
- * @private: User private data.
*
* The monitoring thread (&damon_ctx.kdamond) calls @before_start and
* @before_terminate just before starting and finishing the monitoring,
- * respectively. Therefore, those are good places for installing and cleaning
- * @private.
+ * respectively.
*
* The monitoring thread calls @after_wmarks_check after each DAMON-based
* operation schemes' watermarks check. If users need to make changes to the
@@ -625,8 +623,6 @@ struct damon_operations {
* If any callback returns non-zero, monitoring stops.
*/
struct damon_callback {
- void *private;
-
int (*before_start)(struct damon_ctx *context);
int (*after_wmarks_check)(struct damon_ctx *context);
int (*after_sampling)(struct damon_ctx *context);
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 10/13] mm/damon: remove ->before_start of damon_callback
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (8 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 09/13] mm/damon: remove damon_callback->private SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 11/13] mm/damon: remove damon_callback->after_sampling SeongJae Park
` (2 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
The function pointer field was added to be used as a place to do some
initialization works just before DAMON starts working. However, nobody
is using it now. Remove it.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
include/linux/damon.h | 7 ++-----
mm/damon/core.c | 2 --
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 2b783c91cbb5..11cd916b41fb 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -598,16 +598,14 @@ struct damon_operations {
/**
* struct damon_callback - Monitoring events notification callbacks.
*
- * @before_start: Called before starting the monitoring.
* @after_wmarks_check: Called after each schemes' watermarks check.
* @after_sampling: Called after each sampling.
* @after_aggregation: Called after each aggregation.
* @before_damos_apply: Called before applying DAMOS action.
* @before_terminate: Called before terminating the monitoring.
*
- * The monitoring thread (&damon_ctx.kdamond) calls @before_start and
- * @before_terminate just before starting and finishing the monitoring,
- * respectively.
+ * The monitoring thread (&damon_ctx.kdamond) calls @before_terminate just
+ * before finishing the monitoring.
*
* The monitoring thread calls @after_wmarks_check after each DAMON-based
* operation schemes' watermarks check. If users need to make changes to the
@@ -623,7 +621,6 @@ struct damon_operations {
* If any callback returns non-zero, monitoring stops.
*/
struct damon_callback {
- int (*before_start)(struct damon_ctx *context);
int (*after_wmarks_check)(struct damon_ctx *context);
int (*after_sampling)(struct damon_ctx *context);
int (*after_aggregation)(struct damon_ctx *context);
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 5b807caaec95..8efb249be855 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2399,8 +2399,6 @@ static int kdamond_fn(void *data)
if (ctx->ops.init)
ctx->ops.init(ctx);
- if (ctx->callback.before_start && ctx->callback.before_start(ctx))
- goto done;
ctx->regions_score_histogram = kmalloc_array(DAMOS_MAX_SCORE + 1,
sizeof(*ctx->regions_score_histogram), GFP_KERNEL);
if (!ctx->regions_score_histogram)
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 11/13] mm/damon: remove damon_callback->after_sampling
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (9 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 10/13] mm/damon: remove ->before_start of damon_callback SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 12/13] mm/damon: remove damon_callback->before_damos_apply SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 13/13] mm/damon: remove damon_operations->reset_aggregated SeongJae Park
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
The callback was used by DAMON sysfs interface for reading DAMON
internal data. But it is no more being used, and damon_call() can do
similar works in a better way. Remove it.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
include/linux/damon.h | 11 ++++-------
mm/damon/core.c | 3 ---
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 11cd916b41fb..c9abacf16d88 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -599,7 +599,6 @@ struct damon_operations {
* struct damon_callback - Monitoring events notification callbacks.
*
* @after_wmarks_check: Called after each schemes' watermarks check.
- * @after_sampling: Called after each sampling.
* @after_aggregation: Called after each aggregation.
* @before_damos_apply: Called before applying DAMOS action.
* @before_terminate: Called before terminating the monitoring.
@@ -612,17 +611,15 @@ struct damon_operations {
* attributes of the monitoring context while it's deactivated due to the
* watermarks, this is the good place to do.
*
- * The monitoring thread calls @after_sampling and @after_aggregation for each
- * of the sampling intervals and aggregation intervals, respectively.
- * Therefore, users can safely access the monitoring results without additional
- * protection. For the reason, users are recommended to use these callback for
- * the accesses to the results.
+ * The monitoring thread calls @after_aggregation for each of the aggregation
+ * intervals. Therefore, users can safely access the monitoring results
+ * without additional protection. For the reason, users are recommended to use
+ * these callback for the accesses to the results.
*
* If any callback returns non-zero, monitoring stops.
*/
struct damon_callback {
int (*after_wmarks_check)(struct damon_ctx *context);
- int (*after_sampling)(struct damon_ctx *context);
int (*after_aggregation)(struct damon_ctx *context);
int (*before_damos_apply)(struct damon_ctx *context,
struct damon_target *target,
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 8efb249be855..14e4122464a1 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2423,9 +2423,6 @@ static int kdamond_fn(void *data)
if (ctx->ops.prepare_access_checks)
ctx->ops.prepare_access_checks(ctx);
- if (ctx->callback.after_sampling &&
- ctx->callback.after_sampling(ctx))
- break;
kdamond_usleep(sample_interval);
ctx->passed_sample_intervals++;
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 12/13] mm/damon: remove damon_callback->before_damos_apply
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (10 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 11/13] mm/damon: remove damon_callback->after_sampling SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
2025-02-26 6:36 ` [RFC PATCH 13/13] mm/damon: remove damon_operations->reset_aggregated SeongJae Park
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
The hook was introduced to let DAMON kernel API users access DAMOS
schemes-eligible regions in a safe way. Now it is no more used by
anyone, and the functionality is provided in a better way by
damos_walk(). Remove it.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
include/linux/damon.h | 5 -----
mm/damon/core.c | 13 ++++---------
2 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index c9abacf16d88..2808ea07e1cc 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -600,7 +600,6 @@ struct damon_operations {
*
* @after_wmarks_check: Called after each schemes' watermarks check.
* @after_aggregation: Called after each aggregation.
- * @before_damos_apply: Called before applying DAMOS action.
* @before_terminate: Called before terminating the monitoring.
*
* The monitoring thread (&damon_ctx.kdamond) calls @before_terminate just
@@ -621,10 +620,6 @@ struct damon_operations {
struct damon_callback {
int (*after_wmarks_check)(struct damon_ctx *context);
int (*after_aggregation)(struct damon_ctx *context);
- int (*before_damos_apply)(struct damon_ctx *context,
- struct damon_target *target,
- struct damon_region *region,
- struct damos *scheme);
void (*before_terminate)(struct damon_ctx *context);
};
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 14e4122464a1..22f90666fe16 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1723,7 +1723,6 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
struct timespec64 begin, end;
unsigned long sz_applied = 0;
unsigned long sz_ops_filter_passed = 0;
- int err = 0;
/*
* We plan to support multiple context per kdamond, as DAMON sysfs
* implies with 'nr_contexts' file. Nevertheless, only single context
@@ -1763,14 +1762,10 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
if (damos_filter_out(c, t, r, s))
return;
ktime_get_coarse_ts64(&begin);
- if (c->callback.before_damos_apply)
- err = c->callback.before_damos_apply(c, t, r, s);
- if (!err) {
- trace_damos_before_apply(cidx, sidx, tidx, r,
- damon_nr_regions(t), do_trace);
- sz_applied = c->ops.apply_scheme(c, t, r, s,
- &sz_ops_filter_passed);
- }
+ trace_damos_before_apply(cidx, sidx, tidx, r,
+ damon_nr_regions(t), do_trace);
+ sz_applied = c->ops.apply_scheme(c, t, r, s,
+ &sz_ops_filter_passed);
damos_walk_call_walk(c, t, r, s, sz_ops_filter_passed);
ktime_get_coarse_ts64(&end);
quota->total_charged_ns += timespec64_to_ns(&end) -
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH 13/13] mm/damon: remove damon_operations->reset_aggregated
2025-02-26 6:36 [RFC PATCH 00/13] mm/damon/sysfs: commit parameters online via damon_call() SeongJae Park
` (11 preceding siblings ...)
2025-02-26 6:36 ` [RFC PATCH 12/13] mm/damon: remove damon_callback->before_damos_apply SeongJae Park
@ 2025-02-26 6:36 ` SeongJae Park
12 siblings, 0 replies; 15+ messages in thread
From: SeongJae Park @ 2025-02-26 6:36 UTC (permalink / raw)
Cc: SeongJae Park, Andrew Morton, damon, kernel-team, linux-kernel, linux-mm
The operations layer hook was introduced to let operations set do any
aggregation data reset if needed. But it is not really be used now.
Remove it.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
include/linux/damon.h | 7 +------
mm/damon/core.c | 2 --
mm/damon/paddr.c | 1 -
mm/damon/vaddr.c | 1 -
4 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 2808ea07e1cc..957c7f3af80f 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -537,7 +537,6 @@ enum damon_ops_id {
* @update: Update operations-related data structures.
* @prepare_access_checks: Prepare next access check of target regions.
* @check_accesses: Check the accesses to target regions.
- * @reset_aggregated: Reset aggregated accesses monitoring results.
* @get_scheme_score: Get the score of a region for a scheme.
* @apply_scheme: Apply a DAMON-based operation scheme.
* @target_valid: Determine if the target is valid.
@@ -549,8 +548,7 @@ enum damon_ops_id {
* (&damon_ctx.kdamond) calls @init and @prepare_access_checks before starting
* the monitoring, @update after each &damon_attrs.ops_update_interval, and
* @check_accesses, @target_valid and @prepare_access_checks after each
- * &damon_attrs.sample_interval. Finally, @reset_aggregated is called after
- * each &damon_attrs.aggr_interval.
+ * &damon_attrs.sample_interval.
*
* Each &struct damon_operations instance having valid @id can be registered
* via damon_register_ops() and selected by damon_select_ops() later.
@@ -565,8 +563,6 @@ enum damon_ops_id {
* last preparation and update the number of observed accesses of each region.
* It should also return max number of observed accesses that made as a result
* of its update. The value will be used for regions adjustment threshold.
- * @reset_aggregated should reset the access monitoring results that aggregated
- * by @check_accesses.
* @get_scheme_score should return the priority score of a region for a scheme
* as an integer in [0, &DAMOS_MAX_SCORE].
* @apply_scheme is called from @kdamond when a region for user provided
@@ -584,7 +580,6 @@ struct damon_operations {
void (*update)(struct damon_ctx *context);
void (*prepare_access_checks)(struct damon_ctx *context);
unsigned int (*check_accesses)(struct damon_ctx *context);
- void (*reset_aggregated)(struct damon_ctx *context);
int (*get_scheme_score)(struct damon_ctx *context,
struct damon_target *t, struct damon_region *r,
struct damos *scheme);
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 22f90666fe16..6f821a579257 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2463,8 +2463,6 @@ static int kdamond_fn(void *data)
kdamond_reset_aggregated(ctx);
kdamond_split_regions(ctx);
- if (ctx->ops.reset_aggregated)
- ctx->ops.reset_aggregated(ctx);
}
if (ctx->passed_sample_intervals >= next_ops_update_sis) {
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index fee66a3cc82b..e5b532340102 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -618,7 +618,6 @@ static int __init damon_pa_initcall(void)
.update = NULL,
.prepare_access_checks = damon_pa_prepare_access_checks,
.check_accesses = damon_pa_check_accesses,
- .reset_aggregated = NULL,
.target_valid = NULL,
.cleanup = NULL,
.apply_scheme = damon_pa_apply_scheme,
diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index a6174f725bd7..e6d99106a7f9 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -710,7 +710,6 @@ static int __init damon_va_initcall(void)
.update = damon_va_update,
.prepare_access_checks = damon_va_prepare_access_checks,
.check_accesses = damon_va_check_accesses,
- .reset_aggregated = NULL,
.target_valid = damon_va_target_valid,
.cleanup = NULL,
.apply_scheme = damon_va_apply_scheme,
--
2.39.5
^ permalink raw reply [flat|nested] 15+ messages in thread