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

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] [day] [month] [year] [list]

Message-Id: <20250806214237.51484-1-sj@kernel.org>
Date: Wed,  6 Aug 2025 14:42:37 -0700
From: SeongJae Park <sj@...nel.org>
To: Bijan Tabatabai <bijan311@...il.com>
Cc: SeongJae Park <sj@...nel.org>,
	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] mm/damon/core: skip needless update of next_{aggregation,ops_update}_sis

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

> On Wed, Aug 6, 2025 at 2:09 PM SeongJae Park <sj@...nel.org> wrote:
> >
> > Hi Bijan,
> >
> > On Wed,  6 Aug 2025 11:43:16 -0500 Bijan Tabatabai <bijan311@...il.com> wrote:
> >
> > > From: Bijan Tabatabai <bijantabatab@...ron.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

[...]