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("a->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))) {
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
[...]
+ 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
[...]
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
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.
- jiffies overflows to exactly 0
- And quota is configured but never actually applied, so total_charged_sz is 0
Or, total_charged_sz is also overflows and bcome 0.
- 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
[...]
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.
- jiffies overflows to exactly 0
- 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.
- 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
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
[...]
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
(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
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
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
[...]
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
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
[...]
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
linux-stable-mirror@lists.linaro.org
-
Sang-Heon Jeon -
SeongJae Park