Re: [PATCH v8] mm/damon: add node_eligible_mem_bp goal metric

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Casting phys_addr_t to unsigned long causes truncation and potential divide-by-zero on 32-bit systems
- [High] Missing cond_resched() inside the PFN-scanning loop leads to CPU soft lockups
- [High] Synchronous O(N) full-memory scanning defeats DAMON's bounded-overhead design
- [Medium] damos_calc_eligible_bytes() skips core filters, corrupting the metric calculation
--

commit efcd6d55e5292f8b5cd0c6202c9d95000e99dc4c
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

mm/damon: add node_eligible_mem_bp goal metric

This commit introduces a new quota goal metric, DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP,
to track the ratio of scheme-eligible memory on a specific NUMA node. It
implements calculation functions that iterate over eligible regions and examine
the NUMA node of each folio.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 3dbbbfdeff71..a9303a8c4384 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -2287,7 +2306,112 @@ static unsigned long damos_get_node_memcg_used_bp(
[ ... ]
> +static phys_addr_t damos_calc_eligible_bytes(struct damon_ctx *c,
> +		struct damos *s, int nid, phys_addr_t *total)
> +{
> +	struct damon_target *t;
> +	struct damon_region *r;
> +	phys_addr_t total_eligible = 0;
> +	phys_addr_t node_eligible = 0;
> +
> +	damon_for_each_target(t, c) {
> +		damon_for_each_region(r, t) {
> +			phys_addr_t addr, end_addr;
> +
> +			if (!__damos_valid_target(r, s))
> +				continue;

Does this also need to check damos_core_filter_out(c, t, r, s)? By skipping
the core filters, the eligible bytes calculation might erroneously include
memory that the scheme is explicitly configured to ignore.

> +
> +			/* Convert from core address units to physical bytes */
> +			addr = (phys_addr_t)r->ar.start * c->addr_unit;
> +			end_addr = (phys_addr_t)r->ar.end * c->addr_unit;
> +			while (addr < end_addr) {

Since DAMON is designed to provide low, constant-overhead memory monitoring via
region-based tracking, does performing a synchronous PFN-to-folio lookup for
every physical page across all eligible regions break this bounded-overhead
contract?

Because this function is called on every quota reset interval, scanning
potentially gigabytes of memory might introduce severe latency spikes and CPU
overhead.

Additionally, since DAMON regions can cover extremely large areas of memory,
should cond_resched() be called inside this inner while loop rather than
outside it? Iterating through millions of pages without yielding might trigger
RCU stalls and soft lockups.

> +				struct folio *folio;
> +				phys_addr_t folio_start, folio_end;
> +				phys_addr_t overlap_start, overlap_end;
> +				phys_addr_t counted;
> +
> +				folio = damon_get_folio(PHYS_PFN(addr));
[ ... ]
> +
> +				/* Advance past the entire folio */
> +				addr = folio_end;
> +				folio_put(folio);
> +			}
> +			cond_resched();
> +		}
> +	}
> +
> +	*total = total_eligible;
> +	return node_eligible;
> +}
> +
> +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((unsigned long)node_eligible, 10000,
> +			(unsigned long)total_eligible);

Can casting total_eligible from phys_addr_t to unsigned long cause a
divide-by-zero on 32-bit systems?

On 32-bit architectures with PAE, phys_addr_t is 64-bit while unsigned long
is only 32-bit. If the total eligible memory is exactly a multiple of 4GB,
the 64-bit value passes the if (!total_eligible) check, but the truncated
32-bit value passed as the divisor to mult_frac() evaluates to 0, which
would trigger a kernel panic.

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