Re: [RFC PATCH v2.2 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:
- [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?

> +		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?

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

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