[PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

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

And they cast unsigned value to signed to cover wraparound

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

In 64bit system, these might not be a problem because wrapround occurs
300 million years after the boot, assuming HZ value is 1000.

With same assuming, In 32bit system, wraparound occurs 5 minutues after
the initial boot and every 49 days after the first wraparound. And about
25 days after first wraparound, it continues quota charging window up to
next 25 days.

Example 1: initial boot
jiffies=0xFFFB6C20, charged_from+interval=0x000003E8
time_after_eq(jiffies, charged_from+interval)=(long)0xFFFB6838; In
signed values, it is considered negative so it is false.

Example 2: after about 25 days first wraparound
jiffies=0x800004E8, charged_from+interval=0x000003E8
time_after_eq(jiffies, charged_from+interval)=(long)0x80000100; In
signed values, it is considered negative so it is false

So, change quota->charged_from to jiffies at damos_adjust_quota() when
it is consider first charge window.

In theory; but almost impossible; quota->total_charged_sz and
qutoa->charged_from should be both zero even if it is not in first
charge window. But It will only delay one reset_interval, So it is not
big problem.

Fixes: 2b8a248d5873 ("mm/damon/schemes: implement size quota for schemes application speed control") # 5.16
Cc: stable@vger.kernel.org
Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
Changes from v1 [1]
- not change current default value of quota->charged_from
- set quota->charged_from when it is consider first charge below
- add more description of jiffies and wraparound example to commit
  messages

SeongJae, please re-check Fixes commit is valid. Thank you.

[1] https://lore.kernel.org/damon/20250818183803.1450539-1-ekffu200098@gmail.com/
---
 mm/damon/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index cb41fddca78c..93bad6d0da5b 100644
--- 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))) {
-- 
2.43.0

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[SeongJae Park]

On Wed, 20 Aug 2025 00:01:23 +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))
> 
> And they cast unsigned value to signed to cover wraparound

"they" sounds bit vague.  I think "jiffies comparison helper functions" would
be better.

> 
>  #define time_after_eq(a,b) \
>   (typecheck(unsigned long, a) && \
>   typecheck(unsigned long, b) && \
>   ((long)((a) - (b)) >= 0))
> 
> In 64bit system, these might not be a problem because wrapround occurs
> 300 million years after the boot, assuming HZ value is 1000.
> 
> With same assuming, In 32bit system, wraparound occurs 5 minutues after
> the initial boot and every 49 days after the first wraparound. And about
> 25 days after first wraparound, it continues quota charging window up to
> next 25 days.

It would be nice if you can further explain what real user impacts that could
make.  To my understanding the impact is that, when the unexpected extension of
the charging window is happened, the scheme will work until the quota is full,
but then stops working until the unexpectedly extended window is over.

The after-boot issue is really bad since there is no way to work around other
than reboot the machine.

> 
> Example 1: initial boot
> jiffies=0xFFFB6C20, charged_from+interval=0x000003E8
> time_after_eq(jiffies, charged_from+interval)=(long)0xFFFB6838; In
> signed values, it is considered negative so it is false.

The above part is using hex numbers and look like psuedo-code.  This is
unnecessarily difficult to read.  To me, this feels like your personal note
rather than a nice commit message that written for others.  I think you could
write this in a much better way.

> 
> Example 2: after about 25 days first wraparound
> jiffies=0x800004E8, charged_from+interval=0x000003E8
> time_after_eq(jiffies, charged_from+interval)=(long)0x80000100; In
> signed values, it is considered negative so it is false

Ditto.

> 
> So, change quota->charged_from to jiffies at damos_adjust_quota() when
> it is consider first charge window.
> 
> In theory; but almost impossible; quota->total_charged_sz and
> qutoa->charged_from should be both zero even if it is not in first

s/should/could/ ?

Also, explaining when that "could" happen will be nice.

> charge window. But It will only delay one reset_interval, So it is not
> big problem.
> 
> Fixes: 2b8a248d5873 ("mm/damon/schemes: implement size quota for schemes application speed control") # 5.16
> Cc: stable@vger.kernel.org
> Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>

I think the commit message could be much be improved, but the code change seems
right.

Reviewed-by: SeongJae Park <sj@kernel.org>

> ---
> Changes from v1 [1]
> - not change current default value of quota->charged_from
> - set quota->charged_from when it is consider first charge below
> - add more description of jiffies and wraparound example to commit
>   messages
> 
> SeongJae, please re-check Fixes commit is valid. Thank you.

I think it is valid.  Thank you for addressing my comments!


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[SeongJae Park]

+ Andrew

