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

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

On Sat, 29 Aug 2026 09:15:08 -0700 SJ Park <sj@kernel.org> wrote:

> On Sat, 29 Aug 2026 16:34:26 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> 
> > On Fri, 28 Aug 2026 11:29:09 -0700 SJ Park <sj@kernel.org> wrote:
> > 
> > > 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.
> 
> Ah, good catch.  The 'return true' is a wrong copy-pasta.  Let's drop the line.
> 
> > 
> > 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.
> > +        */
> 
> Looks too verbose to me.  Let's drop this.
> 
> > +       if ((r == damon_last_region(t) && skip) || !skip) {
> 
> Why this becomes this complex?  We should reset charge_{target,addr}_from if it
> is the last region, always.  Am I missing something?

Thank you for pointing this out!  As long as it is the last region, we
should reset.

> 
> >                 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.
> 
> Nice.  Looking forward to your patch.
> 
> > 
> > Should the tests include these scenarios?
> 
> Let's not add new topics.  We can discuss this on your kunit patch.  Please
> ensure it covers at least the corner case this patch is trying to fix.

Okay, I will ensure that.  Should the test be sent along with this
patch (as part of the same series), or should they be sent separately?

Best regards,
Rui Yan