Re: (sashiko review) [PATCH 6.18.y] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

[SJ Park <sj@kernel.org>]

TL;DR: This patch is wrong, please ignore.  I will send v2.

# sashiko review suggestions
#
# 1. Consider reducing recipients.  Maybe the author,
#    maintainers, reviewers, and mailing list of the
#    direct subsystem and parent susystem mailing lists
#    could be a starting point.
# 2. Add short summary of your opinion at the beginning.
#    For example:
#
#      TL;DR: sashiko found an issue.  I will respin.
#      TL;DR: sashiko found no issue on this patch.
#
# Please DON'T FORGET removing this comment block before
# sending this!

Somehow Sashiko stopped sending review as reply, so manually sharing it.

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/20261003163101.32739-1-sj@kernel.org
> # start of sashiko.dev inline review
> commit 21f2d44a9b003b2e8d2502cbeb64ca223ff26e08
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: do non-safe region walk on kdamond_apply_schemes()
> 
> The commit replaces the safe region iterator damon_for_each_region_safe()
> with damon_for_each_region() in kdamond_apply_schemes(). It updates
> damos_skip_charged_region() to return true when a region is split, allowing
> the new fragment to be processed in the next iteration.
> 
> Link: https://lkml.kernel.org/r/20260227170623.95384-3-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index c0dcb190b06e3..8217815de8982 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1770,9 +1772,7 @@ static bool damos_skip_charged_region(struct damon_target *t,
> >  				sz_to_skip = min_sz_region;
> >  			}
> >  			damon_split_region_at(t, r, sz_to_skip);
> > -			r = damon_next_region(r);
> > -			*rp = r;
> > -			skip = false;
> > +			return true;
> >  		}
> >  	}
> >  out:
> 
> [Severity: High]
> Does this early return bypass the cleanup block at the out label?
> 
> If damon_split_region_at() fails on the final region due to memory
> pressure, returning true here skips the out block that clears
> quota->charge_target_from.
> 
> Could this leaked cursor cause the scheme to permanently skip all
> subsequent memory targets in this and future evaluation intervals until
> an allocation succeeds?

Correct.  It is my mistake during porting.  I will fix this in the next
version.

> 
> [ ... ]
> 
> > @@ -2329,7 +2329,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
> >  
> >  	mutex_lock(&c->walk_control_lock);
> >  	damon_for_each_target(t, c) {
> > -		damon_for_each_region_safe(r, next_r, t)
> > +		damon_for_each_region(r, t)
> >  			damon_do_apply_schemes(c, t, r);
> >  	}
> >  
> 
> [Severity: High]
> Can changing this to a dynamic list iterator lead to regions being
> processed multiple times?
> 
> If a DAMON configuration has multiple schemes, and a later scheme splits
> a region, the newly inserted right-hand fragment will be visited by this
> outer loop in the next iteration.
> 
> Since damon_do_apply_schemes() evaluates all schemes sequentially,
> wouldn't schemes that already processed the original uncut region
> process the new fragment again, leading to duplicate memory operations
> and double-counting in quota tracking?

Good finding.  The bug exist in mainline too, and doesn't cause critical crash
or corruption.  Hence I don't think this is a blocker of this patch.  I will
separately work on it.  Please let me know if anyone has a different opinion.


> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20261003163101.32739-1-sj@kernel.org

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


Thanks,
SJ