All of lore.kernel.org
 help / color / mirror / Atom feed
From: SeongJae Park <sj@kernel.org>
To: sashiko-bot@kernel.org
Cc: SeongJae Park <sj@kernel.org>, damon@lists.linux.dev
Subject: Re: [RFC PATCH] mm/damon/core: trace esz at first setup
Date: Tue, 19 May 2026 18:28:14 -0700	[thread overview]
Message-ID: <20260520012814.94187-1-sj@kernel.org> (raw)
In-Reply-To: <20260520011837.6192D1F000E9@smtp.kernel.org>

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

[...]

      reply	other threads:[~2026-05-20  1:28 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260520012814.94187-1-sj@kernel.org \
    --to=sj@kernel.org \
    --cc=damon@lists.linux.dev \
    --cc=sashiko-bot@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.