Re: [PATCH v3 10/12] mm/collapse: work out the orders a VMA allows once per VMA

[David Hildenbrand (Arm)]

On 9/16/26 11:31, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@xxxxxxxxxx>
> 
> The scan asked collapse_possible_orders() for every PTE table, for an
> answer that is a property of the VMA.  Both callers walk a VMA a table at
> a time, so let them work it out once and pass the mask in.  It is only
> good while the lock that produced it is held, so madvise_collapse() takes
> it again after every collapse.
> 
> The mask is then sampled once per VMA rather than once per table.  A thp
> enabled knob written during a walk takes effect one VMA later, and cannot
> widen a collapse: hugepage_vma_revalidate() tests the order again under
> the lock the collapse retakes.
> 
> Assisted-by: LLM
> Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> ---
>  mm/khugepaged.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 9e6b2af6519e..12cb67d8df32 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1549,12 +1549,12 @@ static enum scan_result mthp_collapse(struct mm_struct *mm, unsigned long addres
>  }
>  

[...]

> +		/* One mask for the whole VMA */
> +		orders = collapse_possible_orders(vma, vma->vm_flags,
> +						  cc->policy.tva_type);

cc->policy.tva_type is always sattic here, no?

> +		if (!orders) {
>  			cc->progress++;
>  			continue;
>  		}
> @@ -2922,7 +2922,7 @@ static void collapse_scan_mm_slot(unsigned int progress_max,
>  			/* move to next address */
>  			khugepaged_scan.address += HPAGE_PMD_SIZE;
>  
> -			*result = collapse_scan_pmd(vma, addr, cc);
> +			*result = collapse_scan_pmd(vma, addr, cc, orders);
>  			/* Nothing to do here, and the lock is still ours */
>  			if (*result != SCAN_SUCCEED &&
>  			    *result != SCAN_PTE_MAPPED_HUGEPAGE) {
> @@ -3207,14 +3207,16 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>  {
>  	struct collapse_control *cc;
>  	struct mm_struct *mm = vma->vm_mm;
> -	unsigned long hstart, hend, addr;
> +	unsigned long hstart, hend, addr, orders;
>  	enum scan_result last_fail = SCAN_FAIL;
>  	int thps = 0;
>  
>  	BUG_ON(vma->vm_start > start);
>  	BUG_ON(vma->vm_end < end);
>  
> -	if (!collapse_possible_orders(vma, vma->vm_flags, TVA_FORCED_COLLAPSE))
> +	orders = collapse_possible_orders(vma, vma->vm_flags,
> +					  TVA_FORCED_COLLAPSE);
> +	if (!orders)
>  		return -EINVAL;
>  
>  	hstart = ALIGN(start, HPAGE_PMD_SIZE);
> @@ -3252,9 +3254,11 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>  			}
>  			vma = found;
>  			hend = min(hend, vma->vm_end & HPAGE_PMD_MASK);
> +			orders = collapse_possible_orders(vma, vma->vm_flags,
> +							  cc->policy.tva_type);


That's always TVA_FORCED_COLLAPSE, no?

It's a shame we cannot get rid of cc->policy.tva_type because we need it for
hugepage_vma_revalidate to calculate orders. Which sucks a bit.


-- 
Cheers,

David