Re: [RFC PATCH 1/4] [PATCH 1/4] mm: refactor of vma_merge()

[Liam Howlett]

* Jakub Matěna <matenajakub@xxxxxxxxx> [220218 07:21]:
> Refactor vma_merge() to make it shorter, more understandable and
> suitable for tracing of successful merges made possible by following
> patches in the series.
> 
> Signed-off-by: Jakub Matěna <matenajakub@xxxxxxxxx>
> ---
>  mm/mmap.c | 81 +++++++++++++++++++++++++++----------------------------
>  1 file changed, 39 insertions(+), 42 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 1e8fdb0b51ed..b55e11f20571 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1172,6 +1172,9 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
>  	pgoff_t pglen = (end - addr) >> PAGE_SHIFT;
>  	struct vm_area_struct *area, *next;
>  	int err;
> +	int merge_prev = 0;
> +	int merge_both = 0;
> +	int merge_next = 0;

You set these as true, can you please use booleans?

>  
>  	/*
>  	 * We later require that vma->vm_flags == vm_flags,
> @@ -1191,65 +1194,59 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
>  	VM_WARN_ON(addr >= end);
>  
>  	/*
> -	 * Can it merge with the predecessor?
> +	 * Can we merge 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_merge(prev, vm_flags);
> -		return prev;
> +		merge_prev = true;

You could set area = prev here and simplify the if statements below.

>  	}
> -
>  	/*
> -	 * Can this new request be merged in front of next?
> +	 * Can we merge 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)) {
> +					anon_vma, file, pgoff+pglen,
> +					vm_userfaultfd_ctx, anon_name)) {
> +		merge_next = true;
> +	}
> +	/*
> +	 * Can we merge both predecessor and successor?
> +	 */
> +	if (merge_prev && merge_next)
> +		merge_both = is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL);
> +
> +	if (merge_both) {	 /* cases 1, 6 */
> +		err = __vma_adjust(prev, prev->vm_start,
> +					next->vm_end, prev->vm_pgoff, NULL,
> +					prev);
> +		area = prev;

I don't think you need all three booleans since merge_both is only used
here.

> +	} else if (merge_prev) {			/* cases 2, 5, 7 */
> +		err = __vma_adjust(prev, prev->vm_start,
> +					end, prev->vm_pgoff, NULL, prev);
> +		area = 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 */
> +					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.
> -			 */

I think the above comment is still true?

> -			area = next;
> -		}
> -		if (err)
> -			return NULL;
> -		khugepaged_enter_vma_merge(area, vm_flags);
> -		return area;
> +					next->vm_pgoff - pglen, NULL, next);
> +		area = next;
> +	} else {
> +		err = -1;
>  	}

If you initialize err to something, you can drop this else.

>  
> -	return NULL;
> +	/*
> +	 * Cannot merge with predecessor or successor or error in __vma_adjust?
> +	 */
> +	if (err)
> +		return NULL;
> +	khugepaged_enter_vma_merge(area, vm_flags);
> +	return area;
>  }
>  
>  /*
> -- 
> 2.34.1
>