damon.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
@ 2026-09-02  8:44 Liew Rui Yan
  2026-09-02  8:53 ` sashiko-bot
  2026-09-02 14:10 ` SJ Park
  0 siblings, 2 replies; 13+ messages in thread
From: Liew Rui Yan @ 2026-09-02  8:44 UTC (permalink / raw)
  To: SJ Park; +Cc: Andrew Morton, damon, linux-mm, linux-kernel, Liew Rui Yan,
	stable

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.

Fix it by:

- Returning false in damos_quota_is_full() when quota->charged_sz is
  zero.
- Adding an early exit in damon_do_apply_schemes() for schemes with zero
  esz.

Fixes: c7ec7d5f6b3d ("mm/damon/core: handle <min_region_sz remaining quota as empty")
Cc: <stable@vger.kernel.org> # v7.2.x
Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---

Changes from v2:
- v2: https://lore.kernel.org/damon/20260902081825.18931-1-aethernet65535@gmail.com
- Changed patch title, v2 title is truncated.

Changes from v1:
- v1: https://lore.kernel.org/damon/20260901105925.106186-1-aethernet65535@gmail.com
- Changed the check in damos_quota_is_full() from '!quota->esz' to
  '!quota->charged_sz', because the former is an upper bound, while the
  latter represents the actual quota usage.
- Added an early exit in damon_do_apply_schemes() to prevent a scheme
  with zero esz from being evaluated by damos_quota_is_full().
- Changed patch title, original title: always return false when esz is
  zero

