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

On Sat, 25 Jul 2026 21:17:05 +0000 [email protected] wrote:

> 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.

That will be implemented in a later patch of this series.

> 
> > +		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().

Known issue, I'm working on it.

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


Thanks,
SJ