On Tue, 19 Aug 2025 10:27:18 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Wed, 20 Aug 2025 00:01:23 +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))
> > 
> > And they cast unsigned value to signed to cover wraparound
> 
> "they" sounds bit vague.  I think "jiffies comparison helper functions" would
> be better.
> 
> > 
> >  #define time_after_eq(a,b) \
> >   (typecheck(unsigned long, a) && \
> >   typecheck(unsigned long, b) && \
> >   ((long)((a) - (b)) >= 0))
> > 
> > In 64bit system, these might not be a problem because wrapround occurs
> > 300 million years after the boot, assuming HZ value is 1000.
> > 
> > With same assuming, In 32bit system, wraparound occurs 5 minutues after
> > the initial boot and every 49 days after the first wraparound. And about
> > 25 days after first wraparound, it continues quota charging window up to
> > next 25 days.
> 
> It would be nice if you can further explain what real user impacts that could
> make.  To my understanding the impact is that, when the unexpected extension of
> the charging window is happened, the scheme will work until the quota is full,
> but then stops working until the unexpectedly extended window is over.
> 
> The after-boot issue is really bad since there is no way to work around other
> than reboot the machine.
> 
> > 
> > Example 1: initial boot
> > jiffies=0xFFFB6C20, charged_from+interval=0x000003E8
> > time_after_eq(jiffies, charged_from+interval)=(long)0xFFFB6838; In
> > signed values, it is considered negative so it is false.
> 
> The above part is using hex numbers and look like psuedo-code.  This is
> unnecessarily difficult to read.  To me, this feels like your personal note
> rather than a nice commit message that written for others.  I think you could
> write this in a much better way.
> 
> > 
> > Example 2: after about 25 days first wraparound
> > jiffies=0x800004E8, charged_from+interval=0x000003E8
> > time_after_eq(jiffies, charged_from+interval)=(long)0x80000100; In
> > signed values, it is considered negative so it is false
> 
> Ditto.
> 
> > 
> > So, change quota->charged_from to jiffies at damos_adjust_quota() when
> > it is consider first charge window.
> > 
> > In theory; but almost impossible; quota->total_charged_sz and
> > qutoa->charged_from should be both zero even if it is not in first
> 
> s/should/could/ ?
> 
> Also, explaining when that "could" happen will be nice.
> 
> > charge window. But It will only delay one reset_interval, So it is not
> > big problem.
> > 
> > Fixes: 2b8a248d5873 ("mm/damon/schemes: implement size quota for schemes application speed control") # 5.16
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> 
> I think the commit message could be much be improved, but the code change seems
> right.
> 
> Reviewed-by: SeongJae Park <sj@kernel.org>
> 
> > ---
> > Changes from v1 [1]
> > - not change current default value of quota->charged_from
> > - set quota->charged_from when it is consider first charge below
> > - add more description of jiffies and wraparound example to commit
> >   messages
> > 
> > SeongJae, please re-check Fixes commit is valid. Thank you.
> 
> I think it is valid.  Thank you for addressing my comments!
> 
> 
> Thanks,
> SJ
> 
> [...]

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[Sang-Heon Jeon]

Hello, SeongJae

On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
>
> On Wed, 20 Aug 2025 00:01:23 +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))
> >
> > And they cast unsigned value to signed to cover wraparound
>
> "they" sounds bit vague.  I think "jiffies comparison helper functions" would
> be better.

I agree, I will change it.

> >
> >  #define time_after_eq(a,b) \
> >   (typecheck(unsigned long, a) && \
> >   typecheck(unsigned long, b) && \
> >   ((long)((a) - (b)) >= 0))
> >
> > In 64bit system, these might not be a problem because wrapround occurs
> > 300 million years after the boot, assuming HZ value is 1000.
> >
> > With same assuming, In 32bit system, wraparound occurs 5 minutues after
> > the initial boot and every 49 days after the first wraparound. And about
> > 25 days after first wraparound, it continues quota charging window up to
> > next 25 days.
>
> It would be nice if you can further explain what real user impacts that could
> make.  To my understanding the impact is that, when the unexpected extension of
> the charging window is happened, the scheme will work until the quota is full,
> but then stops working until the unexpectedly extended window is over.
>
> The after-boot issue is really bad since there is no way to work around other
> than reboot the machine.

I agree with your point that user impact should be added to commit
messages. Before modifying the commit message, I want to check that my
understanding of "user impact" is correct.

In the logic before this patch is applied, I think
time_after_eq(jiffies, ...) should only evaluate to false when the MSB
of jiffies is 1 and charged_from is 0. because if charging has
occurred, it changes charge_from to jiffies at that time. Therefore,
esz should also be zero because it is initialized with charged_from.
So I think the real user impact is that "quota is not applied", rather
than "stops working". If my understanding is wrong, please let me know
what point is wrong.

> >
> > Example 1: initial boot
> > jiffies=0xFFFB6C20, charged_from+interval=0x000003E8
> > time_after_eq(jiffies, charged_from+interval)=(long)0xFFFB6838; In
> > signed values, it is considered negative so it is false.
>
> The above part is using hex numbers and look like psuedo-code.  This is
> unnecessarily difficult to read.  To me, this feels like your personal note
> rather than a nice commit message that written for others.  I think you could
> write this in a much better way.
>
> >
> > Example 2: after about 25 days first wraparound
> > jiffies=0x800004E8, charged_from+interval=0x000003E8
> > time_after_eq(jiffies, charged_from+interval)=(long)0x80000100; In
> > signed values, it is considered negative so it is false
>
> Ditto.

Okay, I think I can fix these sections with explanation using MSB.

> >
> > So, change quota->charged_from to jiffies at damos_adjust_quota() when
> > it is consider first charge window.
> >
> > In theory; but almost impossible; quota->total_charged_sz and
> > qutoa->charged_from should be both zero even if it is not in first
>
> s/should/could/ ?

Sorry for my poor english.

> Also, explaining when that "could" happen will be nice.

I want to confirm this situation as well. I think the situation below
is the only case.

1. jiffies overflows to exactly 0
2. And quota is configured but never actually applied, so total_charged_sz is 0
3. And charging occurs at that exact moment.

Is that right? If right, I think this situation is almost impossible
and uncommon. I feel like It's unnecessary to describe it. I'm not
trying to ignore your valuable opinion, but do you still think it's
better to add a description?

> > charge window. But It will only delay one reset_interval, So it is not
> > big problem.
> >
> > Fixes: 2b8a248d5873 ("mm/damon/schemes: implement size quota for schemes application speed control") # 5.16
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
>
> I think the commit message could be much be improved, but the code change seems
> right.

Once again, Sorry for my poor english. I'm doing my best on my own.

> Reviewed-by: SeongJae Park <sj@kernel.org>
>
> > ---
> > Changes from v1 [1]
> > - not change current default value of quota->charged_from
> > - set quota->charged_from when it is consider first charge below
> > - add more description of jiffies and wraparound example to commit
> >   messages
> >
> > SeongJae, please re-check Fixes commit is valid. Thank you.
>
> I think it is valid.  Thank you for addressing my comments!
>
>
> Thanks,
> SJ
>
> [...]

Best Regards
Sang-Heon Jeon

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[SeongJae Park]

