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

On Tue, 28 Jul 2026 04:32:59 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] DAMON_FILTER_TYPE_PGIDLE_UNSET blindly matches almost all pages when probe weights are used, rendering the filter ineffective.
> - [High] The newly added DAMON_FILTER_TYPE_PGIDLE_UNSET is not exposed in the sysfs interface, making it impossible for users to configure.
> --
> 
> --- Patch [2]: [RFC PATCH v2.2 02/17] mm/damon/paddr: support PGIDLE_UNSET probe filter type ---
> commit 98c0d4f6b2dcc4c706f1af62a258dd827b89bc7e
> 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:
> 
> [Severity: High]
> Is this new filter type missing from the sysfs interface mapping?
> 
> Looking at damon_sysfs_filter_type_names[] in mm/damon/sysfs.c, there is
> no entry for DAMON_FILTER_TYPE_PGIDLE_UNSET:
> 
> mm/damon/sysfs.c:damon_sysfs_filter_type_names[] = {
>         {
>                 .type = DAMON_FILTER_TYPE_ANON,
>                 .name = "anon",
>         },
>         {
>                 .type = DAMON_FILTER_TYPE_MEMCG,
>                 .name = "memcg",
>         },
> };
> 
> Without an entry here, will users attempting to configure this new filter via
> sysfs receive -EINVAL, making the filter unconfigurable?

A later patch of this series does that.

> 
> > +		if (!folio)
> > +			matched = false;
> > +		else
> > +			matched = damon_folio_young(folio);
> 
> [Severity: High]
> Will this blindly match almost all folios when probe weights are used?
> 
> In kdamond_fn(), if damon_has_probe_weights(ctx) is true, it skips
> ctx->ops.prepare_access_checks():
> 
> 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);
>         ...
> }
> 
> For physical addresses, this skips damon_pa_mkold() which normally sets the
> PG_idle flag on folios.
> 
> When the filter uses damon_folio_young() here to evaluate access,
> damon_folio_young_one() checks !folio_test_idle(folio).
> 
> Since the idle flag was never set, will this evaluate to true for all
> folios, causing the PGIDLE_UNSET filter to unconditionally match?

Yes, and it is intentional.

Later patches of this series add a way to automatically do such idle flag
setup.

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


Thanks,
SJ