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

[SeongJae Park <sj@kernel.org>]

+ Andrew

On Tue, 19 Aug 2025 10:27:18 -0700 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.
> 
> > 
> >  #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.
> 
> > 
> > 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.
> 
> > 
> > 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/ ?
> 
> Also, explaining when that "could" happen will be nice.
> 
> > 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.
> 
> 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
> 
> [...]