On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> Hello, SeongJae
> 
> On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > On Wed, 20 Aug 2025 00:01:23 +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))
> > >
> > > And they cast unsigned value to signed to cover wraparound
> >
> > "they" sounds bit vague.  I think "jiffies comparison helper functions" would
> > be better.
> 
> I agree, I will change it.
> 
> > >
> > >  #define time_after_eq(a,b) \
> > >   (typecheck(unsigned long, a) && \
> > >   typecheck(unsigned long, b) && \
> > >   ((long)((a) - (b)) >= 0))
> > >
> > > In 64bit system, these might not be a problem because wrapround occurs
> > > 300 million years after the boot, assuming HZ value is 1000.
> > >
> > > With same assuming, In 32bit system, wraparound occurs 5 minutues after
> > > the initial boot and every 49 days after the first wraparound. And about
> > > 25 days after first wraparound, it continues quota charging window up to
> > > next 25 days.
> >
> > It would be nice if you can further explain what real user impacts that could
> > make.  To my understanding the impact is that, when the unexpected extension of
> > the charging window is happened, the scheme will work until the quota is full,
> > but then stops working until the unexpectedly extended window is over.
> >
> > The after-boot issue is really bad since there is no way to work around other
> > than reboot the machine.
> 
> I agree with your point that user impact should be added to commit
> messages. Before modifying the commit message, I want to check that my
> understanding of "user impact" is correct.

I think you should make clear at least you believe you understand the
consequences of your patches including user impacts before sending your patches
without RFC tag.  I'd suggest you to take more time on making such
preparational confidences and/or discussions _before_ sending non-RFC patches.
You're nver lagging.  Take your time.

> 
> In the logic before this patch is applied, I think
> time_after_eq(jiffies, ...) should only evaluate to false when the MSB
> of jiffies is 1 and charged_from is 0. because if charging has
> occurred, it changes charge_from to jiffies at that time.

It is not the only case that time_after_eq() can be evaluated to false.  Maybe
you're saying only about the just-after-boot running case?  If so, please
clarify.  You and I know the context, but others may not.  I hope the commit
message be nicer for them.

> Therefore,
> esz should also be zero because it is initialized with charged_from.
> So I think the real user impact is that "quota is not applied", rather
> than "stops working". If my understanding is wrong, please let me know
> what point is wrong.

Thank you for clarifying your view.  The code is behaving in the way you
described above.  It is because damon_set_effective_quota(), which sets the
esz, is called only when the time_after_eq() call returns true.

However, this is a bug rather than an intended behavior.  The current behavior
is making the first charging window just be wasted without doing nothing.

Probably the bug was introduced by the commit that introduced esz.

> 
> > >
> > > Example 1: initial boot
> > > jiffies=0xFFFB6C20, charged_from+interval=0x000003E8
> > > time_after_eq(jiffies, charged_from+interval)=(long)0xFFFB6838; In
> > > signed values, it is considered negative so it is false.
> >
> > The above part is using hex numbers and look like psuedo-code.  This is
> > unnecessarily difficult to read.  To me, this feels like your personal note
> > rather than a nice commit message that written for others.  I think you could
> > write this in a much better way.
> >
> > >
> > > Example 2: after about 25 days first wraparound
> > > jiffies=0x800004E8, charged_from+interval=0x000003E8
> > > time_after_eq(jiffies, charged_from+interval)=(long)0x80000100; In
> > > signed values, it is considered negative so it is false
> >
> > Ditto.
> 
> Okay, I think I can fix these sections with explanation using MSB.

Also please make it easier to read for more human people.

> 
> > >
> > > So, change quota->charged_from to jiffies at damos_adjust_quota() when
> > > it is consider first charge window.
> > >
> > > In theory; but almost impossible; quota->total_charged_sz and
> > > qutoa->charged_from should be both zero even if it is not in first
> >
> > s/should/could/ ?
> 
> Sorry for my poor english.
> 
> > Also, explaining when that "could" happen will be nice.
> 
> I want to confirm this situation as well. I think the situation below
> is the only case.

Again, if there is anything unclear, let's do discussions before sending
non-RFC patches.

> 
> 1. jiffies overflows to exactly 0
> 2. And quota is configured but never actually applied, so total_charged_sz is 0

Or, total_charged_sz is also overflows and bcome 0.

> 3. And charging occurs at that exact moment.

It's not necessarily when charging occurs but when damon_adjust_quota() is
called.  More technically speaking once per the scheme's apply interval.

> 
> Is that right? If right, I think this situation is almost impossible
> and uncommon. I feel like It's unnecessary to describe it. I'm not
> trying to ignore your valuable opinion, but do you still think it's
> better to add a description?

I'm ok to completely drop the explanation.  But if you are gonna mention it
partially, please clarify.

> 
> > > charge window. But It will only delay one reset_interval, So it is not
> > > big problem.
> > >
> > > Fixes: 2b8a248d5873 ("mm/damon/schemes: implement size quota for schemes application speed control") # 5.16
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> >
> > I think the commit message could be much be improved, but the code change seems
> > right.
> 
> Once again, Sorry for my poor english. I'm doing my best on my own.

This is not about English skill but the commit "message".  Your English skill
is good and probably betetr than mine.  But I ad a difficult time at reviewing
your patch, and feeling it could been easier if the message was nicer.

So what I'm saying is that I tink this patch's commit message can be more nice
to readers.


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[Sang-Heon Jeon]

