Re: (sashiko review) [RFC PATCH v5.1 02/11] mm/damon/core: merge regions after applying DAMOS schemes

[SeongJae Park]

TL; DR: sashiko found a typo and a fact that I forgot adding a fixup that I
promised on the previous version.  I will fix those in the next spin.

Forwarding full sashiko review in a reply format with my 
inline comments below, for sharing details of my view and
doing followup discussions via mails if needed.

> # review url: https://sashiko.dev/#/patchset/20260411164908.77189-3-sj@xxxxxxxxxx
> # start of sashiko.dev inline review
> commit 1a56d6af604a3fc86a23180a9619447641294a2f
> Author: SeongJae Park <sj@xxxxxxxxxx>
> 
> mm/damon/core: merge regions after applying DAMOS schemes
> 
> This commit introduces a mechanism to merge regions after applying schemes
> to avoid overhead from regions split during quota-limited scheme application.
> 
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > +static void damos_apply_target(struct damon_ctx *c, struct damon_target *t)
> > +{
> > +	struct damon_region *r;
> > +
> > +	damon_for_each_region(r, t) {
> 
> Is this list iteration pattern safe for long-term maintenance? 
> 
> This loop uses damon_for_each_region(), which is an unsafe list iterator,
> but destroys r inside the loop and reassigns r = prev_r to safely continue.
> 
> While this works by relying on internal macro expansion mechanics, if a future
> developer converts this to use damon_for_each_region_safe(), the cached
> next pointer would be evaluated before damon_do_apply_schemes() potentially
> splits the region.
> 
> Could this cause the loop to silently skip the newly inserted slices entirely?

Yes, such mistake could happen in future, and this code could be better written
for long term maitenance.  But that seems better to be revisited as a future
cleanup work.

> 
> > +		struct damon_region *prev_r;
> > +
> > +		damon_do_apply_schemes(c, t, r);
> > +		/*
> > +		 * damon_do_apply_scheems() could split the region for the
> 
> This isn't a bug, but there is a typo in the comment (scheems).

Good eyes!  I will fix this in the next spin.

> 
> > +		 * quota.  Keeping the new slices is an overhead.  Merge back
> > +		 * the slices into the previous region if it doesn't lose any
> > +		 * information.
> > +		 */
> > +		if (damon_first_region(t) == r)
> > +			continue;
> > +		prev_r = damon_prev_region(r);
> > +		if (prev_r->ar.end != r->ar.start)
> > +			continue;
> > +		if (prev_r->age != r->age)
> > +			continue;
> > +		if (prev_r->last_nr_accesses != r->last_nr_accesses)
> > +			continue;
> > +		if (prev_r->nr_accesses != r->nr_accesses)
> > +			continue;
> > +		prev_r->ar.end = r->ar.end;
> > +		damon_destroy_region(r, t);
> > +		r = prev_r;
> > +	}
> > +}
> 
> The commit message explicitly states the merge operation is made only if it
> keeps the min_nr_regions constraint. Does this inline merge logic bypass
> that constraint?

Argh, I forgot adding the fixup [1] to this patch.  I will add that in the next
version.

> 
> Earlier in the execution, damon_apply_min_nr_regions() enforces the limit by
> calculating a size limit and splitting any region larger than this limit into
> smaller sub-regions. These newly created sub-regions naturally inherit
> identical age and nr_accesses values from their parent.
> 
> When kdamond_apply_schemes() subsequently calls damos_apply_target(), this
> logic will immediately merge those freshly split regions back together
> because their age and nr_accesses match, entirely omitting the size limit
> check. 
> 
> Does this unintentionally destroy the spatial monitoring granularity every
> sample interval?
> 
> Additionally, when merging the regions, this code expands prev_r->ar.end and
> destroys r without accounting for r->nr_accesses_bp.
> 
> Since standard functions like damon_merge_two_regions() re-average metrics
> including nr_accesses_bp, could this inline discard of the fractional
> nr_accesses_bp tracking result in a slight precision loss, contrary to the
> commit message's goal of not losing monitoring information?

Yes, that's valid.
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260411164908.77189-3-sj@xxxxxxxxxx

[1] https://lore.kernel.org/20260411163340.46397-1-sj@xxxxxxxxxx


Thanks,
SJ