Re: [RESEND RFC PATCH v2 5/5] mm/damon/vaddr: implement DAMOS_SPLIT handler

[SJ Park <sj@kernel.org>]

On Thu,  2 Jul 2026 17:46:33 +0800 Lian Wang <lianux.mm@gmail.com> wrote:

> Implement the vaddr operations layer handler for DAMOS_SPLIT.
> For each folio in the target region that is larger than the
> scheme's target_order, split it via split_folio_to_order().
> 
> This supports both anonymous and file-backed (e.g. tmpfs/shmem)
> folios, covering KVM guest memory backed by THP tmpfs.
> 
> Signed-off-by: Lian Wang <lianux.mm@gmail.com>
> Signed-off-by: Lian Wang <lianux.wang@processmission.com>

Let's use single S-o-b: if it works.

> ---
>  mm/damon/vaddr.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 98a87609376b..a177703f7e0a 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -944,6 +944,83 @@ static unsigned long damos_va_collapse(struct damon_target *target,
>  	return applied;
>  }
>  
> +static unsigned long damos_va_split(struct damon_target *target,
> +		struct damon_region *r, struct damos *s,
> +		unsigned long *sz_filter_passed)
> +{
> +	unsigned long addr, end, chunk_sz;
> +	unsigned int target_order = s->target_order;
> +	unsigned long applied = 0;
> +	struct mm_struct *mm;
> +	struct vm_area_struct *vma;
> +	struct folio *folio;
> +	struct folio_walk fw;
> +
> +	mm = damon_get_mm(target);
> +	if (!mm)
> +		return 0;
> +
> +	chunk_sz = PAGE_SIZE << HPAGE_PMD_ORDER;
> +	addr = ALIGN_DOWN(r->ar.start, chunk_sz);
> +	end = ALIGN(r->ar.end, chunk_sz);

Like I commented to collapse part, would it make more sense to ALIGN() addr and
ALIGN_DOWN() end?

> +	if (end < addr)
> +		goto out_mmput;
> +
> +	while (addr < end) {

Above 'if (end < addr)' seems unnecessary, as while() here is covering it?

> +		unsigned long folio_sz;
> +
> +		if (addr + chunk_sz < addr)
> +			break;

What's the purpose here?  Preventing overflow?  How the overflow can happen,
and what is the consequence?

> +
> +		mmap_read_lock(mm);
> +		vma = find_vma(mm, addr);
> +
> +		if (!vma || addr < vma->vm_start ||
> +		    vma->vm_flags & (VM_HUGETLB | VM_MIXEDMAP))
> +			goto unlock;
> +
> +		folio = folio_walk_start(&fw, vma, addr, 0);

Why we need this?  Can't we use the simpler pattern like that on
damos_va_stat_pmd_entry()?

> +		if (!folio)
> +			goto unlock;
> +
> +		folio_sz = PAGE_SIZE << folio_order(folio);

You can use folio_size().

> +
> +		if (folio_order(folio) > target_order) {
> +			if (!folio_trylock(folio)) {
> +				folio_walk_end(&fw, vma);
> +				goto unlock;
> +			}
> +			folio_get(folio);
> +			folio_walk_end(&fw, vma);
> +
> +			if (!split_folio_to_order(folio, target_order))
> +				applied += folio_sz;
> +
> +			folio_unlock(folio);
> +			folio_put(folio);
> +			*sz_filter_passed += folio_sz;

Shoudn't this code calls damos_va_filter_out() before split_folio_to_order()?

> +			addr += folio_sz;
> +		} else {
> +			folio_walk_end(&fw, vma);
> +			*sz_filter_passed += chunk_sz;
> +			addr += chunk_sz;
> +		}
> +		mmap_read_unlock(mm);
> +		cond_resched();
> +		continue;
> +
> +unlock:
> +		*sz_filter_passed += chunk_sz;
> +		addr += chunk_sz;
> +		mmap_read_unlock(mm);
> +		cond_resched();
> +	}
> +
> +out_mmput:
> +	mmput(mm);
> +	return applied;
> +}
> +
>  static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  		struct damon_target *t, struct damon_region *r,
>  		struct damos *scheme, unsigned long *sz_filter_passed)
> @@ -977,6 +1054,9 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  		return damos_va_migrate(t, r, scheme, sz_filter_passed);
>  	case DAMOS_STAT:
>  		return damos_va_stat(t, r, scheme, sz_filter_passed);
> +	case DAMOS_SPLIT:
> +		return damos_va_split(t, r, scheme,
> +					  sz_filter_passed);

Let's keep the order same to that of damos_action.

>  	default:
>  		/*
>  		 * DAMOS actions that are not yet supported by 'vaddr'.


Thanks,
SJ