On Thu, Aug 21, 2025 at 3:27 AM SeongJae Park <sj@kernel.org> wrote:
>
> On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> > Hello, SeongJae
> >
> > On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> > >
> > > On Wed, 20 Aug 2025 00:01:23 +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))
> > > >
> > > > And they cast unsigned value to signed to cover wraparound
> > >
> > > "they" sounds bit vague.  I think "jiffies comparison helper functions" would
> > > be better.
> >
> > I agree, I will change it.
> >
> > > >
> > > >  #define time_after_eq(a,b) \
> > > >   (typecheck(unsigned long, a) && \
> > > >   typecheck(unsigned long, b) && \
> > > >   ((long)((a) - (b)) >= 0))
> > > >
> > > > In 64bit system, these might not be a problem because wrapround occurs
> > > > 300 million years after the boot, assuming HZ value is 1000.
> > > >
> > > > With same assuming, In 32bit system, wraparound occurs 5 minutues after
> > > > the initial boot and every 49 days after the first wraparound. And about
> > > > 25 days after first wraparound, it continues quota charging window up to
> > > > next 25 days.
> > >
> > > It would be nice if you can further explain what real user impacts that could
> > > make.  To my understanding the impact is that, when the unexpected extension of
> > > the charging window is happened, the scheme will work until the quota is full,
> > > but then stops working until the unexpectedly extended window is over.
> > >
> > > The after-boot issue is really bad since there is no way to work around other
> > > than reboot the machine.
> >
> > I agree with your point that user impact should be added to commit
> > messages. Before modifying the commit message, I want to check that my
> > understanding of "user impact" is correct.
>
> I think you should make clear at least you believe you understand the
> consequences of your patches including user impacts before sending your patches
> without RFC tag.  I'd suggest you to take more time on making such
> preparational confidences and/or discussions _before_ sending non-RFC patches.
> You're nver lagging.  Take your time.

I think that I checked about user impact already but it should be
insufficient. As you said, I should discuss it first. Anyway, the
whole thing is my mistake. I'm really so sorry.

So, Would it be better to send an RFC patch even now, instead of
asking on this email thread? (I'll make next v3 patch with RFC tag,
it's not question of v3 direction and just about remained question on
this email thread)

> >
> > In the logic before this patch is applied, I think
> > time_after_eq(jiffies, ...) should only evaluate to false when the MSB
> > of jiffies is 1 and charged_from is 0. because if charging has
> > occurred, it changes charge_from to jiffies at that time.
>
> It is not the only case that time_after_eq() can be evaluated to false.  Maybe
> you're saying only about the just-after-boot running case?  If so, please
> clarify.  You and I know the context, but others may not.  I hope the commit
> message be nicer for them.

I think it is not just-after-boot running case also whole and only
case, because charging changes charged_from to jiffies. if it is not
the only case, could you please describe the specific case?

> > Therefore,
> > esz should also be zero because it is initialized with charged_from.
> > So I think the real user impact is that "quota is not applied", rather
> > than "stops working". If my understanding is wrong, please let me know
> > what point is wrong.
>
> Thank you for clarifying your view.  The code is behaving in the way you
> described above.  It is because damon_set_effective_quota(), which sets the
> esz, is called only when the time_after_eq() call returns true.
>
> However, this is a bug rather than an intended behavior.  The current behavior
> is making the first charging window just be wasted without doing nothing.
>
> Probably the bug was introduced by the commit that introduced esz.

Thanks for your explanation. I'll try to cover this point in the next
patch as well.

> >
> > > >
> > > > Example 1: initial boot
> > > > jiffies=0xFFFB6C20, charged_from+interval=0x000003E8
> > > > time_after_eq(jiffies, charged_from+interval)=(long)0xFFFB6838; In
> > > > signed values, it is considered negative so it is false.
> > >
> > > The above part is using hex numbers and look like psuedo-code.  This is
> > > unnecessarily difficult to read.  To me, this feels like your personal note
> > > rather than a nice commit message that written for others.  I think you could
> > > write this in a much better way.
> > >
> > > >
> > > > Example 2: after about 25 days first wraparound
> > > > jiffies=0x800004E8, charged_from+interval=0x000003E8
> > > > time_after_eq(jiffies, charged_from+interval)=(long)0x80000100; In
> > > > signed values, it is considered negative so it is false
> > >
> > > Ditto.
> >
> > Okay, I think I can fix these sections with explanation using MSB.
>
> Also please make it easier to read for more human people.

I see.

> >
> > > >
> > > > So, change quota->charged_from to jiffies at damos_adjust_quota() when
> > > > it is consider first charge window.
> > > >
> > > > In theory; but almost impossible; quota->total_charged_sz and
> > > > qutoa->charged_from should be both zero even if it is not in first
> > >
> > > s/should/could/ ?
> >
> > Sorry for my poor english.
> >
> > > Also, explaining when that "could" happen will be nice.
> >
> > I want to confirm this situation as well. I think the situation below
> > is the only case.
>
> Again, if there is anything unclear, let's do discussions before sending
> non-RFC patches.
>
> >
> > 1. jiffies overflows to exactly 0
> > 2. And quota is configured but never actually applied, so total_charged_sz is 0
>
> Or, total_charged_sz is also overflows and bcome 0.

Thanks for clarifying me.

> > 3. And charging occurs at that exact moment.
>
> It's not necessarily when charging occurs but when damon_adjust_quota() is
> called.  More technically speaking once per the scheme's apply interval.

Ditto.

> >
> > Is that right? If right, I think this situation is almost impossible
> > and uncommon. I feel like It's unnecessary to describe it. I'm not
> > trying to ignore your valuable opinion, but do you still think it's
> > better to add a description?
>
> I'm ok to completely drop the explanation.  But if you are gonna mention it
> partially, please clarify.

I see, your opinion is reasonable. I'll keep that in my mind.

> >
> > > > charge window. But It will only delay one reset_interval, So it is not
> > > > big problem.
> > > >
> > > > Fixes: 2b8a248d5873 ("mm/damon/schemes: implement size quota for schemes application speed control") # 5.16
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> > >
> > > I think the commit message could be much be improved, but the code change seems
> > > right.
> >
> > Once again, Sorry for my poor english. I'm doing my best on my own.
>
> This is not about English skill but the commit "message".  Your English skill
> is good and probably betetr than mine.  But I ad a difficult time at reviewing
> your patch, and feeling it could been easier if the message was nicer.
>
> So what I'm saying is that I tink this patch's commit message can be more nice
> to readers.

You're right. I'll try to make the commit message more clear. I'm
really sorry for bothering you.

>
> Thanks,
> SJ
>
> [...]

Best Regards
Sang-Heon Jeon

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[SeongJae Park]

