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

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

On Sun, 30 Aug 2026 11:38:51 -0700 SJ Park <sj@kernel.org> wrote:

> On Sun, 30 Aug 2026 16:29:46 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> 
> > On Sun, 30 Aug 2026 06:47:07 +0000 sashiko-bot@kernel.org wrote:
> > 
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > 
> > > Pre-existing issues:
> > > - [High] A preexisting bug causes DAMOS scheme application to permanently stall for all subsequent targets if `quota->charge_target_from` points to a target that becomes invalid or loses all its regions.
> > > --
> > > 
> > > commit 630d73d512e66245612cbdfe5c49647aa6ed2874
> > > Author: Liew Rui Yan <aethernet65535@gmail.com>
> > > 
> > > mm/damon/core: fix unconditionally skip last region
> > > 
> > > This patch fixes an issue where the last region of a tracked target could
> > > be unconditionally skipped and reset without being processed when a scheme
> > > quota is exhausted. By modifying the reset logic, it ensures the last region
> > > is appropriately handled rather than always skipped.
> > > 
> > > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > > index 644daf5a16560..fb76b530bbc68 100644
> > > > --- a/mm/damon/core.c
> > > > +++ b/mm/damon/core.c
> > > > @@ -2342,36 +2342,39 @@ 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;
> > > 
> > > [Severity: High]
> > > This isn't a bug introduced by this patch, but does this logic cause all
> > > subsequent targets to permanently stall if the target currently stored in
> > > quota->charge_target_from becomes invalid?
> > > 
> > > If the monitored process dies before the next window, kdamond_apply_schemes()
> > > appears to bypass it without clearing the saved target:
> > > 
> > > mm/damon/core.c:kdamond_apply_schemes() {
> > >     ...
> > > 		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> > > 			continue;
> > > 		damos_apply_target(c, t, max_region_sz);
> > >     ...
> > > }
> > > 
> > > Because the target is skipped entirely, we never call damos_apply_target()
> > > and never reach the clearing mechanism inside damos_skip_charged_region().
> > > 
> > > When this happens, wouldn't all other valid targets continue to evaluate
> > > (t != quota->charge_target_from) as true here, permanently skipping scheme
> > > applications?
> > > 
> > > It appears a local user could reliably trigger this by setting a restrictive
> > > quota on a memory-intensive process they own, waiting for the quota to
> > > exhaust, and then terminating the process.
> > 
> > Thank you for pointing this out!  Yes, this can happens.
> > 
> > I just wrote a simple fix, this fix introduces damon_target->'bool
> > has_charged', which was added to prevent entering the
> > damon_target_remove_ref() (new function introduces by this fix) loop
> > every time an invalid target is encountered.  However, adding this field
> > may be unnecessary, or it might better to use something like 'unsigned
> > int charged_ref_count' instead.
> > 
> > '''
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 0c8b7ddef9ab..19d46d792858 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -93,6 +93,7 @@ struct damon_region {
> >  struct damon_target {
> >  	struct pid *pid;
> >  	bool obsolete;
> > +	bool has_charged;
> 
> I'd prefer not introducing new field.
> 
> >  /* private: */
> >  	/* Number of monitoring target regions of this target. */
> >  	unsigned int nr_regions;
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 644daf5a1656..6c14b2d4e9cd 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2630,6 +2630,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> >  		if (damos_quota_is_full(quota, c->min_region_sz)) {
> >  			quota->charge_target_from = t;
> >  			quota->charge_addr_from = r->ar.end;
> > +			t->has_charged = true;
> >  		}
> >  	}
> >  	if (s->action != DAMOS_STAT)
> > @@ -3216,6 +3217,26 @@ static void damos_trace_stat(struct damon_ctx *c, struct damos *s)
> >  	trace_call__damos_stat_after_apply_interval(cidx, sidx, &s->stat);
> >  }
> >  
> > +static void damon_target_remove_ref(struct damon_ctx *c,
> > +					 struct damon_target *t)
> > +{
> > +	struct damos *s;
> > +
> > +	damon_for_each_scheme(s, c) {
> > +		struct damos_quota *quota = &s->quota;
> > +
> > +		if (!quota->charge_target_from)
> > +			continue;
> > +
> > +		if (quota->charge_target_from == t) {
> > +			quota->charge_target_from = NULL;
> > +			quota->charge_addr_from = 0;
> > +		}
> > +	}
> > +
> > +	t->has_charged = false;
> > +}
> > +
> >  static void kdamond_apply_schemes(struct damon_ctx *c)
> >  {
> >  	struct damon_target *t;
> > @@ -3241,8 +3262,11 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
> >  	max_region_sz = damon_region_sz_limit(c);
> >  	mutex_lock(&c->walk_control_lock);
> >  	damon_for_each_target(t, c) {
> > -		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> > +		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
> > +			if (t->has_charged)
> > +				damon_target_remove_ref(c, t);
> >  			continue;
> > +		}
> >  		damos_apply_target(c, t, max_region_sz);
> >  	}
> 
> I think damos_adjust_quota() is a better place to do this.  For example,
> 
> '''
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3530,6 +3530,22 @@ static void damos_trace_esz(struct damon_ctx *c, struct damos *s,
>         trace_damos_esz(cidx, sidx, quota->esz);
>  }
> 
> +static void damos_reset_invalid_charge_target_from(struct damos_quota *quota,
> +               struct damon_ctx *c)
> +{
> +       struct damon_target *t;
> +
> +       t = quota->charge_target_from;
> +       if (!t)
> +               return;
> +       if (!c->ops.target_valid)
> +               return;
> +       if (c->ops.target_valid(t))
> +               return;
> +       quota->charge_target_from = NULL;
> +       quota->charge_addr_from = 0;
> +}
> +
>  static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
>  {
>         struct damos_quota *quota = &s->quota;
> @@ -3570,6 +3586,8 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
>                         damos_trace_esz(c, s, quota);
>         }
> 
> +       damos_reset_invalid_charge_target_from(quota, c);
> +
>         if (!c->ops.get_scheme_score)
>                 return;
> '''
> 
> > '''
> > 
> > SJ, when should this bug be fixed?  Along with this patch, or before
> > this patch?
> 
> Whenever the fix is ready.  If you don't mind, I will take this.

I do not mind, thank you :>

Best regards,
Rui Yan