Re: [RESEND RFC PATCH v2 1/5] mm/damon: add target_order field for DAMOS_COLLAPSE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Sysfs value for `target_order` can silently diverge from the active DAMON scheme's internal state.
- [Low] The `#else` block fallback for `target_order` when THP is disabled is unreachable.
--

commit 38547105610a7847b39b76172e675c32ee3ca4f5
Author: Lian Wang <lianux.mm@gmail.com>

mm/damon: add target_order field for DAMOS_COLLAPSE
    
Adds a target_order field to express per-order mTHP collapse intent for
DAMOS_COLLAPSE. Wires up the sysfs interface target_order to configure this
value and warns when an unsupported non-PMD order is used.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index dbf2b0515d582..9067cb66632ed 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c

[ ... ]

> @@ -2644,6 +2648,39 @@ static ssize_t target_nid_store(struct kobject *kobj,
>  	return err ? err : count;
>  }
>  
> +static ssize_t target_order_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	struct damon_sysfs_scheme *scheme = container_of(kobj,
> +			struct damon_sysfs_scheme, kobj);
> +
> +	return sysfs_emit(buf, "%u\n", scheme->target_order);
> +}
> +
> +static ssize_t target_order_store(struct kobject *kobj,
> +		struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	struct damon_sysfs_scheme *scheme = container_of(kobj,
> +			struct damon_sysfs_scheme, kobj);
> +	unsigned int val;
> +	int err;
> +
> +	err = kstrtouint(buf, 0, &val);
> +	if (err)
> +		return err;
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	if (val != 0 && (val < 2 || val > HPAGE_PMD_ORDER))
> +		return -EINVAL;
> +#else
> +	if (val != 0)
> +		return -EINVAL;
> +#endif
> +
> +	scheme->target_order = val;
> +	return count;
> +}

[ ... ]

> @@ -3010,6 +3052,25 @@ static struct damos *damon_sysfs_mk_scheme(
>  	if (!scheme)
>  		return NULL;
>  
> +	target_order = sysfs_scheme->target_order;
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	if (sysfs_scheme->action == DAMOS_COLLAPSE &&
> +	    target_order != 0 &&
> +	    target_order != HPAGE_PMD_ORDER) {
> +		pr_warn("DAMON collapse: target_order %u not supported, only PMD order (%u) is available. Use 0 or %u.\n",
> +			target_order,
> +			HPAGE_PMD_ORDER, HPAGE_PMD_ORDER);
> +		target_order = 0;

[Severity: Medium]
Will this cause the sysfs API to report an incorrect state to userspace?

When an unsupported order is encountered, this overrides target_order to 0 for
the internal scheme, but it fails to update sysfs_scheme->target_order or
return an error. Userspace reading from target_order_show() will see the
divergent uncorrected value and believe a specific target order is active
when it has silently been overridden to the default.

> +	}
> +#else
> +	if (sysfs_scheme->action == DAMOS_COLLAPSE && target_order != 0) {
> +		pr_warn("DAMON collapse: target_order not supported without THP. Use 0.\n");
> +		target_order = 0;
> +	}

[Severity: Low]
Is this block reachable?

In target_order_store(), if CONFIG_TRANSPARENT_HUGEPAGE is not set, any
non-zero value unconditionally returns -EINVAL. It seems target_order will
always be 0 when evaluating this block.

> +#endif
> +	scheme->target_order = target_order;
> +
>  	err = damos_sysfs_add_quota_score(sysfs_quotas->goals, &scheme->quota);
>  	if (err) {
>  		damon_destroy_scheme(scheme);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702095227.75866-1-lianux.mm@gmail.com?part=1