On Thu, 21 Aug 2025 10:08:03 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> On Thu, Aug 21, 2025 at 3:27 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> >
> > > Hello, SeongJae
> > >
> > > On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > >
> > > > On Wed, 20 Aug 2025 00:01:23 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
[...]
> I think that I checked about user impact already but it should be
> insufficient. As you said, I should discuss it first. Anyway, the
> whole thing is my mistake. I'm really so sorry.

Everyone makes mistakes.  You don't need to apologize.

> 
> So, Would it be better to send an RFC patch even now, instead of
> asking on this email thread? (I'll make next v3 patch with RFC tag,
> it's not question of v3 direction and just about remained question on
> this email thread)

If you unsure something and there is no reason to send a patch without a
discussion for the point, please discuss first.  To be honest I don't
understand the above question at all.

> 
> > >
> > > In the logic before this patch is applied, I think
> > > time_after_eq(jiffies, ...) should only evaluate to false when the MSB
> > > of jiffies is 1 and charged_from is 0. because if charging has
> > > occurred, it changes charge_from to jiffies at that time.
> >
> > It is not the only case that time_after_eq() can be evaluated to false.  Maybe
> > you're saying only about the just-after-boot running case?  If so, please
> > clarify.  You and I know the context, but others may not.  I hope the commit
> > message be nicer for them.
> 
> I think it is not just-after-boot running case also whole and only
> case, because charging changes charged_from to jiffies. if it is not
> the only case, could you please describe the specific case?

I don't understand the first sentence.  But...

I mean, time_after_eq() can return false for many cases including just when the
time is before.  Suppose a case that the first and the second arguments are,
say, 5000 and 7000.

> 
> > > Therefore,
> > > esz should also be zero because it is initialized with charged_from.
> > > So I think the real user impact is that "quota is not applied", rather
> > > than "stops working". If my understanding is wrong, please let me know
> > > what point is wrong.
> >
> > Thank you for clarifying your view.  The code is behaving in the way you
> > described above.  It is because damon_set_effective_quota(), which sets the
> > esz, is called only when the time_after_eq() call returns true.
> >
> > However, this is a bug rather than an intended behavior.  The current behavior
> > is making the first charging window just be wasted without doing nothing.
> >
> > Probably the bug was introduced by the commit that introduced esz.
> 
> Thanks for your explanation. I'll try to cover this point in the next
> patch as well.

If you gonna send a patch for fixing this bug, make it as a separate one,
please.

[...]
> > So what I'm saying is that I tink this patch's commit message can be more nice
> > to readers.
> 
> You're right. I'll try to make the commit message more clear. I'm
> really sorry for bothering you.

Again, you don't need to apologize.


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[Sang-Heon Jeon]

On Thu, Aug 21, 2025 at 11:54 AM SeongJae Park <sj@kernel.org> wrote:
>
> On Thu, 21 Aug 2025 10:08:03 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> > On Thu, Aug 21, 2025 at 3:27 AM SeongJae Park <sj@kernel.org> wrote:
> > >
> > > On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > >
> > > > Hello, SeongJae
> > > >
> > > > On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > > >
> > > > > On Wed, 20 Aug 2025 00:01:23 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> [...]
> > I think that I checked about user impact already but it should be
> > insufficient. As you said, I should discuss it first. Anyway, the
> > whole thing is my mistake. I'm really so sorry.
>
> Everyone makes mistakes.  You don't need to apologize.
>
> >
> > So, Would it be better to send an RFC patch even now, instead of
> > asking on this email thread? (I'll make next v3 patch with RFC tag,
> > it's not question of v3 direction and just about remained question on
> > this email thread)
>
> If you unsure something and there is no reason to send a patch without a
> discussion for the point, please discuss first.  To be honest I don't
> understand the above question at all.

Ah, I just mean that I need to make a new RFC patch instead of
replying to this email thread. I'll just keep asking about previous
comments on this email thread.

> >
> > > >
> > > > In the logic before this patch is applied, I think
> > > > time_after_eq(jiffies, ...) should only evaluate to false when the MSB
> > > > of jiffies is 1 and charged_from is 0. because if charging has
> > > > occurred, it changes charge_from to jiffies at that time.
> > >
> > > It is not the only case that time_after_eq() can be evaluated to false.  Maybe
> > > you're saying only about the just-after-boot running case?  If so, please
> > > clarify.  You and I know the context, but others may not.  I hope the commit
> > > message be nicer for them.
> >
> > I think it is not just-after-boot running case also whole and only
> > case, because charging changes charged_from to jiffies. if it is not
> > the only case, could you please describe the specific case?
>
> I don't understand the first sentence.  But...
>
> I mean, time_after_eq() can return false for many cases including just when the
> time is before.  Suppose a case that the first and the second arguments are,
> say, 5000 and 7000.

I think my previous explanation is not enough. I just want to say,
time_after_eq return false, but user expected true case; And I think
that's the point we want to fix.

Maybe I can change my previous question like this, "Is there any
situation, that charged_from has been updated before and even though
reset_interval has passed but time_after_equal() returns false".

I asked this question because I think that kind of situation can't
exist and minimum version of Fixes patch(5.16) uses esz in the same
way as it is now. So I think that we shouldn't use "stop working" in
the commit message.

As I was writing this, I thought about your comments deeply again.
Since you describe the current state of esz as a bug, I think you
might want to write "stop working" to comments, because I think you're
thinking that some fixes patch could change esz initialized value
(also reasonable, I agree)

I think adding an explanation of the above knowledge is good to help
newcomers to understand DAMON well. Also, Could you please check the
above question for a more detailed commit message?

> >
> > > > Therefore,
> > > > esz should also be zero because it is initialized with charged_from.
> > > > So I think the real user impact is that "quota is not applied", rather
> > > > than "stops working". If my understanding is wrong, please let me know
> > > > what point is wrong.
> > >
> > > Thank you for clarifying your view.  The code is behaving in the way you
> > > described above.  It is because damon_set_effective_quota(), which sets the
> > > esz, is called only when the time_after_eq() call returns true.
> > >
> > > However, this is a bug rather than an intended behavior.  The current behavior
> > > is making the first charging window just be wasted without doing nothing.
> > >
> > > Probably the bug was introduced by the commit that introduced esz.
> >
> > Thanks for your explanation. I'll try to cover this point in the next
> > patch as well.
>
> If you gonna send a patch for fixing this bug, make it as a separate one,
> please.

