Re: [Devel] [PATCH RFC PATCH hci-8.1 1/4] mm: support moving anonymous page tables between address spaces

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: cross-mm move forces rmap locks, serializing padata workers on one anon_vma
- [Low] mm: padata workers in move_page_range_exec() allocate page tables outside exec task memcg
--

commit 8261a13046a4e58b21e6c687a520d3921507b391
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

mm: support moving anonymous page tables between address spaces

This teaches move_page_tables() and move_huge_pmd() to use the destination
mm for page-table operations and to transfer RSS, swap-list and huge-page
table accounting. Cross-mm moves are restricted to same-address anonymous
DOEXEC VMAs. It also adds a parallel exec move wrapper,
move_page_range_exec(), built on padata.

Reference in the commit message:
https://virtuozzo.atlassian.net/browse/VSTOR-103860

> diff --git a/mm/mremap.c b/mm/mremap.c
> --- a/mm/mremap.c
> +++ b/mm/mremap.c

[ ... ]

> @@ -610,10 +646,19 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
>  	struct mmu_notifier_range range;
>  	pmd_t *old_pmd, *new_pmd;
>  	pud_t *old_pud, *new_pud;
> +	bool cross_mm = vma->vm_mm != new_vma->vm_mm;
>  
>  	if (!len)
>  		return 0;
>  
> +	/* Cross-mm moves are only used for same-address anonymous DOEXEC VMAs. */
> +	if (cross_mm) {
> +		if (WARN_ON_ONCE(!vma_is_anonymous(vma) ||
> +				 !(vma->vm_flags & VM_EXEC_KEEP) ||
> +				 old_addr != new_addr || !need_rmap_locks))
> +			return 0;
> +	}

[Severity: Medium]
Does requiring need_rmap_locks for every cross-mm move serialize the padata
workers added below?

move_page_range_chunk() passes need_rmap_locks=true for every chunk:

    return move_page_tables(args->old_vma, addr, args->new_vma, addr,
                            len, true, false) == len ? 0 : -ENOMEM;

For each PMD-sized extent, move_ptes() and move_pgt_entry() then call
take_rmap_locks(vma), which does:

    if (vma->anon_vma)
            anon_vma_lock_write(vma->anon_vma);

All workers operate on the same old VMA, so they all contend on the same
anon_vma write lock, and the PTE-moving loop runs inside that critical
section. Only page-table allocation and cond_resched() can overlap.

Since passing false is rejected by the WARN_ON_ONCE() above, the parallel
path cannot avoid the lock. Does that largely cancel the speedup that
move_page_range_exec() is meant to provide with up to 16 threads?

The commit message does not say why rmap locks are required for cross-mm
moves, or that the workers end up serialized on this lock. Could that be
documented, or could the requirement be relaxed for this case?

[ ... ]

> +static int move_page_range_chunk(unsigned long addr, unsigned long end, void *arg)
> +{
> +	struct move_page_range_args *args = arg;
> +	unsigned long len = end - addr;
> +
> +	return move_page_tables(args->old_vma, addr, args->new_vma, addr,
> +				len, true, false) == len ? 0 : -ENOMEM;
> +}

[ ... ]

> +int move_page_range_exec(struct vm_area_struct *new_vma,
> +			 struct vm_area_struct *old_vma)
> +{

[ ... ]

> +#ifdef CONFIG_PADATA
> +	return padata_do_multithreaded(&job);
> +#else
> +	return move_page_range_chunk(old_vma->vm_start, old_vma->vm_end, &args);
> +#endif
> +}

[Severity: Low]
Are the destination page tables allocated by the padata helpers charged to
the right memcg?

When the job spans more than one chunk, padata_do_multithreaded() queues the
helpers on system_unbound_wq. Each helper then runs:

    padata_mt_helper()
      move_page_range_chunk()
        move_page_tables()
          alloc_new_pud(new_vma->vm_mm, ...)
          alloc_new_pmd(new_vma->vm_mm, ...)
          pte_alloc(new_vma->vm_mm, new_pmd)

These use GFP_PGTABLE_USER, which includes __GFP_ACCOUNT. The charge is
resolved from active_memcg() or current, and here that is the kworker rather
than the exec'ing task.

Neither padata nor this patch calls set_active_memcg() or otherwise attaches
the workers to the submitter's cgroup. Can part of the exec'ing task's
page-table memory therefore escape its memcg limit?

copy_page_range_mt() in mm/memory.c appears to have the same behavior, but
this adds a new padata consumer that relies on it. Should the workers run
under the submitter's memcg, for example by wrapping the chunk function with
set_active_memcg()?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-1901cd6c3c74@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel