Re: (sashiko review) [RFC PATCH v2 1/9] mm/damon/core: introduce failed region quota charge ratio

[SeongJae Park <sj@kernel.org>]

TL;DR: sashiko found one minor issue that better not to be fixed, one issue
that orthogonal to this patch, and one minor issue that better to be fixed.  I
will separately work on the hotfix of the second issue and address the third
issue in the next revision of this series.

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

> # review url: https://sashiko.dev/#/patchset/20260405151232.102690-2-sj@kernel.org
> # start of sashiko.dev inline review
> commit feec6568732fbe7a81b21aa5fac5c470ce0f7381
> Author: SeongJae Park <sj@kernel.org>
> Subject: mm/damon/core: introduce failed region quota charge ratio
> 
> This commit introduces a configurable ratio to charge memory quota for
> DAMOS actions that failed, aiming for fairer quota accounting when actions
> do not fully succeed.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- 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);
> > +}
> 
> Can mult_frac() overflow here?
> 
> If a user configures fail_charge_num to a large value, the multiplication of
> the failed region size (which could be terabytes) and fail_charge_num could
> exceed ULONG_MAX on 64-bit systems, resulting in a truncated value added to
> charged_sz.

It can indeed make DAMOS work in an odd manner, but it doesn't break something
in real.  Also, the real world privileged users ain't set such large value.
I'd not make change for this, as I prefer code simplicity here rather than
handling the weird user inputs.  We can revisit this later if a real issue from
this is reported, or a simple fix is found.

> 
> Also, if fail_charge_num is 0 and actions fail, charged_sz increases by 0,
> leaving total_charged_sz at 0.
> 
> If total_charged_ns continues to accumulate the time spent attempting these
> actions, throughput in damos_set_effective_quota() would evaluate to 0:
> 
> throughput = mult_frac(quota->total_charged_sz, 1000000,
>                        quota->total_charged_ns);
> 
> If throughput is 0, the effective size quota (esz) becomes 0.
> When esz is 0, damos_apply_scheme() calculates sz as 0:
> 
> sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz);
> 
> This triggers the if (!sz) goto update_stat; condition, skipping time
> measurement entirely. Does this cause the scheme to permanently lock up with
> 0 throughput and 0 quota?

Nice finding.  But the issue exists even before this patch, because throughput
could be zero if 'total_charged_sz < total_charged_ns / 1000000'.

I will fix this with a separate hotfix, like below:

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2683,7 +2683,7 @@ static void damos_set_effective_quota(struct damos_quota *quota)
        }

        if (quota->ms) {
-               if (quota->total_charged_ns)
+               if (quota->total_charged_ns && quota->total_charged_sz)
                        throughput = mult_frac(quota->total_charged_sz,
                                        1000000, quota->total_charged_ns);
                else
'''

> 
> [ ... ]
> 
> > @@ -2097,7 +2116,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);
> >  		if (damos_quota_is_set(quota) &&
> >  				quota->charged_sz >= quota->esz) {
> >  			quota->charge_target_from = t;
> 
> Since the new fractional charging logic can produce unaligned charged_sz
> values, what happens if the remaining quota drops below min_region_sz?
> 
> In damos_apply_scheme(), sz is calculated as:
> 
> sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz);
> 
> If esz - charged_sz < c->min_region_sz, sz evaluates to 0. It then skips the
> charge update and jumps to update_stat.
> 
> Because charged_sz doesn't increase, this skip condition evaluates to true
> for every subsequent region, iterating through the remaining address space
> without doing anything.
> 
> Furthermore, the loop exits without ever satisfying charged_sz >= esz.
> Without satisfying this condition, charge_target_from is never set, causing
> the next charge window to start from the beginning of the address space
> again. Does this prevent tail regions from ever being processed?

Good finding.  Not a critical issue for a quite rare case, but I agree this
(charge_target_from never set) is better to be fixed.  This happens because the
quota exceed check logic is not aware of this case.  I will address this in the
next version by updating the quota exceed check to aware of this case, like
below.

'''
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 47770221abe29..42e444b48634f 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2320,6 +2320,14 @@ static void damos_charge_quota(struct damos_quota *quota,
 				quota->fail_charge_denom);
 }
 
+static bool damos_quota_is_full(struct damos_quota *quota,
+		unsigned long min_region_sz)
+{
+	if (!damos_quota_is_set(quota))
+		return false;
+	return quota->charged_sz + min_region_sz > quota->esz;
+}
+
 static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		struct damon_region *r, struct damos *s)
 {
@@ -2377,8 +2385,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		quota->total_charged_ns += timespec64_to_ns(&end) -
 			timespec64_to_ns(&begin);
 		damos_charge_quota(quota, sz, sz_applied);
-		if (damos_quota_is_set(quota) &&
-				quota->charged_sz >= quota->esz) {
+		if (damos_quota_is_full(quota, c->min_region_sz)) {
 			quota->charge_target_from = t;
 			quota->charge_addr_from = r->ar.end + 1;
 		}
@@ -2406,8 +2413,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 			continue;
 
 		/* Check the quota */
-		if (damos_quota_is_set(quota) &&
-				quota->charged_sz >= quota->esz)
+		if (damos_quota_is_full(quota, c->min_region_sz))
 			continue;
 
 		if (damos_skip_charged_region(t, r, s, c->min_region_sz))
@@ -2736,8 +2742,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
 	if (!time_in_range_open(jiffies, quota->charged_from,
 				quota->charged_from +
 				msecs_to_jiffies(quota->reset_interval))) {
-		if (damos_quota_is_set(quota) &&
-				quota->charged_sz >= quota->esz)
+		if (damos_quota_is_full(quota, c->min_region_sz))
 			s->stat.qt_exceeds++;
 		quota->total_charged_sz += quota->charged_sz;
 		if (quota->total_charged_sz > ULONG_MAX / 2 ||
'''


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