Re: [PATCH v4 1/2] [PATCH 1/2] mm: refactor of vma_merge()

[Vlastimil Babka]

On 6/3/22 16:57, Jakub Matěna wrote:
> Refactor vma_merge() to make it shorter and more understandable.
> Main change is the elimination of code duplicity in the case of
> merge next check. This is done by first doing checks and caching
> the results before executing the merge itself. The variable 'area' is
> divided into 'mid' and 'res' as previously it was used for two purposes,
> as the middle VMA between prev and next and also as the result of the
> merge itself. Exit paths are also unified.
> 
> Signed-off-by: Jakub Matěna <matenajakub@xxxxxxxxx>

Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>

> ---
>  mm/mmap.c | 87 +++++++++++++++++++++++--------------------------------
>  1 file changed, 37 insertions(+), 50 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 313b57d55a63..91100fdc400a 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1170,8 +1170,10 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
>  			struct anon_vma_name *anon_name)
>  {
>  	pgoff_t pglen = (end - addr) >> PAGE_SHIFT;
> -	struct vm_area_struct *area, *next;
> -	int err;
> +	struct vm_area_struct *mid, *next, *res;
> +	int err = -1;
> +	bool merge_prev = false;
> +	bool merge_next = false;
>  
>  	/*
>  	 * We later require that vma->vm_flags == vm_flags,
> @@ -1181,75 +1183,60 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
>  		return NULL;
>  
>  	next = find_vma(mm, prev ? prev->vm_end : 0);
> -	area = next;
> -	if (area && area->vm_end == end)		/* cases 6, 7, 8 */
> +	mid = next;
> +	if (next && next->vm_end == end)		/* cases 6, 7, 8 */
>  		next = find_vma(mm, next->vm_end);
>  
>  	/* verify some invariant that must be enforced by the caller */
>  	VM_WARN_ON(prev && addr <= prev->vm_start);
> -	VM_WARN_ON(area && end > area->vm_end);
> +	VM_WARN_ON(mid && end > mid->vm_end);
>  	VM_WARN_ON(addr >= end);
>  
> -	/*
> -	 * Can it merge with the predecessor?
> -	 */
> +	/* Can we merge the predecessor? */
>  	if (prev && prev->vm_end == addr &&
>  			mpol_equal(vma_policy(prev), policy) &&
>  			can_vma_merge_after(prev, vm_flags,
>  					    anon_vma, file, pgoff,
>  					    vm_userfaultfd_ctx, anon_name)) {
> -		/*
> -		 * OK, it can.  Can we now merge in the successor as well?
> -		 */
> -		if (next && end == next->vm_start &&
> -				mpol_equal(policy, vma_policy(next)) &&
> -				can_vma_merge_before(next, vm_flags,
> -						     anon_vma, file,
> -						     pgoff+pglen,
> -						     vm_userfaultfd_ctx, anon_name) &&
> -				is_mergeable_anon_vma(prev->anon_vma,
> -						      next->anon_vma, NULL)) {
> -							/* cases 1, 6 */
> -			err = __vma_adjust(prev, prev->vm_start,
> -					 next->vm_end, prev->vm_pgoff, NULL,
> -					 prev);
> -		} else					/* cases 2, 5, 7 */
> -			err = __vma_adjust(prev, prev->vm_start,
> -					 end, prev->vm_pgoff, NULL, prev);
> -		if (err)
> -			return NULL;
> -		khugepaged_enter_vma(prev, vm_flags);
> -		return prev;
> +		merge_prev = true;
>  	}
> -
> -	/*
> -	 * Can this new request be merged in front of next?
> -	 */
> +	/* Can we merge the successor? */
>  	if (next && end == next->vm_start &&
>  			mpol_equal(policy, vma_policy(next)) &&
>  			can_vma_merge_before(next, vm_flags,
>  					     anon_vma, file, pgoff+pglen,
>  					     vm_userfaultfd_ctx, anon_name)) {
> +		merge_next = true;
> +	}
> +	/* Can we merge both the predecessor and the successor? */
> +	if (merge_prev && merge_next &&
> +			is_mergeable_anon_vma(prev->anon_vma,
> +				next->anon_vma, NULL)) {	 /* cases 1, 6 */
> +		err = __vma_adjust(prev, prev->vm_start,
> +					next->vm_end, prev->vm_pgoff, NULL,
> +					prev);
> +		res = prev;
> +	} else if (merge_prev) {			/* cases 2, 5, 7 */
> +		err = __vma_adjust(prev, prev->vm_start,
> +					end, prev->vm_pgoff, NULL, prev);
> +		res = prev;
> +	} else if (merge_next) {
>  		if (prev && addr < prev->vm_end)	/* case 4 */
>  			err = __vma_adjust(prev, prev->vm_start,
> -					 addr, prev->vm_pgoff, NULL, next);
> -		else {					/* cases 3, 8 */
> -			err = __vma_adjust(area, addr, next->vm_end,
> -					 next->vm_pgoff - pglen, NULL, next);
> -			/*
> -			 * In case 3 area is already equal to next and
> -			 * this is a noop, but in case 8 "area" has
> -			 * been removed and next was expanded over it.
> -			 */
> -			area = next;
> -		}
> -		if (err)
> -			return NULL;
> -		khugepaged_enter_vma(area, vm_flags);
> -		return area;
> +					addr, prev->vm_pgoff, NULL, next);
> +		else					/* cases 3, 8 */
> +			err = __vma_adjust(mid, addr, next->vm_end,
> +					next->vm_pgoff - pglen, NULL, next);
> +		res = next;
>  	}
>  
> -	return NULL;
> +	/*
> +	 * Cannot merge with predecessor or successor or error in __vma_adjust?
> +	 */
> +	if (err)
> +		return NULL;
> +	khugepaged_enter_vma(res, vm_flags);
> +	return res;
>  }
>  
>  /*