Re: [PATCH] mm/damon/core: skip needless update of next_{aggregation,ops_update}_sis

[SeongJae Park <sj@kernel.org>]

On Wed, 6 Aug 2025 14:49:21 -0500 Bijan Tabatabai <bijan311@gmail.com> wrote:

> On Wed, Aug 6, 2025 at 2:09 PM SeongJae Park <sj@kernel.org> wrote:
> >
> > Hi Bijan,
> >
> > On Wed,  6 Aug 2025 11:43:16 -0500 Bijan Tabatabai <bijan311@gmail.com> wrote:
> >
> > > From: Bijan Tabatabai <bijantabatab@micron.com>
> > >
> > > In damon_set_attrs(), ctx->next_{aggregation,ops_update}_sis would be
> > > reset, even if the sample interval, aggregation interval, or ops update
> > > interval were not changed. If damon_set_attrs() is called relatively
> > > frequently, such as by frequent "commit" operations, aggregation and ops
> > > update operations could be needlessly delayed.
> > >
> > > This patch avoids this by only updating next_{aggregation,ops_update}_sis
> > > if the relevant intervals were changed.
[...]
> > What about modifying damon_commit_ctx() to check if new and old
> > damon_ctx->attrs are entirely same, and skip calling damon_set_attrs() in the
> > case?  Doing the entire damon_attrs comparison might be suboptimum, but would
> > make the change simpler.  I assume the suboptimum comparison is not a real
> > problem for your use case, so I think that could be a good tradeoff?
> 
> I can definitely do this. Checking a few extra fields is no big deal.
> 
> Silly question, but think it's best to get it out of the way before
> sending another patch: do you think there's a more elegant way of just
> having a dumb comparison function like
> 
> bool damon_attrs_equal(struct damon_attrs *a, struct damon_attrs *b)
> {
>         return a->sample_interval == b->sample_interval &&
>             a->aggr_interval == b->aggr_interval &&
>             ...
> }
> 
> And I assume I shouldn't compare the aggr_samples field because it's
> private, is that right?

Ah, you're right, thank you for asking this!

Maybe we can copy src->attrs to a local damon_attrs variable, overwrite
aggr_samples of the copy and dst, and memcmp() the copy and dst?

I wouldn't mind the dumb comparison function, though, if you prefer.  I'll
defer the decision to you!


Thanks,
SJ

[...]