Re: (sashiko review) [RFC PATCH v4 03/11] mm/damon/core: introduce failed region quota charge ratio

[SeongJae Park <sj@kernel.org>]

# 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!

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-4-sj@kernel.org
> # start of sashiko.dev inline review
> commit 55afcaa519ab8046d8aa94af44973c3cff0c2a26
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: introduce failed region quota charge ratio
> 
> DAMOS quota is charged to all DAMOS action application attempted memory,
> regardless of how much of the memory the action was successful and
> failed.  This makes understanding quota behavior without DAMOS stat but
> only with end level metrics (e.g., increased amount of free memory for
> DAMOS_PAGEOUT action) difficult.  Also, charging action-failed memory
> same as action-successful memory is somewhat unfair, as successful
> action application will induce more overhead in most cases.
> 
> Introduce DAMON core API for setting the charge ratio for such
> action-failed memory.  It allows API callers to specify the ratio in a
> flexible way, by setting the numerator and the denominator.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 31ba739d28e2f..1f171665509b8 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -2041,6 +2043,23 @@ static void damos_walk_cancel(struct damon_ctx *ctx)
> >  	mutex_unlock(&ctx->walk_control_lock);
> >  }
> >  
> > +static void damos_charge_quota(struct damos_quota *quota,
> > +		unsigned long sz_region, unsigned long sz_applied)
> > +{
> > +	/*
> > +	 * sz_applied could be bigger than sz_region, depending on ops
> > +	 * implementation of the action, e.g., damos_pa_pageout().  Charge only
> > +	 * the region size in the case.
> > +	 */
> > +	if (!quota->fail_charge_denom || sz_applied > sz_region)
> > +		quota->charged_sz += sz_region;
> > +	else
> > +		quota->charged_sz += sz_applied + mult_frac(
> > +				(sz_region - sz_applied),
> > +				quota->fail_charge_num,
> > +				quota->fail_charge_denom);
> 
> Could this calculation lead to an integer overflow?
> 
> The mult_frac() macro evaluates the numerator multiplication internally
> using the type of the first argument, which is unsigned long. The sysfs
> interface in the subsequent patch ("mm/damon/sysfs-schemes: implement
> fail_charge_{num,denom} files") populates these values using kstrtouint,
> allowing inputs up to UINT_MAX.
> 
> If a user configures large values for fail_charge_num, could the
> multiplication silently overflow the 32-bit unsigned long result on 32-bit
> platforms, or on 64-bit platforms for sufficiently large memory regions?
> 
> This might produce a wrapped-around charged_sz and affect quota enforcement.

Sashiko asked same question in the previous version.  I decided to keep this as
is for code simplicity, as the issue is quite minor, unless other humans make
some voices.

> 
> > +}
> > +
> >  static bool damos_quota_is_full(struct damos_quota *quota,
> >  		unsigned long min_region_sz)
> >  {
> > @@ -2111,7 +2130,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> >  		ktime_get_coarse_ts64(&end);
> >  		quota->total_charged_ns += timespec64_to_ns(&end) -
> >  			timespec64_to_ns(&begin);
> > -		quota->charged_sz += sz;
> > +		damos_charge_quota(quota, sz, sz_applied);
> 
> Does discounting charged_sz for failed actions affect time quota enforcement?
> 
> DAMON converts the time quota (ms) into an effective size quota (esz) using
> throughput. If fail_charge_num is small or 0, the charged bytes accumulate
> very slowly, which could drive the throughput estimate to near zero.
> 
> This would cause esz to bottom out at min_region_sz. Since kdamond stops
> only when charged_sz >= esz, and charged_sz increases only by heavily
> discounted bytes, is it possible that charged_sz never reaches esz? This
> might cause kdamond to endlessly process the entire target memory space in a
> single cycle and consume unbounded CPU time.

Action failure is expected to be faster than successful action applying.  Hence
the denominator of the throughput, the total time took for the charging, should
also be reduced.  Users may set the ratio based on their expected overhead
difference between the time for failure and success of the actions.  So, this
is an intended behavior, not a bug.

> 
> Additionally, does the discounting affect region splitting earlier in
> damos_apply_scheme()?
> 
> The region is split using:
> 
>     sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz);
> 
> Here, esz and charged_sz track discounted bytes, but the resulting sz is
> passed to damon_split_region_at() to split the physical memory region.
> Could using a discounted byte limit as a physical byte size cause the action
> to be unnecessarily restricted to a tiny chunk, preventing the scheme from
> being applied to the full allowed physical area?

Yes.  And therefore the previous patch is made, though the current version of
the patch is completely wrong.

> 
> >  		if (damos_quota_is_full(quota, c->min_region_sz)) {
> >  			quota->charge_target_from = t;
> >  			quota->charge_addr_from = r->ar.end + 1;
> 
> As a separate note regarding a preceding patch ("mm/damon/core: merge
> quota-sliced regions back"), are regions split due to quota limits being
> skipped and orphaned during the target loop?
> 
> In damos_apply_target(), the loop uses damon_for_each_region_safe(r, next, t)
> which fetches next before executing the body. If damon_do_apply_schemes()
> splits r, damon_split_region_at() inserts the new remainder region
> immediately after r.
> 
> Because next was pre-fetched to the original next region, the newly inserted
> remainder appears to be bypassed by the iterator. This might prevent the
> remainder region from being processed by other schemes in the current cycle,
> and the intended merge-back logic might not execute for it.

Yes, as I mentioned to the earlier question just above.

> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260409142148.60652-4-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-4-sj@kernel.org
#
# [1] https://github.com/sjp38/hackermail