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 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