Re: (sashiko review) [RFC PATCH v3 01/10] mm/damon/core: handle <min_region_sz remaining quota as empty

[SeongJae Park <sj@kernel.org>]

TL; DR: sashiko made a good suggestion.  I will address that in the next
revision.

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

> # review url: https://sashiko.dev/#/patchset/20260407010536.83603-2-sj@kernel.org
> # start of sashiko.dev inline review
> commit 3657261e42ea1ce93233b322fabf0d2c5c335a35
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: handle <min_region_sz remaining quota as empty
> 
> This commit updates the DAMON quota checks to treat a remaining quota of less
> than min_region_sz as fully charged. It introduces a new helper function,
> damos_quota_is_full(), and replaces the existing explicit quota checks with it.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 3bc7a2bbfe7de..c29f70bcce894 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -2041,6 +2041,14 @@ static void damos_walk_cancel(struct damon_ctx *ctx)
> >  	mutex_unlock(&ctx->walk_control_lock);
> >  }
> >  
> > +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;
> 
> Can quota->charged_sz + min_region_sz overflow here?
> 
> Since both variables are unsigned long, if min_region_sz is configured to a
> very large value via sysfs, or on 32-bit systems where memory regions can
> naturally be a large fraction of ULONG_MAX, the sum could wrap around.
> 
> If it wraps around, it would evaluate to a value smaller than quota->esz.
> This would cause damos_quota_is_full() to incorrectly return false even when
> the quota is fully exhausted, bypassing the DAMON action quota limits.
> 
> Could this be written using subtraction to prevent overflow?
> 
> return quota->charged_sz >= quota->esz ||
>        quota->esz - quota->charged_sz < min_region_sz;

Good suggestion, I will do so in the next revision.

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