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

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

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