* 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
>