I didn't mean newer code changes, just commit messge. As you said code
change should be created with another patch, if it has another
intension; Also, i didn't have any plan yet. I'm trying to resolve
this patch first

> [...]
> > > So what I'm saying is that I tink this patch's commit message can be more nice
> > > to readers.
> >
> > You're right. I'll try to make the commit message more clear. I'm
> > really sorry for bothering you.
>
> Again, you don't need to apologize.

Maybe, I just want to express my gratitude :)

>
> Thanks,
> SJ
>
> [...]

Best Regards
Sang-Heon Jeon

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[Sang-Heon Jeon]

(Restore missing CC) + Andrew

On Thu, Aug 21, 2025 at 1:29 PM Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> On Thu, Aug 21, 2025 at 11:54 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > On Thu, 21 Aug 2025 10:08:03 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> >
> > > On Thu, Aug 21, 2025 at 3:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > >
> > > > On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > >
> > > > > Hello, SeongJae
> > > > >
> > > > > On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, 20 Aug 2025 00:01:23 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > [...]
> > > I think that I checked about user impact already but it should be
> > > insufficient. As you said, I should discuss it first. Anyway, the
> > > whole thing is my mistake. I'm really so sorry.
> >
> > Everyone makes mistakes.  You don't need to apologize.
> >
> > >
> > > So, Would it be better to send an RFC patch even now, instead of
> > > asking on this email thread? (I'll make next v3 patch with RFC tag,
> > > it's not question of v3 direction and just about remained question on
> > > this email thread)
> >
> > If you unsure something and there is no reason to send a patch without a
> > discussion for the point, please discuss first.  To be honest I don't
> > understand the above question at all.
>
> Ah, I just mean that I need to make a new RFC patch instead of
> replying to this email thread. I'll just keep asking about previous
> comments on this email thread.
>
> > >
> > > > >
> > > > > In the logic before this patch is applied, I think
> > > > > time_after_eq(jiffies, ...) should only evaluate to false when the MSB
> > > > > of jiffies is 1 and charged_from is 0. because if charging has
> > > > > occurred, it changes charge_from to jiffies at that time.
> > > >
> > > > It is not the only case that time_after_eq() can be evaluated to false.  Maybe
> > > > you're saying only about the just-after-boot running case?  If so, please
> > > > clarify.  You and I know the context, but others may not.  I hope the commit
> > > > message be nicer for them.
> > >
> > > I think it is not just-after-boot running case also whole and only
> > > case, because charging changes charged_from to jiffies. if it is not
> > > the only case, could you please describe the specific case?
> >
> > I don't understand the first sentence.  But...
> >
> > I mean, time_after_eq() can return false for many cases including just when the
> > time is before.  Suppose a case that the first and the second arguments are,
> > say, 5000 and 7000.
>
> I think my previous explanation is not enough. I just want to say,
> time_after_eq return false, but user expected true case; And I think
> that's the point we want to fix.
>
> Maybe I can change my previous question like this, "Is there any
> situation, that charged_from has been updated before and even though
> reset_interval has passed but time_after_equal() returns false".
>
> I asked this question because I think that kind of situation can't
> exist and minimum version of Fixes patch(5.16) uses esz in the same
> way as it is now. So I think that we shouldn't use "stop working" in
> the commit message.
>
> As I was writing this, I thought about your comments deeply again.
> Since you describe the current state of esz as a bug, I think you
> might want to write "stop working" to comments, because I think you're
> thinking that some fixes patch could change esz initialized value
> (also reasonable, I agree)
>
> I think adding an explanation of the above knowledge is good to help
> newcomers to understand DAMON well. Also, Could you please check the
> above question for a more detailed commit message?
>
> > >
> > > > > Therefore,
> > > > > esz should also be zero because it is initialized with charged_from.
> > > > > So I think the real user impact is that "quota is not applied", rather
> > > > > than "stops working". If my understanding is wrong, please let me know
> > > > > what point is wrong.
> > > >
> > > > Thank you for clarifying your view.  The code is behaving in the way you
> > > > described above.  It is because damon_set_effective_quota(), which sets the
> > > > esz, is called only when the time_after_eq() call returns true.
> > > >
> > > > However, this is a bug rather than an intended behavior.  The current behavior
> > > > is making the first charging window just be wasted without doing nothing.
> > > >
> > > > Probably the bug was introduced by the commit that introduced esz.
> > >
> > > Thanks for your explanation. I'll try to cover this point in the next
> > > patch as well.
> >
> > If you gonna send a patch for fixing this bug, make it as a separate one,
> > please.
>
> I didn't mean newer code changes, just commit messge. As you said code
> change should be created with another patch, if it has another
> intension; Also, i didn't have any plan yet. I'm trying to resolve
> this patch first
>
> > [...]
> > > > So what I'm saying is that I tink this patch's commit message can be more nice
> > > > to readers.
> > >
> > > You're right. I'll try to make the commit message more clear. I'm
> > > really sorry for bothering you.
> >
> > Again, you don't need to apologize.
>
> Maybe, I just want to express my gratitude :)
>
> >
> > Thanks,
> > SJ
> >
> > [...]
>
> Best Regards
> Sang-Heon Jeon

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[SeongJae Park]

