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