All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] mm/damon/core: trace esz at first setup
@ 2026-05-20  0:59 SeongJae Park
  2026-05-20  1:18 ` sashiko-bot
  0 siblings, 1 reply; 3+ messages in thread
From: SeongJae Park @ 2026-05-20  0:59 UTC (permalink / raw)
  Cc: SeongJae Park, # 6 . 17 . x, Andrew Morton, damon, linux-kernel,
	linux-mm

DAMON traces effective size quota from the second update, only if a
change has been made by the update.  Tracing only changed updates was an
intentional decision to avoid unnecessary same value tracing.  Always
skipping the first value is just an unintended mistake.

The mistake makes the tracepoint based investigation incomplete, because
the first effective size quota is never traced.  It is not a big issue
when the 'consist' quota tuner is used, because it keeps changing the
quota in the usual setup.

However, when the 'temporal' tuner is used, the quota value is not
changed before the goal achievement status is completely changed.  For
example, if the DAMOS scheme is started with an under-achieved goal, the
quota is set to the maximum value, and kept the same value until the
goal is achieved.  Because DAMON skips the first value, the user cannot
know what effective quota the current scheme is using.  Only after the
goal is achieved, the effective quota is changed to zero, and traced.

Unconditionally trace the initial quota value to fix this problem.

Note that the 'temporal' quota tuner was introduced by commit
af738a6a00c1 ("mm/damon/core: introduce
DAMOS_QUOTA_GOAL_TUNER_TEMPORAL"), which was added to 7.1-rc1.  But even
with the 'consist' quota tuner, the tracing is unintentionally
incomplete. Hence this commit marks the introduction of the trace event
as the broken commit.

Fixes: a86d695193bf ("mm/damon: add trace event for effective size quota")
Cc: <stable@vger.kernel.org> # 6.17.x
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 68b3b4bbc8fc9..0267faf216b95 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2886,6 +2886,8 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
 	if (!quota->total_charged_sz && !quota->charged_from) {
 		quota->charged_from = jiffies;
 		damos_set_effective_quota(c, s);
+		if (trace_damos_esz_enabled())
+			damos_trace_esz(c, s, quota);
 	}
 
 	/* New charge window starts */

base-commit: abe8c076ff4a1283d77c1c5cb7b975723314aec9
-- 
2.47.3

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

* Re: [RFC PATCH] mm/damon/core: trace esz at first setup
  2026-05-20  0:59 [RFC PATCH] mm/damon/core: trace esz at first setup SeongJae Park
@ 2026-05-20  1:18 ` sashiko-bot
  2026-05-20  1:28   ` SeongJae Park
  0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-05-20  1:18 UTC (permalink / raw)
  To: SeongJae Park; +Cc: damon

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The monotonic accumulators `total_charged_ns` and `total_charged_sz` will rapidly overflow on 32-bit architectures, breaking the throughput estimation logic.
--

commit 4cb83c3ed801305fa15e94548668201c275a1037
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: trace esz at first setup

This commit fixes an issue where the initial effective size quota is not
traced, which leaves tracepoint investigations incomplete when using the
temporal tuner. It updates the quota adjustment logic to unconditionally
trace the initial quota value.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 4e223857a0f99..0db6530825d1d 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2883,6 +2883,8 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
>  	if (!quota->total_charged_sz && !quota->charged_from) {

[Severity: High]
This isn't a bug introduced by this patch, but will total_charged_ns and
total_charged_sz overflow on 32-bit architectures?

Since unsigned long is 32 bits on those systems, total_charged_ns will
overflow after roughly 4.29 seconds of total active execution time.

Additionally, the throughput calculation in damos_set_effective_quota():

damos_set_effective_quota() {
    ...
    if (quota->total_charged_ns)
        throughput = mult_frac(quota->total_charged_sz,
                1000000, quota->total_charged_ns);
    ...
}

uses mult_frac() which evaluates (x % d) * n internally using the type of x.
Since the remainder could be up to total_charged_ns - 1, multiplying it by
1,000,000 will quickly overflow the 32-bit boundary. This produces incorrect
throughput estimates and impacts the effective size quota calculations on
32-bit systems.

>  		quota->charged_from = jiffies;
>  		damos_set_effective_quota(c, s);
> +		if (trace_damos_esz_enabled())
> +			damos_trace_esz(c, s, quota);
>  	}
>  
>  	/* New charge window starts */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520005940.92003-1-sj@kernel.org?part=1

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

* Re: [RFC PATCH] mm/damon/core: trace esz at first setup
  2026-05-20  1:18 ` sashiko-bot
@ 2026-05-20  1:28   ` SeongJae Park
  0 siblings, 0 replies; 3+ messages in thread
From: SeongJae Park @ 2026-05-20  1:28 UTC (permalink / raw)
  To: sashiko-bot; +Cc: SeongJae Park, damon

On Wed, 20 May 2026 01:18:37 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The monotonic accumulators `total_charged_ns` and `total_charged_sz` will rapidly overflow on 32-bit architectures, breaking the throughput estimation logic.

Not introduced by this patch, so no blocker.

> --
> 
> commit 4cb83c3ed801305fa15e94548668201c275a1037
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: trace esz at first setup
> 
> This commit fixes an issue where the initial effective size quota is not
> traced, which leaves tracepoint investigations incomplete when using the
> temporal tuner. It updates the quota adjustment logic to unconditionally
> trace the initial quota value.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 4e223857a0f99..0db6530825d1d 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2883,6 +2883,8 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
> >  	if (!quota->total_charged_sz && !quota->charged_from) {
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but will total_charged_ns and
> total_charged_sz overflow on 32-bit architectures?
> 
> Since unsigned long is 32 bits on those systems, total_charged_ns will
> overflow after roughly 4.29 seconds of total active execution time.
> 
> Additionally, the throughput calculation in damos_set_effective_quota():
> 
> damos_set_effective_quota() {
>     ...
>     if (quota->total_charged_ns)
>         throughput = mult_frac(quota->total_charged_sz,
>                 1000000, quota->total_charged_ns);
>     ...
> }
> 
> uses mult_frac() which evaluates (x % d) * n internally using the type of x.
> Since the remainder could be up to total_charged_ns - 1, multiplying it by
> 1,000,000 will quickly overflow the 32-bit boundary. This produces incorrect
> throughput estimates and impacts the effective size quota calculations on
> 32-bit systems.

On 32-bit environment, addr_unit can be used to workaround this issue.  On
systems that runs DAMON after a reasonable amount of tests, the issue should
not happen.  I'm thinking about a better solution, though.

Anyway, not a blocker of this patch.


Thanks,
SJ

[...]

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

end of thread, other threads:[~2026-05-20  1:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-20  0:59 [RFC PATCH] mm/damon/core: trace esz at first setup SeongJae Park
2026-05-20  1:18 ` sashiko-bot
2026-05-20  1:28   ` SeongJae Park

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.