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

[Liew Rui Yan <aethernet65535@gmail.com>]

On Fri, 28 Aug 2026 11:29:09 -0700 SJ Park <sj@kernel.org> wrote:

> 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).

Yes, thank you for pointing that out!  I changed cursor to
charge_{target,addr}_from now.

> 
> >     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.

This is the updated example:

'''
Example:

    1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
    2. Quota is configured to process only 100 bytes per window.
    3. Window 1: Processes R1 (0-100).  Quota is full.  charge_{target,
        addr}_from is saved at (Target, 100).
    4. Window 2: The loop reaches R2.  Because R2 is
        damon_last_region(t), the old code unconditionally returns true,
        skipping R2 entirely and resetting the charge_{target,addr}_from.

    Result: R2 is permanently skipped even though it has never been
    processed.
'''

> 
> 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.

I will add this clarification in the next revision:

'''
However, it is important to note that this is a very minor issue.  This
is because it is triggered only when the previous window saved/kept
charge_{target,addr}_from, and in the next window, all regions except
the last region were skipped by damos_skip_charged_region().
'''

> 
> > 
> > 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,
> '''

I noticed a potential subtle issue in the suggested fix above:

'''
+out:
+       if (r == damon_last_region(t)) {
                quota->charge_target_from = NULL;
                quota->charge_addr_from = 0;
+               return true;
        }
'''

If 'skip' is false (region should be processed), but it happens to be
the last region, the condition 'if (r == damon_last_region(t))' would
still be met.  This would cause it to reset the state and 'return true'
(skip it), which inadvertently re-introduces the original bug we are
trying to fix.

To ensure the reset logic is centralized and correct, I refined the fix
as follows.  The comment is intended to help you and other reviewers
quickly understand the rationale behind the compound condition.  I will
remove this comment in the next revision.

'''
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 644daf5a1656..82c5aed8a417 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2342,36 +2342,48 @@ 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:
+       /*
+        * The last region may remain unapplied for extended period due to
+        * various regions (e.g., it is invalid or has been filtered out),
+        * preventing other regions from being applied (those preceding the last
+        * region and all regions with different targets).  Therefore, when
+        * encountering a region that needs to be processed, reset
+        * charge_{target,addr}_from.  If necessary, this parameters will be set
+        * to the correct value in damos_do_apply() due to quota is full.
+        */
+       if ((r == damon_last_region(t) && skip) || !skip) {
                quota->charge_target_from = NULL;
                quota->charge_addr_from = 0;
        }
-       return false;
+       return skip;
 }
 
 static void damos_update_stat(struct damos *s,
'''

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

I agree that a kunit test would be valuable.  While I am still getting
familiar with the kunit and it might take me a little time, I plan to
work on it.

Should the tests include these scenarios?  Note that I use 1-based index
in here since it is easier to understand.

1. Baseline test:

    - Parameters: charge_target_from = NULL, charge_addr_from = 0,
      nr_target = 1, nr_region = 3.

    - Expected: Returns false three times in a row.
      charge_{target,addr}_from remains (NULL, 0).

2. Skip test:

    - Parameters: charge_target_from = target[1], charge_addr_from =
      region[2]->ar.end, nr_target = 1, nr_region = 3.

    - Expected: Returns true, true (for region[1] and region[2]), then
      false (for region[3]).  charge_{target,addr}_from is reset to
      (NULL, 0) after region[1].

3. Split test:

    - Parameters: charge_target_from = target[1], charge_addr_from =
      midpoint of region[2], nr_target = 1, nr_region = 3.

    - Other: Ensure region[2] is large enough for the split to succeed.

    - Expected: Returns true (region[1]), true (region[2] first half),
      false (region[3] second half), false (region[4]).  Total nr_region
      becomes 4.  charge_{target,addr}_from is reset to (NULL, 0) after
      region[0].

4. Sashiko's edge case (Split failure on last region):

    - Parameters: charge_target_from = target[1], charge_addr_from =
      midpoint of region[3], nr_target = 1, nr_region = 3.

    - Other: Ensure region[3] is small enough so that the split is
      guaranteed to fail (sz_to_skip < min_region_sz).

    - Expected: Returns true three times in a row.  Crucially,
      charge_{target,addr}_from is reset to (NULL, 0) on the third call,
      preventing permanent state leakage.

5. Last region processing test:

   - Parameters: charge_target_from = target[1], charge_addr_from =
     region[2]->ar.end, nr_target = 1, nr_region = 3.

    - Expected:

        - Round 1: Returns true (region[1]), true (region[2]), false
          (region[3], resets charge_{target,addr}_from because !skip).

        - Round 2: Since the charge_{target,addr}_from is now (NULL, 0),
          it should return false three times in a row, proving that
          subsequent regions are not incorrectly blocked.

If there are any issues or missing edge cases in these scenarios, please
let me know!

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

Best regards,
Rui Yan