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

SeongJae Park
2026-04-10T00:02:02Z

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@kernel.org
> # start of sashiko.dev inline review
> commit e2cfd0aa5e77f600726ed8baabf9eb6c27b3546d
> Author: SeongJae Park <sj@kernel.org>
>
> 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@kernel.org

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@kernel.org
#
# [1] https://github.com/sjp38/hackermail