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

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

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