Re: [PATCH v1 1/2] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[SJ Park <sj@kernel.org>]

Hi Donggeun,


Thank you for this patch.

On Sat, 19 Sep 2026 16:13:23 +0900 Donggeun Yoo <donggeunyoo.kernel@gmail.com> wrote:

> damos_goal_tune_esz_bp_temporal() converts the scheme's size quota into
> basis points with "quota->esz_bp = quota->sz * 10000", both unsigned long,
> and damos_set_effective_quota() divides the result back by 10000.
> quotas/bytes is unbounded; bytes_store() hands it to kstrtoul() as is.
> 
> On 32-bit the product wraps for any size quota above ULONG_MAX / 10000,
> that is 429496 bytes.  Documentation/admin-guide/mm/damon/usage.rst
> instructs "echo $((1024*1024*1024)) > quotas/bytes", and 1 GiB * 10000 is
> 2500 * 2^32, so that documented value wraps to exactly zero; 256 MiB and
> every multiple of it do the same.  quota->esz then becomes zero while the
> goal is not achieved, the trailing "if (quota->sz && quota->sz < esz)" can
> only lower esz further, and damos_quota_is_full() is true on the first test
> of every charge window, so the scheme applies nothing and the goal is never
> approached.  Other sizes are wrong without being zero: 500000 yields 70503.

For 32-bit machines, we have addr_unit parameter.  I believe use of it could
effectively solve this kind of issues.  Correct me if I'm wrong.

There could be cases that addr_unit cannot help, though.  Particularly, if I
remember correctly, 'addr_unit' works for only paddr.  Also it doesn't fix all
theoretical corner cases.  Even on 64 bit machines, same problem exists in
theory.  So I think this change is worthy to have.  But I think it is better to
mention existence of addr_unit and why it is not the perfect solution in the
commit message.

Maybe it is worthy to add the comment on the user documents, too.

> 
> Saturate to ULONG_MAX, which is what the same function already writes for a
> scheme with no size quota.  Widening esz_bp instead would reach the consist
> tuner, which runs the same field through damon_feed_loop_next_input(),
> unsigned long in and out; bounding the multiply keeps the change to this
> branch.  On 32-bit a large size quota then behaves like no size quota
> rather than like a dead scheme.
> 
> Fixes: af738a6a00c1 ("mm/damon/core: introduce DAMOS_QUOTA_GOAL_TUNER_TEMPORAL")
> Cc: <stable@vger.kernel.org> # 7.1.x
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
> Measured on i386 under QEMU: one paddr context with a stat scheme, the
> temporal goal tuner, and one unachieved user_input goal.  Each size is
> written to quotas/bytes, the kdamond is started, and
> quotas/effective_bytes is read back after
> update_schemes_effective_quotas.
> 
>   quotas/bytes    effective_bytes    effective_bytes
>                   before             after
>   4096            4096               4096
>   429496          429496             429496
>   429497          0                  429496
>   268435456       0                  429496
>   1073741824      0                  429496
>   500000          70503              429496
>   4294967295      429495             429496
>   0               429496             429496
> 
> Everything the conversion can hold is unchanged, and 429496 is what the
> no-size-quota row already produced before the patch.
> 
> Patch 2 pins the same boundary at ULONG_MAX / 10000 and so runs on any
> word size.  Without this patch it fails on x86_64:
> 
>   # damos_test_esz_goal_temporal: EXPECTATION FAILED at mm/damon/tests/core-kunit.h:1959
>   Expected s.quota.esz == max_sz, but
>       s.quota.esz == 0 (0x0)
>       max_sz == 1844674407370955 (0x68db8bac710cb)
> 
>  mm/damon/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 2258b72da7a7..5ec476cef4db 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3274,10 +3274,10 @@ static void damos_goal_tune_esz_bp_temporal(struct damon_ctx *c,
>  
>  	if (score >= 10000)
>  		quota->esz_bp = 0;
> -	else if (quota->sz)
> -		quota->esz_bp = quota->sz * 10000;
> -	else
> +	else if (!quota->sz || quota->sz > ULONG_MAX / 10000)
>  		quota->esz_bp = ULONG_MAX;
> +	else
> +		quota->esz_bp = quota->sz * 10000;
>  }

In my humble opinion, this could be easier to read in below way:

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3524,7 +3524,7 @@ static void damos_goal_tune_esz_bp_temporal(struct damon_ctx *c,

        if (score >= 10000)
                quota->esz_bp = 0;
-       else if (quota->sz)
+       else if (quota->sz && quota->sz <= ULONG_MAX / 10000)
                quota->esz_bp = quota->sz * 10000;
        else
                quota->esz_bp = ULONG_MAX;
'''

What do you think?


Thanks,
SJ

[...]