Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero

[SJ Park <sj@kernel.org>]

On Thu,  3 Sep 2026 06:31:38 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> On Wed, 02 Sep 2026 07:48:03 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > On Wed,  2 Sep 2026 22:22:31 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > 
> > > On Wed, 02 Sep 2026 07:10:00 -0700 SJ Park <sj@kernel.org> wrote:
> > > 
> > > > On Wed,  2 Sep 2026 16:44:45 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > > > 
> > > > > When setting goal_tuner to 'temporal', if the goal is achieved,
> > > > > quota->esz_bp will be set to zero.  In this case, damos_quota_is_full()
> > > > > will always return true, even if no regions have been tried to apply at
> > > > > all.  This causes qt_exceeds to increase unexpectedly.
> > > > 
> > > > To me, this looks logically correct.  Could this cause any user issue?
> > > 
> > > Yes, I think this might cause unnecessary confusion for users,
> > 
> > If it is _might_ kind thing, please ask questions first, or add RFC tag at
> > least.
> > 
> > > at least
> > > it did for me.
> > 
> > If it confused you, definitely we need to fix it.  But if it is just confusing,
> > let's try to clarify by adding documentation, rather than changing existing
> > behaviors.
> 
> I agree that we should avoid unnecessary code changes if the issue is
> merely a matter of confusion.
> 
> However, I believe a code change is necessary in this specific case,
> rather than just adding documentation, for the following reasons:
> 
> First, this distorts the semantics of qt_exceeds.  It is a statistic
> meant to track actual quota full/overflows.  Incrementing qt_exceeds
> when the quota is completely unused (because the temporal goal is
> already met) misleads users and could break user-space monitoring tools
> or scripts that rely on this metric's accuracy.

As I mentioned in the previous comment, this doesn't look wrong to me.  I
understand some people could think differently.  That's why I think this is a
matter of clarification, not a bug to fix.

> 
> Second, considering its original behavior when it was first introduced,
> the recent changes [2] have inadvertently altered this behavior.
> 
> When qt_exceeds was first introduced [1], the conditional check was:
> 
>     if (quota->esz && quota->charged_sz >= quota->esz)
>             s->stat.qt_exceeds++;
> 
> Later, to handle cases where 'remaining quota < min_region_sz',
> damos_quota_is_full() was added [2].  When this function was added,
> quota->esz was replaced with !damos_quota_is_set(quota).

I don't find what's wrong with this.

> 
>     static bool damos_quota_is_full(struct damos_quota *quota,
>             unsigned long min_region_sz)
>     {
>         if (!damos_quota_is_set(quota))
>             return false;
>         if (quota->charged_sz >= quota->esz)
>             return true;
>         /*
>          * DAMOS action is applied per region, so <min_region_sz remaining
>          * quota means the quota is effectively full.
>          */
>         return quota->esz - quota->charged_sz < min_region_sz;
>     }
> 
> With the introduction of DAMOS_QUOTA_GOAL_TUNER_TEMPORAL [3], quota->
> esz == 0 carries two meanings: "unlimited quota" and "should not
> be applied".

I don't find how you think this way.

> This overlap causes the damos_quota_is_full() check to
> return true incorrectly in the temporal goal scenario, leading to the
> false positive in the statistic.

That still sounds like just a matter of different interpretations.  Let me know
if I'm missing something.

> 
> > 
> > I feel like we also found similar case that people send patches to change some
> > behaviors that confusing them, but not really problematic.  That sometimes
> > waste unnecessary time for understanding the intention of the patch.  Please
> > feel free to ask question first, if you found some confusing behaviors and
> > don't know why it behaves in the way or if it is a real problem.
> 
> I will ask questions on the first in the future when I encounter
> ambiguous behaviors, to ensure we are aligned on whether it's a real
> problem before drafting a patch.

Thanks.

> 
> [1] 6268eac34ca30 ("mm/damon/schemes: account how many times quota limit has exceeded")
>     (Fri Jan 14 14:10:202022 -0800)
> [2] c7ec7d5f6b3d1 ("mm/damon/core: handle <min_region_sz remaining quota as empty")
>     (Mon Apr 27 18:33:50 2026 -0700)
> [3] af738a6a00c1f ("mm/damon/core: introduce DAMOS_QUOTA_GOAL_TUNER_TEMPORAL")
>     (Mon Mar 9 18:05:19 2026 -0700)
> 
> Best regards,
> Rui Yan


Thanks,
SJ