Re: [PATCH] mm/damon/core: set initial quota->charged_from to INITIAL_JIFFIES

[SeongJae Park <sj@kernel.org>]

On Tue, 19 Aug 2025 03:38:03 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> Kernel initialize "jiffies" timer as 5 minutes below zero, as shown in
> include/linux/jiffies.h
> 
>  /*
>   * Have the 32 bit jiffies value wrap 5 minutes after boot
>   * so jiffies wrap bugs show up earlier.
>   */
>   #define INITIAL_JIFFIES ((unsigned long)(unsigned int) (-300*HZ))
> 
> In 32bit system, if quota->charged_from is initialized to 0 as it now,
> it will not adjust event if reset_interval_ms passes for the first 5
> minutes.

Thanks to your clarification on another mail, I now understand this can happen
because time_after_eq() casts the diff of the given two unsigned long values
into signed long type.  I might be not the only one who can be confused on this
part, though.  I think at least I of future will be confused again.  Please add
the detail on the changelog.

The above explanation is not technically wrong, but I think it is not
describing the issue completely.  The above description is saying about only a
case that a DAMOS scheme with quotas is applied just after the system is boot.

But, a similar and much worse problems can happen at anytime if such scheme is
applied while jiffies value is somewhat that can be casted into any negative
signed long value, e.g., about 25 days after the boot, assuming HZ value 1,000.
In the worst case, hence, a quota charging window can continue for up to about
25 days.

The same problem can theoretically happen on 64 bit machines too, though not
practically early (about 300 million years after the boot, assuming HZ value
1,000).

If I'm not wrong, I think the fix of this bug deserves Cc-ing stable@.

> 
> So change initialize value of quota->charged_from to INITIAL_JIFFIES.

So this will fix only just-after-boot time issue.

I think it should be initialized to the jiffies value of the time that the
quota really starts being charged.

> This soultion has already been applied in commit 7d70e15480c0 ("writeback:
> add missing INITIAL_JIFFIES init in global_update_bandwidth()") or else.
> 
> Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> ---
> I think it would be good to add selftest of below senario. 
> 
> 1. Set DAMON with quota.
> 2. Wait and check esz is updated well.
> 3. If esz is not updated after quite long time, set test to fail.
> 
> but I'm not sure that selftest can support environment handling; jiffies
> with INITIAL_JIFFIES or near there.
> 
> If there is another method that i don't know. Could you please let me
> know? Or any better idea?
> 
> ---
>  mm/damon/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index cb41fddca78c..90317a3bcf78 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -366,7 +366,7 @@ static struct damos_quota *damos_quota_init(struct damos_quota *quota)
>  	quota->total_charged_sz = 0;
>  	quota->total_charged_ns = 0;
>  	quota->charged_sz = 0;
> -	quota->charged_from = 0;
> +	quota->charged_from = INITIAL_JIFFIES;

As I argued above, I think we should at least set this as 'jiffies', not
INITIAL_JIFFIES.

Also, damos_quota_init() is called by damon_new_scheme().  We cannot guarantee
if the caller will directly start DAMON with it, or commit it into running
DAMON.

So a better approach would be initializing ->charged_from at the beginning of
kdamond_fn(), e.g., inside kdamond_init_ctx().  For commit case, damos_commit()
may also need to be updated.  To me, this feels too complicated for stable
kernel backports.

What about modifying damos_adjust_quota() to initialize ->charged_from, like
below?

--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2130,6 +2130,10 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
        if (!quota->ms && !quota->sz && list_empty(&quota->goals))
                return;

+       /* First charge window */
+       if (!quota->total_charged_sz && !quota->charged_from)
+               quota->charged_from = jiffies;
+
        /* New charge window starts */
        if (time_after_eq(jiffies, quota->charged_from +
                                msecs_to_jiffies(quota->reset_interval))) {
[...]

In theory, this can also incur wrong behavior in a case that charged_from is
overflowed to zero while total_charged_sz remains zero.  Such cases would be
unrealistic, though.  And even if it happens, nothing goes really wrong.  It
will only extend the current charging window for one reset_interval, once per
jiffies overflow.  Meanwhile, this fix is simple and can be easily backported
to old stable kernels.  So I'd like to recommend this option.

What do you think, Sang-Heon?


Thanks,
SJ