Re: [PATCH] mm/damon: fix unconditionally skip last region

[SJ Park <sj@kernel.org>]

On Fri, 28 Aug 2026 16:47:37 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Once quota set, the charge_{target,addr}_from unconditionally skips and
> resets at the last region of the tracked target, so the last region can
> be skipped even when it has not been processed.
> 
> Example:
> 
>     1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
>     2. Quota is configured to process only 50 bytes per window.
>     3. Window 1: Processes R1 (0-50).  Quota is full.  Cursor is saved
>        at (Target, 50).

Cursor means charge_{target,addr}_from, right?  Let's explain that, or just
keep using the terms (charge_{target,addr}_from).

>     4. Window 2: Skips R1 (0-50).  Processes R1 (50-100).  Quota is
>        full. Cursor is saved at (Target, 100), which is exactly the
>        start of R2.
>     5. Window 3: The loop reaches R2.  Because R2 is
>        damon_last_region(t), the old code unconditionally returns true,
>        skipping R2 entirely and resetting the cursor.
> 
>     Result: R2 is permanently skipped even though it has never been
>     processed.

Let's make example simpler by setting R1 (0-50 bytes) and R2 (50-100 bytes) or
quota size 100 bytes per window.

Also, it continues being skipped only in a corner case that the region
addresses and the access patterns are kept.  So the user impact is mild.  Let's
clarify that to not make users unnecessarily afraid.

> 
> Fix this by only skipping the last region after it has been applied.
> 
> Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
> Cc: <stable@vger.kernel.org> # v5.16.x
> Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> ---
> 
> Changes from RFC v1:
> - Minimal fix, only fixes the issue where the last-region is skipped.
> - Add an example to the commit message to demonstrate that this error
>   occurs very rarely.
> - RFC v1: https://lore.kernel.org/damon/20260825124616.5129-1-aethernet65535@gmail.com
> 
> ---
>  mm/damon/core.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a1656..21dc6b086c42 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2347,14 +2347,15 @@ static bool damos_skip_charged_region(struct damon_target *t,
>  	if (quota->charge_target_from) {
>  		if (t != quota->charge_target_from)
>  			return true;
> -		if (r == damon_last_region(t)) {
> -			quota->charge_target_from = NULL;
> -			quota->charge_addr_from = 0;
> -			return true;
> -		}
>  		if (quota->charge_addr_from &&
> -				r->ar.end <= quota->charge_addr_from)
> +				r->ar.end <= quota->charge_addr_from) {
> +			if (r->ar.end == quota->charge_addr_from ||
> +					damon_is_last_region(r, t)) {
> +				quota->charge_target_from = NULL;
> +				quota->charge_addr_from = 0;
> +			}
>  			return true;
> +		}
>  
>  		if (quota->charge_addr_from && r->ar.start <
>  				quota->charge_addr_from) {

As Sashiko pointed out, this doesn't work if the the last region's start
address is smaller than charge_addr_from and the end address is larger than
charge_addr_from, but the size to skip (charge_addr_from - r->ar.start) is
smaller than min_region_sz.

As you replied to Sashiko, let's do the last region handling in every case.
While doing that, let's do the charge_{target,addr}_from reset in only one
place, like below.

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2688,36 +2688,40 @@ static bool damos_skip_charged_region(struct damon_target *t,
 {
        struct damos_quota *quota = &s->quota;
        unsigned long sz_to_skip;
+       bool skip = false;

        /* Skip previously charged regions */
        if (quota->charge_target_from) {
                if (t != quota->charge_target_from)
                        return true;
-               if (r == damon_last_region(t)) {
-                       quota->charge_target_from = NULL;
-                       quota->charge_addr_from = 0;
-                       return true;
-               }
                if (quota->charge_addr_from &&
-                               r->ar.end <= quota->charge_addr_from)
-                       return true;
+                               r->ar.end <= quota->charge_addr_from) {
+                       skip = true;
+                       goto out;
+               }

                if (quota->charge_addr_from && r->ar.start <
                                quota->charge_addr_from) {
                        sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
                                        r->ar.start, min_region_sz);
                        if (!sz_to_skip) {
-                               if (damon_sz_region(r) <= min_region_sz)
-                                       return true;
+                               if (damon_sz_region(r) <= min_region_sz) {
+                                       skip = true;
+                                       goto out;
+                               }
                                sz_to_skip = min_region_sz;
                        }
                        damon_split_region_at(t, r, sz_to_skip);
-                       return true;
+                       skip = true;
                }
+       }
+out:
+       if (r == damon_last_region(t)) {
                quota->charge_target_from = NULL;
                quota->charge_addr_from = 0;
+               return true;
        }
-       return false;
+       return skip;
 }

 static void damos_update_stat(struct damos *s,
'''

Btw, I think damos_skip_charged_region() may deserve a kunit test.

[1] https://lore.kernel.org/20260828090410.40AEA1F000E9@smtp.kernel.org
[2] https://lore.kernel.org/20260828115047.332978-1-aethernet65535@gmail.com


Thanks,
SJ

[...]