Re: (sashiko review) [RFC PATCH v4 02/11] mm/damon/core: merge quota-sliced regions back

[SeongJae Park]

TL; DR: sashiko found clear issues.  I will fix those in the next revision.

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/20260409142148.60652-3-sj@xxxxxxxxxx
> # start of sashiko.dev inline review
> commit e2cfd0aa5e77f600726ed8baabf9eb6c27b3546d
> Author: SeongJae Park <sj@xxxxxxxxxx>
> 
> mm/damon/core: merge quota-sliced regions back
> 
> This commit modifies the DAMON core to merge back regions that were sliced
> due to quota limits during scheme application. This aims to prevent the total
> number of regions from exceeding the user-defined max_nr_regions limit.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2158,6 +2158,33 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
> [ ... ]
> > +static void damos_apply_target(struct damon_ctx *c, struct damon_target *t)
> > +{
> > +	struct damon_region *r, *next, *orig_region = NULL;
> > +	unsigned long orig_end_addr;
> > +
> > +	damon_for_each_region_safe(r, next, t) {
> 
> Since damon_for_each_region_safe uses list_for_each_entry_safe, it will cache
> the next element before executing the loop body. If damon_do_apply_schemes
> splits the region r by inserting a new slice immediately after r, won't the
> loop advance to the cached next element and skip evaluating the newly inserted
> slice?
> 
> If the newly inserted slice is bypassed, it looks like the merge-back logic
> below would become dead code for that slice, leaving the slice stranded in
> the list and defeating the patch's goal of avoiding max_nr_regions violations.

That's correct.  I will fix this on the next revision.

> 
> > +		/*
> > +		 * damon_do_apply_schemes() split the region if applying the
> > +		 * action to the whole region can make quota exceeded.  That
> > +		 * split can result in DAMON snapshot having more than
> > +		 * max_nr_regions regions.
> > +		 *
> > +		 * Merge back the sliced regions to the original region, as
> > +		 * soon as the schemes-handling of the slice is completed.
> > +		 */
> > +		if (!orig_region || orig_end_addr <= r->ar.start) {
> > +			orig_region = r;
> > +			orig_end_addr = r->ar.end;
> > +		}
> > +		damon_do_apply_schemes(c, t, r);
> > +		if (r == orig_region)
> > +			continue;
> > +		orig_region->ar.end = r->ar.end;
> > +		damon_destroy_region(r, t);
> 
> Does this merge-back logic permanently destroy the monitoring state of the
> unapplied portion of the region?
> 
> When damon_do_apply_schemes applies a scheme to the first part of a region,
> its state might be modified (for example, resetting its age). The second part
> of the region that was split off and skipped due to quota limits is supposed
> to retain its original state.
> 
> By destroying the unapplied slice and extending the applied region's end
> address, the unapplied slice's original state appears to be clobbered by the
> applied slice's modified state. Could this prevent the unapplied memory from
> meeting thresholds like min_age in subsequent intervals?

Again, good finding.  I will fix this in the next revision.

> 
> > +	}
> > +}
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260409142148.60652-3-sj@xxxxxxxxxx


Thanks,
SJ

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260409142148.60652-3-sj@xxxxxxxxxx
#
# [1] https://github.com/sjp38/hackermail