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