Re: [RFC PATCH 02/17] mm/damon/paddr: support PGIDLE_UNSET probe filter type

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

New issues:
- [High] The PGIDLE_UNSET probe filter lacks a mechanism to clear the page idle state and PTE young bits at the start of the sampling interval when used with weights, breaking access monitoring.

Pre-existing issues:
- [High] PFN advancement after page-to-folio conversion using `folio_size()` causes subsequent folios to be skipped if the iteration does not start at the folio's base address.
--

--- Patch [2]: [RFC PATCH 02/17] mm/damon/paddr: support PGIDLE_UNSET probe filter type ---
commit 97a45c1fa60aa6c710c0c0c9226adf390fa8207f
Author: SJ Park <[email protected]>

mm/damon/paddr: support PGIDLE_UNSET probe filter type

Implement support of DAMON_FILTER_TYPE_PGIDLE_UNSET in the physical
address space DAMON operations set. It reuses damon_folio_young(),
which was being used for access monitoring.

> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 5a6a78054784a..199de1463fa06 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -132,6 +132,12 @@ static bool damon_pa_filter_match(struct damon_filter *filter,
>  			matched = filter->memcg_id == mem_cgroup_id(memcg);
>  		rcu_read_unlock();
>  		break;
> +	case DAMON_FILTER_TYPE_PGIDLE_UNSET:
> +		if (!folio)
> +			matched = false;
> +		else
> +			matched = damon_folio_young(folio);

[Severity: High]
Does this filter evaluate historical access state instead of interval-specific
accesses when used with weights?

If a DAMON probe is configured with weights, it skips the
prepare_access_checks() phase:

mm/damon/core.c:kdamond_fn() {
    ...
    bool access_check_disabled = damon_has_probe_weights(ctx);
    ...
    if (!access_check_disabled && ctx->ops.prepare_access_checks)
        ctx->ops.prepare_access_checks(ctx);
    ...
}

Because damon_pa_mkold() is never called on this address at the beginning
of the interval, it seems damon_folio_young() will read stale access state
and perpetually match once a page is accessed.

> +		break;
>  	default:
>  		break;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does the address advancement
in damon_pa_pageout() skip valid physical memory if the DAMON region
boundary is not aligned to a large folio boundary?

mm/damon/paddr.c:damon_pa_pageout() {
    ...
    folio = damon_get_folio(...);
    ...
    addr += folio_size(folio);
    ...
}

When damon_get_folio() returns a large folio, unconditionally adding the full
folio_size(folio) to an unaligned addr over-advances the address by
addr % folio_size(folio). This could cause the loop to skip the beginning
of the next folio.

The same unaligned address advancement also appears in damon_pa_de_activate(),
damon_pa_migrate(), and damon_pa_stat().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2