1. Patchew
  2. linux
  3. View series

[PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit_ctx()

Bijan Tabatabai posted 1 patch 1 month, 4 weeks ago 
mm/damon/core.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
[PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit_ctx()
Posted by Bijan Tabatabai 1 month, 4 weeks ago 
From: Bijan Tabatabai <bijantabatab@micron.com>

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 <bijan311@gmail.com>
Signed-off-by: Bijan Tabatabai <bijantabatab@micron.com>
---
Changes from v1[1]:
  - Compare entirety of struct damon_attrs
  - Apply logic in damon_commit_ctx() instead of damon_set_attrs()

[1] https://lore.kernel.org/all/20250806164316.5728-1-bijan311@gmail.com/
---
 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;
+
+}
+
 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
Re: [PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit_ctx()
Posted by SeongJae Park 1 month, 4 weeks ago 
On Wed,  6 Aug 2025 18:42:54 -0500 Bijan Tabatabai <bijan311@gmail.com> wrote:

> From: Bijan Tabatabai <bijantabatab@micron.com>
> 
> 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 <bijan311@gmail.com>

Maybe above line is added by a mistake?

> Signed-off-by: Bijan Tabatabai <bijantabatab@micron.com>

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!

> 
> [1] https://lore.kernel.org/all/20250806164316.5728-1-bijan311@gmail.com/
> ---
>  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?

> +}
> +
>  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
Re: [PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit_ctx()
Posted by Bijan Tabatabai 1 month, 4 weeks ago 
On Wed, Aug 6, 2025 at 7:19 PM SeongJae Park <sj@kernel.org> wrote:
>
> On Wed,  6 Aug 2025 18:42:54 -0500 Bijan Tabatabai <bijan311@gmail.com> wrote:
>
> > From: Bijan Tabatabai <bijantabatab@micron.com>
> >
> > 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 <bijan311@gmail.com>
>
> Maybe above line is added by a mistake?

Sorry about that. I added it because my internship ends this week and
wanted to make sure I get notifications on the status of this patch
(e.g. email notifications when the patch is merged in Andrew's tree).
If it's inappropriate I will remove it in the next version (unless
Andrew does it himself).

> > Signed-off-by: Bijan Tabatabai <bijantabatab@micron.com>
>
> 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!
>
> >
> > [1] https://lore.kernel.org/all/20250806164316.5728-1-bijan311@gmail.com/
> > ---
> >  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?

Sorry for missing this!

> > +}
> > +
> >  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!

Thank you!
Bijan

[...]
Re: [PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit_ctx()
Posted by SeongJae Park 1 month, 4 weeks ago 
On Wed, 6 Aug 2025 19:48:44 -0500 Bijan Tabatabai <bijan311@gmail.com> wrote:

> On Wed, Aug 6, 2025 at 7:19 PM SeongJae Park <sj@kernel.org> wrote:
> >
> > On Wed,  6 Aug 2025 18:42:54 -0500 Bijan Tabatabai <bijan311@gmail.com> wrote:
> >
> > > From: Bijan Tabatabai <bijantabatab@micron.com>
> > >
> > > 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 <bijan311@gmail.com>
> >
> > Maybe above line is added by a mistake?
> 
> Sorry about that. I added it because my internship ends this week and
> wanted to make sure I get notifications on the status of this patch
> (e.g. email notifications when the patch is merged in Andrew's tree).
> If it's inappropriate I will remove it in the next version (unless
> Andrew does it himself).

I was just thinking you might not added this for a purpose.  I don't mind
adding this Cc tag.  As you may already noticed, Andrew merged this patch as is
with this Cc line, so seems Andrew also doesn't really mind, so all is good.

I hope you had a nice internship and continue seeing on the mailing lists :)

[...]
> > > +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?
> 
> Sorry for missing this!

No worry, as you may already noticed, Andrew thankfully fixed this, with a
followup fix patch[1].

So all is well :)

[...]

[1] https://lore.kernel.org/20250807033443.BD30FC4CEEB@smtp.kernel.org


Thanks,
SJ

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.