On Thu, 21 Aug 2025 13:29:04 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> On Thu, Aug 21, 2025 at 11:54 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > On Thu, 21 Aug 2025 10:08:03 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> >
> > > On Thu, Aug 21, 2025 at 3:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > >
> > > > On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > >
> > > > > Hello, SeongJae
> > > > >
> > > > > On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, 20 Aug 2025 00:01:23 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > [...]
> > > I think that I checked about user impact already but it should be
> > > insufficient. As you said, I should discuss it first. Anyway, the
> > > whole thing is my mistake. I'm really so sorry.
> >
> > Everyone makes mistakes.  You don't need to apologize.
> >
> > >
> > > So, Would it be better to send an RFC patch even now, instead of
> > > asking on this email thread? (I'll make next v3 patch with RFC tag,
> > > it's not question of v3 direction and just about remained question on
> > > this email thread)
> >
> > If you unsure something and there is no reason to send a patch without a
> > discussion for the point, please discuss first.  To be honest I don't
> > understand the above question at all.
> 
> Ah, I just mean that I need to make a new RFC patch instead of
> replying to this email thread.

Why?

> I'll just keep asking about previous
> comments on this email thread.

You said you will send a new patch instead of replying here, and then now you
are saying you will keep aking to this thread.

I'm confused.

[...]

Ok, I think this discussion is going unnecessarily deep and only wasting our
time at clarifying intention of past sentence.  Meanwhile apparently you
understood my major points.  To repeat, please clarify what is the problem and
user impacts, when and how it happens, and why the solution solves it.

Let's restart.  Could you please rewrite the commit log for this patch and send
the draft as a reply to this?

We can further discuss on the new draft if it has more things to improve.  And
once the discussion is finalized, you can post v4 of this patch with the
updated commit message.


Thanks,
SJ

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[SeongJae Park]

On Wed, 20 Aug 2025 22:41:48 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Thu, 21 Aug 2025 13:29:04 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> 
> > On Thu, Aug 21, 2025 at 11:54 AM SeongJae Park <sj@kernel.org> wrote:
> > >
> > > On Thu, 21 Aug 2025 10:08:03 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > >
> > > > On Thu, Aug 21, 2025 at 3:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > > >
> > > > > On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > > >
> > > > > > Hello, SeongJae
> > > > > >
> > > > > > On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed, 20 Aug 2025 00:01:23 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > [...]
> > > > I think that I checked about user impact already but it should be
> > > > insufficient. As you said, I should discuss it first. Anyway, the
> > > > whole thing is my mistake. I'm really so sorry.
> > >
> > > Everyone makes mistakes.  You don't need to apologize.
> > >
> > > >
> > > > So, Would it be better to send an RFC patch even now, instead of
> > > > asking on this email thread? (I'll make next v3 patch with RFC tag,
> > > > it's not question of v3 direction and just about remained question on
> > > > this email thread)
> > >
> > > If you unsure something and there is no reason to send a patch without a
> > > discussion for the point, please discuss first.  To be honest I don't
> > > understand the above question at all.
> > 
> > Ah, I just mean that I need to make a new RFC patch instead of
> > replying to this email thread.
> 
> Why?
> 
> > I'll just keep asking about previous
> > comments on this email thread.
> 
> You said you will send a new patch instead of replying here, and then now you
> are saying you will keep aking to this thread.
> 
> I'm confused.
> 
> [...]
> 
> Ok, I think this discussion is going unnecessarily deep and only wasting our
> time at clarifying intention of past sentence.  Meanwhile apparently you
> understood my major points.  To repeat, please clarify what is the problem and
> user impacts, when and how it happens, and why the solution solves it.
> 
> Let's restart.  Could you please rewrite the commit log for this patch and send
> the draft as a reply to this?
> 
> We can further discuss on the new draft if it has more things to improve.  And
> once the discussion is finalized, you can post v4 of this patch with the
> updated commit message.

s/v4/v3/

Sorry for the typo.


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[Sang-Heon Jeon]

On Thu, Aug 21, 2025 at 2:41 PM SeongJae Park <sj@kernel.org> wrote:
>
> On Thu, 21 Aug 2025 13:29:04 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> > On Thu, Aug 21, 2025 at 11:54 AM SeongJae Park <sj@kernel.org> wrote:
> > >
> > > On Thu, 21 Aug 2025 10:08:03 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > >
> > > > On Thu, Aug 21, 2025 at 3:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > > >
> > > > > On Wed, 20 Aug 2025 22:18:53 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > > >
> > > > > > Hello, SeongJae
> > > > > >
> > > > > > On Wed, Aug 20, 2025 at 2:27 AM SeongJae Park <sj@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed, 20 Aug 2025 00:01:23 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > [...]
> > > > I think that I checked about user impact already but it should be
> > > > insufficient. As you said, I should discuss it first. Anyway, the
> > > > whole thing is my mistake. I'm really so sorry.
> > >
> > > Everyone makes mistakes.  You don't need to apologize.
> > >
> > > >
> > > > So, Would it be better to send an RFC patch even now, instead of
> > > > asking on this email thread? (I'll make next v3 patch with RFC tag,
> > > > it's not question of v3 direction and just about remained question on
> > > > this email thread)
> > >
> > > If you unsure something and there is no reason to send a patch without a
> > > discussion for the point, please discuss first.  To be honest I don't
> > > understand the above question at all.
> >
> > Ah, I just mean that I need to make a new RFC patch instead of
> > replying to this email thread.
>
> Why?
>
> > I'll just keep asking about previous
> > comments on this email thread.
>
> You said you will send a new patch instead of replying here, and then now you
> are saying you will keep aking to this thread.
>
> I'm confused.

I think I miscommunicated something. I'll just follow your suggestion
below. it's the same as my first thought (discussion here, new patch
after discussion finished)

> [...]
>
> Ok, I think this discussion is going unnecessarily deep and only wasting our
> time at clarifying intention of past sentence.  Meanwhile apparently you
> understood my major points.  To repeat, please clarify what is the problem and
> user impacts, when and how it happens, and why the solution solves it.
>
> Let's restart.  Could you please rewrite the commit log for this patch and send
> the draft as a reply to this?
>
> We can further discuss on the new draft if it has more things to improve.  And
> once the discussion is finalized, you can post v4 of this patch with the
> updated commit message.

Good Idea. This is the draft for commit message. Also, Thank you for
your patience and understanding.

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

