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

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

[Sang-Heon Jeon]

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.

So change initialize value of quota->charged_from to INITIAL_JIFFIES.
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;
 	quota->charge_target_from = NULL;
 	quota->charge_addr_from = 0;
 	quota->esz_bp = 0;
-- 
2.43.0

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

[SeongJae Park]

Hello Sang-Heon,

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.

jiffies is unsigned.  Hence the initial invocation of time_after_eq() in
damos_adjust_quota(), which is the only place reading quota->charged_from, will
return 'true' and the quota adjustment would be done without delay, unless the
scheme apply interval is unrealistically big.

Please let me know if I'm missing something.


Thanks,
SJ

[...]

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

[Sang-Heon Jeon]

Hello, SeongJae

On Tue, Aug 19, 2025 at 8:01 AM SeongJae Park <sj@kernel.org> wrote:
>
> Hello Sang-Heon,
>
> 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.
>
> jiffies is unsigned.  Hence the initial invocation of time_after_eq() in
> damos_adjust_quota(), which is the only place reading quota->charged_from, will
> return 'true' and the quota adjustment would be done without delay, unless the
> scheme apply interval is unrealistically big.

Yeah, jiffies is unsigned but to cover wraparound in 32bit,
`time_after_eq` casts intermediate value to signed value.

#define time_after_eq(a,b)    \
    (typecheck(unsigned long, a) && \
     typecheck(unsigned long, b) && \
     ((long)((a) - (b)) >= 0))

Here is an example with INITIAL_JIFFIES(HZ is assumed 1000); a =
INITIAL_JIFFIES = 0xFFFB6C20, b = charged_from + interval = 0 + 1000
(just assumed value) = 0x000003E8 and a-b = 0xFFFB6C20 - 0x00003E8 =
0xFFFB6838; (long)((a) - (b)) is interpreted negative value in 32 bit
system.

> 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.

Also, you can easily reproduce the above situation on a 32bit system.
If my explanation is insufficient, please let me know. I'll try to
find another explanation.

> Please let me know if I'm missing something.
>
>
> Thanks,
> SJ
>
> [...]

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

[SeongJae Park]

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

> Hello, SeongJae
> 
> On Tue, Aug 19, 2025 at 8:01 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > Hello Sang-Heon,
> >
> > 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.
> >
> > jiffies is unsigned.  Hence the initial invocation of time_after_eq() in
> > damos_adjust_quota(), which is the only place reading quota->charged_from, will
> > return 'true' and the quota adjustment would be done without delay, unless the
> > scheme apply interval is unrealistically big.
> 
> Yeah, jiffies is unsigned but to cover wraparound in 32bit,
> `time_after_eq` casts intermediate value to signed value.
> 
> #define time_after_eq(a,b)    \
>     (typecheck(unsigned long, a) && \
>      typecheck(unsigned long, b) && \
>      ((long)((a) - (b)) >= 0))

Oops, I missed this part.  Thank you for clarifying.  I still have a question
about the fix, though.  I will reply to the original mail.


Thanks,
SJ

[...]

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

[SeongJae Park]

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

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

[Sang-Heon Jeon]

On Tue, Aug 19, 2025 at 2:25 PM SeongJae Park <sj@kernel.org> wrote:
>
> 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.

I am also confused with the above things. I'll add more descriptions
including time_after_eq() as well.

FYI: If we use jiffies_64 instead of jiffies, maybe it will not be
confused anymore. but you know, maybe it can be a big change.

> 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.

Maybe I'll add more examples of problem cases to commit messages. I'll
try it on the next version.

> 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).

Yeah, in a 64bit system. I think this could not be a big problem.
Maybe I can add this minor comment to commit messages as well.

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

You think the same as me. I'll find the commit and add Fixes and CC to
the next version patch.

> >
> > 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.

You're totally right. I first used jiffies(this patch didn't show that
anyway), but while trying to find other reference commits and making
the commit messages more detailed, I got confused. late night issue.
I'll fix them.

> > 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.

ditto.

> 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?

Oh, I missed this point. I just consider assignment points not timing.
Thanks for clarifying. Honestly, I think changing kdamond_init_ctx()
and damos_commit() are more accurate solution. but I agree with your
point as well. It looks a little bit more difficult.

> --- 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.

You're right. I agree. It is just in theory.

> 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?

Your solution is reasonable. I'll apply your direction on the next version.

>
> Thanks,
> SJ

Best Regards.
Sang-Heon Jeon