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

[SeongJae Park <sj@kernel.org>]

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