Re: [PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit_ctx()
From: SeongJae Park <sj@kernel.org>
Date: 2025-08-07 00:19:27
Also in:
damon, linux-mm
On Wed, 6 Aug 2025 18:42:54 -0500 Bijan Tabatabai [off-list ref] wrote:
From: Bijan Tabatabai <redacted>
Currently, damon_commit_ctx() always calls damon_set_attrs() even if the
attributes have not been changed. This can be problematic when the DAMON
state is committed relatively frequently because damon_set_attrs() resets
ctx->next_{aggregation,ops_update}_sis, causing aggregation and ops
update operations to be needlessly delayed.
This patch avoids this by only calling damon_set_attrs() in
damon_commit_ctx when the attributes have been changed.
Cc: Bijan Tabatabai <redacted>Maybe above line is added by a mistake?
Signed-off-by: Bijan Tabatabai <redacted>
Reviewed-by: SeongJae Park <sj@kernel.org> I have a trivial comment below, though.
--- Changes from v1[1]: - Compare entirety of struct damon_attrs - Apply logic in damon_commit_ctx() instead of damon_set_attrs()
Thank you for doing this!
quoted hunk ↗ jump to hunk
[1] https://lore.kernel.org/all/20250806164316.5728-1-bijan311@gmail.com/ (local) --- mm/damon/core.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)diff --git a/mm/damon/core.c b/mm/damon/core.c index 6a2fe1f2c952..80aaeb876bf2 100644 --- a/mm/damon/core.c +++ b/mm/damon/core.c@@ -570,6 +570,24 @@ void damon_destroy_ctx(struct damon_ctx *ctx) kfree(ctx); } +static bool damon_attrs_equals(const struct damon_attrs *attrs1, + const struct damon_attrs *attrs2) +{ + const struct damon_intervals_goal *ig1 = &attrs1->intervals_goal; + const struct damon_intervals_goal *ig2 = &attrs2->intervals_goal; + + return attrs1->sample_interval == attrs2->sample_interval && + attrs1->aggr_interval == attrs2->aggr_interval && + attrs1->ops_update_interval == attrs2->ops_update_interval && + attrs1->min_nr_regions == attrs2->min_nr_regions && + attrs1->max_nr_regions == attrs2->max_nr_regions && + ig1->access_bp == ig2->access_bp && + ig1->aggrs == ig2->aggrs && + ig1->min_sample_us == ig2->min_sample_us && + ig1->max_sample_us == ig2->max_sample_us; +
Unnecessary blank line?
quoted hunk ↗ jump to hunk
+} + static unsigned int damon_age_for_new_attrs(unsigned int age, struct damon_attrs *old_attrs, struct damon_attrs *new_attrs) {@@ -1198,9 +1216,11 @@ int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src) * 2. ops update should be done after pid handling is done (target * committing require putting pids). */ - err = damon_set_attrs(dst, &src->attrs); - if (err) - return err; + if (!damon_attrs_equals(&dst->attrs, &src->attrs)) { + err = damon_set_attrs(dst, &src->attrs); + if (err) + return err; + } dst->ops = src->ops; return 0;-- 2.43.0
Other than the above trivial things, looks good to me. Thank you again! Andrew, I assume you would prefer resolving the above nits (removing unnecessary cc and blank line) on your own? Pleae let us know if not. :) Thanks, SJ