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

[Sang-Heon Jeon <ekffu200098@gmail.com>]

(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