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>,
	"Ravi Jonnalagadda" <ravis.opensrc@gmail.com>,
	damon@lists.linux.dev
Subject: Re: [PATCH v9] mm/damon: add node_eligible_mem_bp goal metric
Date: Sun, 26 Apr 2026 16:08:44 -0700	[thread overview]
Message-ID: <20260426230845.106371-1-sj@kernel.org> (raw)
In-Reply-To: <20260426220409.95B26C2BCAF@smtp.kernel.org>

On Sun, 26 Apr 2026 22:04:08 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 4 potential issue(s):
> - [High] Missing `cond_resched()` inside the inner address-scanning loop causes CPU soft lockups.

Unlikely.

> - [High] Direct use of `mult_frac()` with 64-bit `phys_addr_t` causes a linker error on 32-bit architectures with PAE.

Good finding.  Read below for detailed answer.

> - [High] Integer overflow in `mult_frac()` on 32-bit systems without PAE leads to incorrect metric calculations.

No real issue.

> - [High] Unbounded O(N) page-by-page memory scan in quota evaluation breaks DAMON's low-overhead design.

Intended behavior.

[...]
> > +static unsigned long damos_get_node_eligible_mem_bp(struct damon_ctx *c,
> > +		struct damos *s, int nid)
> > +{
> > +	phys_addr_t total_eligible = 0;
> > +	phys_addr_t node_eligible;
> > +
> > +	if (c->ops.id != DAMON_OPS_PADDR)
> > +		return 0;
> > +
> > +	if (nid < 0 || nid >= MAX_NUMNODES || !node_online(nid))
> > +		return 0;
> > +
> > +	node_eligible = damos_calc_eligible_bytes(c, s, nid, &total_eligible);
> > +
> > +	if (!total_eligible)
> > +		return 0;
> > +
> > +	return mult_frac(node_eligible, 10000, total_eligible);
> 
> Can this cause a linker error on 32-bit architectures with PAE?
> 
> On 32-bit systems with CONFIG_PHYS_ADDR_T_64BIT enabled, phys_addr_t is a
> 64-bit integer. The mult_frac() macro performs native division using the
> type of its arguments. Since the kernel does not link 64-bit division
> routines (__udivdi3) on 32-bit platforms, this will cause a build failure.

Heh, good finding.  I suggested a fixup on my reply [1] to the patch.

[1] https://lore.kernel.org/20260426230322.106206-1-sj@kernel.org


Thanks,
SJ

[...]

  reply	other threads:[~2026-04-26 23:08 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-26 21:38 [PATCH v9] mm/damon: add node_eligible_mem_bp goal metric Ravi Jonnalagadda
2026-04-26 22:04 ` sashiko-bot
2026-04-26 23:08   ` SeongJae Park [this message]
2026-04-26 23:03 ` SeongJae Park
2026-04-27  0:59   ` Ravi Jonnalagadda

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=20260426230845.106371-1-sj@kernel.org \
    --to=sj@kernel.org \
    --cc=damon@lists.linux.dev \
    --cc=ravis.opensrc@gmail.com \
    --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.