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

[SeongJae Park <sj@kernel.org>]

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

[...]