And jiffies comparison help functions cast unsigned value to signed to
cover wraparound

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

When quota->charged_from is initialized to 0, time_after_eq() can incorrectly
return FALSE even after reset_interval has elapsed. This occurs when
(jiffies - reset_interval) produces a value with MSB=1, which is interpreted
as negative in signed arithmetic.

This issue primarily affects 32-bit systems because:
On 64-bit systems: MSB=1 values occur after ~292 million years from boot
(assuming HZ=1000), almost impossible.

On 32-bit systems: MSB=1 values occur during the first 5 minutes after boot,
and the second half of every jiffies wraparound cycle, starting from day 25
(assuming HZ=1000)

When above unexpected FALSE return from time_after_eq() occurs, the
charging window will not reset. The user impact depends on esz value
at that time.

If esz is 0, scheme ignores configured quotas and runs without any
limits.

If esz is not 0, scheme stops working once the quota is exhausted. It
remains until the charging window finally resets.

So, change quota->charged_from to jiffies at damos_adjust_quota() when
it is considered as the first charge window. By this change, we can avoid
unexpected FALSE return from time_after_eq()


>
> Thanks,
> SJ

Best Regards
Sang-Heon Jeon

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[SeongJae Park]

On Thu, 21 Aug 2025 20:06:58 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:

> On Thu, Aug 21, 2025 at 2:41 PM SeongJae Park <sj@kernel.org> wrote:
[...]
> > Let's restart.  Could you please rewrite the commit log for this patch and send
> > the draft as a reply to this?
> >
> > We can further discuss on the new draft if it has more things to improve.  And
> > once the discussion is finalized, you can post v4 of this patch with the
> > updated commit message.
> 
> Good Idea. This is the draft for commit message. Also, Thank you for
> your patience and understanding.

Thank you for accepting my humble suggestion.

> 
> 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))
> 
> And jiffies comparison help functions cast unsigned value to signed to
> cover wraparound
> 
>  #define time_after_eq(a,b) \
>   (typecheck(unsigned long, a) && \
>   typecheck(unsigned long, b) && \
>   ((long)((a) - (b)) >= 0))
> 
> When quota->charged_from is initialized to 0, time_after_eq() can incorrectly
> return FALSE even after reset_interval has elapsed. This occurs when
> (jiffies - reset_interval) produces a value with MSB=1, which is interpreted
> as negative in signed arithmetic.
> 
> This issue primarily affects 32-bit systems because:
> On 64-bit systems: MSB=1 values occur after ~292 million years from boot
> (assuming HZ=1000), almost impossible.
> 
> On 32-bit systems: MSB=1 values occur during the first 5 minutes after boot,
> and the second half of every jiffies wraparound cycle, starting from day 25
> (assuming HZ=1000)
> 
> When above unexpected FALSE return from time_after_eq() occurs, the
> charging window will not reset. The user impact depends on esz value
> at that time.
> 
> If esz is 0, scheme ignores configured quotas and runs without any
> limits.
> 
> If esz is not 0, scheme stops working once the quota is exhausted. It
> remains until the charging window finally resets.
> 
> So, change quota->charged_from to jiffies at damos_adjust_quota() when
> it is considered as the first charge window. By this change, we can avoid
> unexpected FALSE return from time_after_eq()

This new draft looks good to me.  I find nothing to further modify.  Could you
please send v3 of this patch with the above commit log?


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: set quota->charged_from to jiffies at first charge window

[Sang-Heon Jeon]

On Fri, Aug 22, 2025 at 12:58 AM SeongJae Park <sj@kernel.org> wrote:
>
> On Thu, 21 Aug 2025 20:06:58 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> > On Thu, Aug 21, 2025 at 2:41 PM SeongJae Park <sj@kernel.org> wrote:
> [...]
> > > Let's restart.  Could you please rewrite the commit log for this patch and send
> > > the draft as a reply to this?
> > >
> > > We can further discuss on the new draft if it has more things to improve.  And
> > > once the discussion is finalized, you can post v4 of this patch with the
> > > updated commit message.
> >
> > Good Idea. This is the draft for commit message. Also, Thank you for
> > your patience and understanding.
>
> Thank you for accepting my humble suggestion.
>
> >
> > 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))
> >
> > And jiffies comparison help functions cast unsigned value to signed to
> > cover wraparound
> >
> >  #define time_after_eq(a,b) \
> >   (typecheck(unsigned long, a) && \
> >   typecheck(unsigned long, b) && \
> >   ((long)((a) - (b)) >= 0))
> >
> > When quota->charged_from is initialized to 0, time_after_eq() can incorrectly
> > return FALSE even after reset_interval has elapsed. This occurs when
> > (jiffies - reset_interval) produces a value with MSB=1, which is interpreted
> > as negative in signed arithmetic.
> >
> > This issue primarily affects 32-bit systems because:
> > On 64-bit systems: MSB=1 values occur after ~292 million years from boot
> > (assuming HZ=1000), almost impossible.
> >
> > On 32-bit systems: MSB=1 values occur during the first 5 minutes after boot,
> > and the second half of every jiffies wraparound cycle, starting from day 25
> > (assuming HZ=1000)
> >
> > When above unexpected FALSE return from time_after_eq() occurs, the
> > charging window will not reset. The user impact depends on esz value
> > at that time.
> >
> > If esz is 0, scheme ignores configured quotas and runs without any
> > limits.
> >
> > If esz is not 0, scheme stops working once the quota is exhausted. It
> > remains until the charging window finally resets.
> >
> > So, change quota->charged_from to jiffies at damos_adjust_quota() when
> > it is considered as the first charge window. By this change, we can avoid
> > unexpected FALSE return from time_after_eq()
>
> This new draft looks good to me.  I find nothing to further modify.  Could you
> please send v3 of this patch with the above commit log?

I'm really glad to hear that! Your thoughtful comments really brought
this together.

>
> Thanks,
> SJ
>
> [...]

Best Regards
Sang-Heon Jeon