---
 mm/damon/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 8e3ded1a2cf6..de78ad56c8b3 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2560,6 +2560,8 @@ static bool damos_quota_is_full(struct damos_quota *quota,
 {
 	if (!damos_quota_is_set(quota))
 		return false;
+	if (!quota->charged_sz)
+		return false;
 	if (quota->charged_sz >= quota->esz)
 		return true;
 	/*
@@ -2666,6 +2668,9 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (!s->wmarks.activated)
 			continue;
 
+		if (!quota->esz)
+			continue;
+
 		/* Check the quota */
 		if (damos_quota_is_full(quota, c->min_region_sz))
 			continue;
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-02  8:44 [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero Liew Rui Yan
@ 2026-09-02  8:53 ` sashiko-bot
  2026-09-02  9:42   ` Liew Rui Yan
  2026-09-02 14:10 ` SJ Park
  1 sibling, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-09-02  8:53 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: damon

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Schemes without configured quota limits (unlimited quotas) are completely skipped and never applied due to the new `!quota->esz` early exit check.
- [Medium] Incorrect quota evaluation for very small quotas (`0 < esz < min_region_sz`) breaks the early short-circuit optimization, causing wasted CPU cycles, artificially inflated statistics, and broken telemetry.
--

commit 4718f859e447852ff19b6830ab8bf79106ad37d1
Author: Liew Rui Yan <aethernet65535@gmail.com>
Subject: mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero

This patch aims to fix an issue where damos_quota_is_full() returns true when
quota->esz_bp drops to zero, even if no regions have been tried. It attempts
to resolve this by adding early checks for zero quota->charged_sz and
zero quota->esz.

Cc: <stable@vger.kernel.org> # v7.2.x

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index df22621765911..f8c784df24600 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2655,6 +2655,8 @@ static bool damos_quota_is_full(struct damos_quota *quota,
>  {
>  	if (!damos_quota_is_set(quota))
>  		return false;
> +	if (!quota->charged_sz)
> +		return false;

[Severity: Medium]
Could this cause an evaluation loop that degrades performance when the
configured quota is very small?

If a quota is configured such that 0 < quota->esz < min_region_sz,
quota->charged_sz starts at 0 at the beginning of the quota window.
This new check will return false, bypassing the subsequent small remaining
quota check:

    return quota->esz - quota->charged_sz < min_region_sz;

This falsely signals available quota. Later, when damon_do_apply_schemes()
calls damos_apply_scheme(), the size is calculated as:

    sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz);

Because esz < min_region_sz and charged_sz is 0, sz evaluates to 0. The code
then jumps to update_stat, which increments nr_tried but does not increase
charged_sz.

Since charged_sz remains 0, the next region will also evaluate to 0 and skip
charging. Won't this loop wastefully over every region in an O(N) fashion,
artificially inflating the nr_tried statistics and breaking the qt_exceeds
telemetry?

>  	if (quota->charged_sz >= quota->esz)
>  		return true;
>  	/*
> @@ -2750,6 +2752,9 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
>  		if (!s->wmarks.activated)
>  			continue;
>  
> +		if (!quota->esz)
> +			continue;
> +

[Severity: High]
Does this unconditional check completely skip schemes that do not have quota
limits configured?

For unlimited schemes (schemes without configured quotas), damos_adjust_quota()
returns early without calculating esz, leaving quota->esz initialized to 0.
With this check, won't unlimited schemes silently fail to apply entirely
because the loop unconditionally continues?

>  		/* Check the quota */
>  		if (damos_quota_is_full(quota, c->min_region_sz))
>  			continue;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084445.27170-1-aethernet65535@gmail.com?part=1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-02  8:53 ` sashiko-bot
@ 2026-09-02  9:42   ` Liew Rui Yan
  0 siblings, 0 replies; 13+ messages in thread
From: Liew Rui Yan @ 2026-09-02  9:42 UTC (permalink / raw)
  To: sashiko-bot; +Cc: aethernet65535, damon, sashiko-reviews

There is no fundamental difference between the Sashiko report in the v2
thread [1] and the Sashiko report in this thread, so I did not foward it.

On Wed, 02 Sep 2026 08:53:35 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Schemes without configured quota limits (unlimited quotas) are completely skipped and never applied due to the new `!quota->esz` early exit check.
> - [Medium] Incorrect quota evaluation for very small quotas (`0 < esz < min_region_sz`) breaks the early short-circuit optimization, causing wasted CPU cycles, artificially inflated statistics, and broken telemetry.
> --
> 
> commit 4718f859e447852ff19b6830ab8bf79106ad37d1
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> Subject: mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
> 
> This patch aims to fix an issue where damos_quota_is_full() returns true when
> quota->esz_bp drops to zero, even if no regions have been tried. It attempts
> to resolve this by adding early checks for zero quota->charged_sz and
> zero quota->esz.
> 
> Cc: <stable@vger.kernel.org> # v7.2.x
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index df22621765911..f8c784df24600 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2655,6 +2655,8 @@ static bool damos_quota_is_full(struct damos_quota *quota,
> >  {
> >  	if (!damos_quota_is_set(quota))
> >  		return false;
> > +	if (!quota->charged_sz)
> > +		return false;
> 
> [Severity: Medium]
> Could this cause an evaluation loop that degrades performance when the
> configured quota is very small?
> 
> If a quota is configured such that 0 < quota->esz < min_region_sz,
> quota->charged_sz starts at 0 at the beginning of the quota window.
> This new check will return false, bypassing the subsequent small remaining
> quota check:
> 
>     return quota->esz - quota->charged_sz < min_region_sz;
> 
> This falsely signals available quota. Later, when damon_do_apply_schemes()
> calls damos_apply_scheme(), the size is calculated as:
> 
>     sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz);
> 
> Because esz < min_region_sz and charged_sz is 0, sz evaluates to 0. The code
> then jumps to update_stat, which increments nr_tried but does not increase
> charged_sz.
> 
> Since charged_sz remains 0, the next region will also evaluate to 0 and skip
> charging. Won't this loop wastefully over every region in an O(N) fashion,
> artificially inflating the nr_tried statistics and breaking the qt_exceeds
> telemetry?

Thank you for pointing this out!

So the issue is that if 0 < esz < min_region_sz, then
damos_apply_scheme() might incorrectly increment nr_tried.

I believe the solution should be to ensure that esz is never less than
min_region_sz if esz is not zero.  However, this may cause unexpected
behavior when users set quota_sz, so additional documentation may be
needed.

That said, this is already out of scope.

> 
> >  	if (quota->charged_sz >= quota->esz)
> >  		return true;
> >  	/*
> > @@ -2750,6 +2752,9 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
> >  		if (!s->wmarks.activated)
> >  			continue;
> >  
> > +		if (!quota->esz)
> > +			continue;
> > +
> 
> [Severity: High]
> Does this unconditional check completely skip schemes that do not have quota
> limits configured?
> 
> For unlimited schemes (schemes without configured quotas), damos_adjust_quota()
> returns early without calculating esz, leaving quota->esz initialized to 0.
> With this check, won't unlimited schemes silently fail to apply entirely
> because the loop unconditionally continues?

Yes, thank you for pointing this out!

So, this check should be
'if (damos_quota_is_set(quota) && !quota->esz)'.

> 
> >  		/* Check the quota */
> >  		if (damos_quota_is_full(quota, c->min_region_sz))
> >  			continue;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260902084445.27170-1-aethernet65535@gmail.com?part=1
> 

[1] https://lore.kernel.org/damon/20260902082931.4536F1F000E9@smtp.kernel.org

Best regards,
Rui Yan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-02  8:44 [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero Liew Rui Yan
  2026-09-02  8:53 ` sashiko-bot
@ 2026-09-02 14:10 ` SJ Park
  2026-09-02 14:22   ` Liew Rui Yan
  1 sibling, 1 reply; 13+ messages in thread
From: SJ Park @ 2026-09-02 14:10 UTC (permalink / raw)
  To: Liew Rui Yan
  Cc: SJ Park, Andrew Morton, damon, linux-mm, linux-kernel, stable

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?


Thanks,
SJ

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-02 14:10 ` SJ Park
@ 2026-09-02 14:22   ` Liew Rui Yan
  2026-09-02 14:48     ` SJ Park
  0 siblings, 1 reply; 13+ messages in thread
From: Liew Rui Yan @ 2026-09-02 14:22 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, akpm, damon, linux-kernel, linux-mm, stable

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, at least
it did for me.

I set quota_ms to 0 and set the target_metric for the quota goal to
user_input (i.e., MemAvailable).  In my use case, this caused
esz_bp to be 0 most of the time.  Also, because the quota->ms clamp
was missing, esz wasn’t set to min_region_sz, it was actually set to
0.

As a result, nr_tried is 0 (because damos_quota_is_full() in
damon_do_apply_scheme() always returns true), but qt_exceeds
increments once per second (likely because reset_interval_ms defaults
to 1000ms / 1s).

Best regards,
Rui Yan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-02 14:22   ` Liew Rui Yan
@ 2026-09-02 14:48     ` SJ Park
  2026-09-02 22:31       ` Liew Rui Yan
  0 siblings, 1 reply; 13+ messages in thread
From: SJ Park @ 2026-09-02 14:48 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, akpm, damon, linux-kernel, linux-mm, stable

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 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.


Thanks,
SJ

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-02 14:48     ` SJ Park
@ 2026-09-02 22:31       ` Liew Rui Yan
  2026-09-03  0:33         ` SJ Park
  0 siblings, 1 reply; 13+ messages in thread
From: Liew Rui Yan @ 2026-09-02 22:31 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, akpm, damon, linux-kernel, linux-mm, stable

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.

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).

    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".  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.

> 
> 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.

[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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-02 22:31       ` Liew Rui Yan
@ 2026-09-03  0:33         ` SJ Park
  2026-09-03 12:41           ` Liew Rui Yan
  0 siblings, 1 reply; 13+ messages in thread
From: SJ Park @ 2026-09-03  0:33 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, akpm, damon, linux-kernel, linux-mm, stable

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-03  0:33         ` SJ Park
@ 2026-09-03 12:41           ` Liew Rui Yan
  2026-09-03 14:05             ` SJ Park
  0 siblings, 1 reply; 13+ messages in thread
From: Liew Rui Yan @ 2026-09-03 12:41 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, akpm, damon, linux-kernel, linux-mm, stable

On Wed, 02 Sep 2026 17:33:50 -0700 SJ Park <sj@kernel.org> wrote:

> 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.

Thank you for keeping this discussion going!

First, I would like to clarify my intention to avoid any
misunderstanding.  My actual goal is to fix the semantic of the
qt_exceeds statistic, rather than necessarily changing the underlying
logic of damos_quota_is_full().

Currently, there is an issue with how qt_exceeds is incremented.  When
the quota is set very small, qt_exceeds increases frequently.  This
produces a statistical trend that looks almost identical to the
continuous increments caused by the Temporal Goal being achieved.

The original intent of introducing qt_exceeds is to let users easily
notice if the quota is too small.

Commit Messages [1]:

    mm/damon/schemes: account how many times quota limit has exceeded

    If the time/space quotas of a given DAMON-based operation scheme is too
    small, the scheme could show unexpectedly slow progress.  However, there
    is no good way to notice the case in runtime.  This commit extends the
    DAMOS stat to provide how many times the quota limits exceeded so that
    the users can easily notice the case and tune the scheme.

However, under the current behavior, users are forced to manually ignore
or filter out the qt_exceeds increments that occur after the Temporal
Goal is achieved.  This adds an unnecessary burden to the users and
contradicts the core goal of making it "easy" for them to tune the
scheme.

If we agree that this adds unnecessary burden, perhaps we could explore
a minimal fix at the point where qt_exceeds is incremented, to skip the
count when the Temporal Goal is already achieved, without touching
damos_quota_is_full().

Please let me know if I'm misunderstanding anything or missing some
context!

[...]

> > [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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-03 12:41           ` Liew Rui Yan
@ 2026-09-03 14:05             ` SJ Park
  2026-09-04  8:07               ` Liew Rui Yan
  0 siblings, 1 reply; 13+ messages in thread
From: SJ Park @ 2026-09-03 14:05 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, akpm, damon, linux-kernel, linux-mm, stable

On Thu,  3 Sep 2026 20:41:48 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> On Wed, 02 Sep 2026 17:33:50 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > On Thu,  3 Sep 2026 06:31:38 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
[...]
> First, I would like to clarify my intention to avoid any
> misunderstanding.  My actual goal is to fix the semantic of the
> qt_exceeds statistic, rather than necessarily changing the underlying
> logic of damos_quota_is_full().
> 
> Currently, there is an issue with how qt_exceeds is incremented.  When
> the quota is set very small, qt_exceeds increases frequently.  This
> produces a statistical trend that looks almost identical to the
> continuous increments caused by the Temporal Goal being achieved.
> 
> The original intent of introducing qt_exceeds is to let users easily
> notice if the quota is too small.
> 
> Commit Messages [1]:
> 
>     mm/damon/schemes: account how many times quota limit has exceeded
> 
>     If the time/space quotas of a given DAMON-based operation scheme is too
>     small, the scheme could show unexpectedly slow progress.  However, there
>     is no good way to notice the case in runtime.  This commit extends the
>     DAMOS stat to provide how many times the quota limits exceeded so that
>     the users can easily notice the case and tune the scheme.
> 
> However, under the current behavior, users are forced to manually ignore
> or filter out the qt_exceeds increments that occur after the Temporal
> Goal is achieved.  This adds an unnecessary burden to the users and
> contradicts the core goal of making it "easy" for them to tune the
> scheme.

Still I feel the problem is unclear.  Why the users need to manually ignore or
filter out the increments under what situation?  Knowing specific and detailed
case would be helpful.  Are you or some people you know doing that and feeling
it is too much?  If so, what is the real use case?  For what purpose and how
DAMON is being used?  Why and how the ignorance of qt_exceeds is being done and
how painful it is?


Thanks,
SJ

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-03 14:05             ` SJ Park
@ 2026-09-04  8:07               ` Liew Rui Yan
  2026-09-04 14:05                 ` SJ Park
  0 siblings, 1 reply; 13+ messages in thread
From: Liew Rui Yan @ 2026-09-04  8:07 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, akpm, damon, linux-kernel, linux-mm, stable

On Thu, 03 Sep 2026 07:05:20 -0700 SJ Park <sj@kernel.org> wrote:

> On Thu,  3 Sep 2026 20:41:48 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> 
> > On Wed, 02 Sep 2026 17:33:50 -0700 SJ Park <sj@kernel.org> wrote:
> > 
> > > On Thu,  3 Sep 2026 06:31:38 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> [...]
> > First, I would like to clarify my intention to avoid any
> > misunderstanding.  My actual goal is to fix the semantic of the
> > qt_exceeds statistic, rather than necessarily changing the underlying
> > logic of damos_quota_is_full().
> > 
> > Currently, there is an issue with how qt_exceeds is incremented.  When
> > the quota is set very small, qt_exceeds increases frequently.  This
> > produces a statistical trend that looks almost identical to the
> > continuous increments caused by the Temporal Goal being achieved.
> > 
> > The original intent of introducing qt_exceeds is to let users easily
> > notice if the quota is too small.
> > 
> > Commit Messages [1]:
> > 
> >     mm/damon/schemes: account how many times quota limit has exceeded
> > 
> >     If the time/space quotas of a given DAMON-based operation scheme is too
> >     small, the scheme could show unexpectedly slow progress.  However, there
> >     is no good way to notice the case in runtime.  This commit extends the
> >     DAMOS stat to provide how many times the quota limits exceeded so that
> >     the users can easily notice the case and tune the scheme.
> > 
> > However, under the current behavior, users are forced to manually ignore
> > or filter out the qt_exceeds increments that occur after the Temporal
> > Goal is achieved.  This adds an unnecessary burden to the users and
> > contradicts the core goal of making it "easy" for them to tune the
> > scheme.
> 
> Still I feel the problem is unclear.  Why the users need to manually ignore or
> filter out the increments under what situation?  Knowing specific and detailed
> case would be helpful.  Are you or some people you know doing that and feeling
> it is too much?  If so, what is the real use case?  For what purpose and how
> DAMON is being used?  Why and how the ignorance of qt_exceeds is being done and
> how painful it is?

First, I'd like to clarify that this isn't a problem encountered by a
real user, it's just a scenario I came up with.

1. Users sample qt_exceeds periodically (e.g., every 10 minutes).

2. Within this 10 minute sampling interval, the counter aggregates both
   the real quota exhaustions and the increments caused by esz==0.

3. When users notice a high qt_exceeds value, they eventually realize
   (perhaps by reading the code or documentation) that it includes the
   counts from the esz==0 state.

4. To get the actual quota exhaustion statistics, the user is now forced
   to perform additional testing and implement external filtering to
   separate the esz==0 increments from the real exceeds.

Even if we explicitly state in the documentation that qt_exceeds
includes the esz==0 counts, it still burdens the user.  The user still
has to figure out how to filter out the esz==0 increments externally to
get the signal they actually care about.

Honestly, I struggle to imagine any valid use case where a user would
actually rely on the qt_exceeds increments caused by esz==0 to make
decisions.

If the only purpose of qt_exceeds is to let users "easily notice" if the
quota is too small, forcing them to manually filter out the noise
defeats that purpose.

Best regards,
Rui Yan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-04  8:07               ` Liew Rui Yan
@ 2026-09-04 14:05                 ` SJ Park
  2026-09-04 15:35                   ` Liew Rui Yan
  0 siblings, 1 reply; 13+ messages in thread
From: SJ Park @ 2026-09-04 14:05 UTC (permalink / raw)
  To: Liew Rui Yan; +Cc: SJ Park, akpm, damon, linux-kernel, linux-mm, stable

On Fri,  4 Sep 2026 16:07:41 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> On Thu, 03 Sep 2026 07:05:20 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > On Thu,  3 Sep 2026 20:41:48 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > 
> > > On Wed, 02 Sep 2026 17:33:50 -0700 SJ Park <sj@kernel.org> wrote:
> > > 
> > > > On Thu,  3 Sep 2026 06:31:38 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > [...]
> > > First, I would like to clarify my intention to avoid any
> > > misunderstanding.  My actual goal is to fix the semantic of the
> > > qt_exceeds statistic, rather than necessarily changing the underlying
> > > logic of damos_quota_is_full().
> > > 
> > > Currently, there is an issue with how qt_exceeds is incremented.  When
> > > the quota is set very small, qt_exceeds increases frequently.  This
> > > produces a statistical trend that looks almost identical to the
> > > continuous increments caused by the Temporal Goal being achieved.
> > > 
> > > The original intent of introducing qt_exceeds is to let users easily
> > > notice if the quota is too small.
> > > 
> > > Commit Messages [1]:
> > > 
> > >     mm/damon/schemes: account how many times quota limit has exceeded
> > > 
> > >     If the time/space quotas of a given DAMON-based operation scheme is too
> > >     small, the scheme could show unexpectedly slow progress.  However, there
> > >     is no good way to notice the case in runtime.  This commit extends the
> > >     DAMOS stat to provide how many times the quota limits exceeded so that
> > >     the users can easily notice the case and tune the scheme.
> > > 
> > > However, under the current behavior, users are forced to manually ignore
> > > or filter out the qt_exceeds increments that occur after the Temporal
> > > Goal is achieved.  This adds an unnecessary burden to the users and
> > > contradicts the core goal of making it "easy" for them to tune the
> > > scheme.
> > 
> > Still I feel the problem is unclear.  Why the users need to manually ignore or
> > filter out the increments under what situation?  Knowing specific and detailed
> > case would be helpful.  Are you or some people you know doing that and feeling
> > it is too much?  If so, what is the real use case?  For what purpose and how
> > DAMON is being used?  Why and how the ignorance of qt_exceeds is being done and
> > how painful it is?
> 
> First, I'd like to clarify that this isn't a problem encountered by a
> real user, it's just a scenario I came up with.

Thank you for clarifying this.

> 
> 1. Users sample qt_exceeds periodically (e.g., every 10 minutes).

What's the purpose of this sampling?

> 
> 2. Within this 10 minute sampling interval, the counter aggregates both
>    the real quota exhaustions and the increments caused by esz==0.
> 
> 3. When users notice a high qt_exceeds value, they eventually realize
>    (perhaps by reading the code or documentation) that it includes the
>    counts from the esz==0 state.
> 
> 4. To get the actual quota exhaustion statistics, the user is now forced
>    to perform additional testing and implement external filtering to
>    separate the esz==0 increments from the real exceeds.
> 
> Even if we explicitly state in the documentation that qt_exceeds
> includes the esz==0 counts, it still burdens the user.  The user still
> has to figure out how to filter out the esz==0 increments externally to
> get the signal they actually care about.

Users set the temporal goal.  They can know when the goal is achieved since
most of the goal metrics are already exposed to user space.  Users can also
show the current effective quotas.  I agree that can be cumbersome, but how
problematic it is?  Also, as I asked above, why they want to do this after all?

> 
> Honestly, I struggle to imagine any valid use case where a user would
> actually rely on the qt_exceeds increments caused by esz==0 to make
> decisions.
> 
> If the only purpose of qt_exceeds is to let users "easily notice" if the
> quota is too small,

I agree it could be a signal to show if the quota is too small.  But the real
purpose of qt_exceeds is, in my opinion, letting users understand how DAMOS is
internally working now.  After all, how much quota means if it is too small or
not?  That all depends on the real use case and complicated things including
their SLO etc.

If documentation is saying the purpose of qt_exceeds is to show if the quota is
too small, that is what need to be updated.

> forcing them to manually filter out the noise
> defeats that purpose.

So, I don't agree.


Thanks,
SJ

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero
  2026-09-04 14:05                 ` SJ Park
@ 2026-09-04 15:35                   ` Liew Rui Yan
  0 siblings, 0 replies; 13+ messages in thread
From: Liew Rui Yan @ 2026-09-04 15:35 UTC (permalink / raw)
  To: sj; +Cc: aethernet65535, akpm, damon, linux-kernel, linux-mm, stable

On Fri, 04 Sep 2026 07:05:35 -0700 SJ Park <sj@kernel.org> wrote:

> On Fri,  4 Sep 2026 16:07:41 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> 
> > First, I'd like to clarify that this isn't a problem encountered by a
> > real user, it's just a scenario I came up with.
> 
> Thank you for clarifying this.
> 
> > 
> > 1. Users sample qt_exceeds periodically (e.g., every 10 minutes).
> 
> What's the purpose of this sampling?
> 
> > 
> > 2. Within this 10 minute sampling interval, the counter aggregates both
> >    the real quota exhaustions and the increments caused by esz==0.
> > 
> > 3. When users notice a high qt_exceeds value, they eventually realize
> >    (perhaps by reading the code or documentation) that it includes the
> >    counts from the esz==0 state.
> > 
> > 4. To get the actual quota exhaustion statistics, the user is now forced
> >    to perform additional testing and implement external filtering to
> >    separate the esz==0 increments from the real exceeds.
> > 
> > Even if we explicitly state in the documentation that qt_exceeds
> > includes the esz==0 counts, it still burdens the user.  The user still
> > has to figure out how to filter out the esz==0 increments externally to
> > get the signal they actually care about.
> 
> Users set the temporal goal.  They can know when the goal is achieved since
> most of the goal metrics are already exposed to user space.  Users can also
> show the current effective quotas.  I agree that can be cumbersome, but how
> problematic it is?  Also, as I asked above, why they want to do this after all?
> 
> > 
> > Honestly, I struggle to imagine any valid use case where a user would
> > actually rely on the qt_exceeds increments caused by esz==0 to make
> > decisions.
> > 
> > If the only purpose of qt_exceeds is to let users "easily notice" if the
> > quota is too small,
> 
> I agree it could be a signal to show if the quota is too small.  But the real
> purpose of qt_exceeds is, in my opinion, letting users understand how DAMOS is
> internally working now.  After all, how much quota means if it is too small or
> not?  That all depends on the real use case and complicated things including
> their SLO etc.

Thank you for your clarify.

> 
> If documentation is saying the purpose of qt_exceeds is to show if the quota is
> too small, that is what need to be updated.

I completely agree your perspective.

This is the current documentation of qt_exceeds:

    - ``qt_exceeds``: Total number of times the quota of the scheme has exceeded.

Although it state the purpose of this statistic, I think adding a
note to clarify that this stat also increase when the quota is zero (but
not unlimited) would be helpful for users.  For example:

    Usually, a quota of zero means the DAMOS scheme has an unlimited
    quota, so qt_exceeds will not increase.  However, if user sets a
    temporal quota goal, the quota is set to zero once the goal is
    [over]-achieved.  In this situation, qt_exceeds will still increase.

I can prepare a formal documentation patch based on this if you agree.

[...]

That said, it's not important for me to add explanations to the
document, but may I know why commit [2] changed the behavior which
introduced by commit [1]?

Commit [1] Behavior:

    if (quota->esz && quota->changed_sz >= quota->esz)
        s->stat.qt_exceeds++;

Commit [2] Behavior:

    if (damos_quota_is_full(quota, c->min_region_sz))
        s->stat.qt_exceeds++;

Before commit [2], qt_exceeds will only increase when quota->esz is not
zero, but after commit [2], qt_exceeds also increase even when
quota->esz is zero.  I'd love to understand the rationale behind this
change to better grasp the design evolution.

[1] 6268eac34ca30 ("mm/damon/schemes: account how many times quota limit has exceeded")
    (Fri Jan 14 14:10:20 2022 -0800)
[2] c7ec7d5f6b3d1 ("mm/damon/core: handle <min_region_sz remaining quota as empty")
    (Mon Apr 27 18:33:50 2026 -0700)

Best regards,
Rui Yan

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2026-09-04 15:36 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-02  8:44 [PATCH v2.1] mm/damon/core: fix false positive in damos_quota_is_full() when esz is zero Liew Rui Yan
2026-09-02  8:53 ` sashiko-bot
2026-09-02  9:42   ` Liew Rui Yan
2026-09-02 14:10 ` SJ Park
2026-09-02 14:22   ` Liew Rui Yan
2026-09-02 14:48     ` SJ Park
2026-09-02 22:31       ` Liew Rui Yan
2026-09-03  0:33         ` SJ Park
2026-09-03 12:41           ` Liew Rui Yan
2026-09-03 14:05             ` SJ Park
2026-09-04  8:07               ` Liew Rui Yan
2026-09-04 14:05                 ` SJ Park
2026-09-04 15:35                   ` Liew Rui Yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).