linux-kernel - Re: [PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMvvPS7mcevjD-b2vz1P+grQfffVA0bx-x5WcUQ8=JApD+UkHw@mail.gmail.com>
Date: Wed, 6 Aug 2025 19:48:44 -0500
From: Bijan Tabatabai <bijan311@...il.com>
To: SeongJae Park <sj@...nel.org>
Cc: damon@...ts.linux.dev, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Andrew Morton <akpm@...ux-foundation.org>, Bijan Tabatabai <bijantabatab@...ron.com>
Subject: Re: [PATCH v2] mm/damon/core: skip needless update of damon_attrs in damon_commit_ctx()

On Wed, Aug 6, 2025 at 7:19 PM SeongJae Park <sj@...nel.org> wrote:
>
> On Wed,  6 Aug 2025 18:42:54 -0500 Bijan Tabatabai <bijan311@...il.com> wrote:
>
> > From: Bijan Tabatabai <bijantabatab@...ron.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@...il.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@...ron.com>
>
> Reviewed-by: SeongJae Park <sj@...nel.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

[...]