| Message ID | 3506997a8146a287ec13ee7a53cf916707537bf8.1679431180.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | further cleanup of vma_merge() | expand |
* Lorenzo Stoakes <lstoakes@gmail.com> [230321 16:51]: > We are only interested in next if end == next->vm_start (in which case we > check to see if we can set merge_next), so perform this check alongside > checking whether curr should be set. > > This groups all of the simple range checks together and establishes the > invariant that, if prev, curr or next are non-NULL then their positions are > as expected. > > This has no functional impact. > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > --- > mm/mmap.c | 25 ++++++++++++------------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index c9834364ac98..6361baf75601 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -930,15 +930,15 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > if (vm_flags & VM_SPECIAL) > return NULL; > > - curr = find_vma(mm, prev ? prev->vm_end : 0); > - if (curr && curr->vm_end == end) /* cases 6, 7, 8 */ > - next = find_vma(mm, curr->vm_end); > - else > - next = curr; > + /* Does the input range span an existing VMA? (cases 5 - 8) */ > + curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end); > > - /* In cases 1 - 4 there's no CCCC vma */ > - if (curr && end <= curr->vm_start) > - curr = NULL; > + if (curr && end == curr->vm_end) > + /* Is there is a VMA immediately adjacent (cases 6 - 8)? */ > + next = vma_lookup(mm, curr->vm_end); Since end == curr->vm_end, this lookup is the same as below so these two statements can be combined. I also think you may need to initialize next to null since it may not be set for the 'cannot merge' case. Something like: if ((!curr) || /* case 1-4 */ (end == curr->vm_end)) /* Case 6-8, adjacent vma */ next = vma_lookup(mm, end); else next = NULL > + else if (!curr) > + /* Is there a VMA next to a hole (case 1 - 3) or prev (4)? */ > + next = vma_lookup(mm, end); Nit, can we have braces for comments that make the if/else look like it's unguarded? > > /* verify some invariant that must be enforced by the caller */ > VM_WARN_ON(prev && addr <= prev->vm_start); > @@ -959,11 +959,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > } > } > /* 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)) { > + if (next && 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; > } > > -- > 2.39.2 >
On Tue, Mar 21, 2023 at 09:42:32PM -0400, Liam R. Howlett wrote: > * Lorenzo Stoakes <lstoakes@gmail.com> [230321 16:51]: > > We are only interested in next if end == next->vm_start (in which case we > > check to see if we can set merge_next), so perform this check alongside > > checking whether curr should be set. > > > > This groups all of the simple range checks together and establishes the > > invariant that, if prev, curr or next are non-NULL then their positions are > > as expected. > > > > This has no functional impact. > > > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > > --- > > mm/mmap.c | 25 ++++++++++++------------- > > 1 file changed, 12 insertions(+), 13 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index c9834364ac98..6361baf75601 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -930,15 +930,15 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > if (vm_flags & VM_SPECIAL) > > return NULL; > > > > - curr = find_vma(mm, prev ? prev->vm_end : 0); > > - if (curr && curr->vm_end == end) /* cases 6, 7, 8 */ > > - next = find_vma(mm, curr->vm_end); > > - else > > - next = curr; > > + /* Does the input range span an existing VMA? (cases 5 - 8) */ > > + curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end); > > > > - /* In cases 1 - 4 there's no CCCC vma */ > > - if (curr && end <= curr->vm_start) > > - curr = NULL; > > + if (curr && end == curr->vm_end) > > + /* Is there is a VMA immediately adjacent (cases 6 - 8)? */ > > + next = vma_lookup(mm, curr->vm_end); > > Since end == curr->vm_end, this lookup is the same as below so these two > statements can be combined. I can't believe I didn't see this :) wood for the trees + a great example as to the benefit of code review. > > I also think you may need to initialize next to null since it may not be > set for the 'cannot merge' case. You're right, will fix. > > Something like: > if ((!curr) || /* case 1-4 */ > (end == curr->vm_end)) /* Case 6-8, adjacent vma */ > next = vma_lookup(mm, end); > else > next = NULL > This is really turning out to be quite nicely succinct now I think! Will do. > > > + else if (!curr) > > + /* Is there a VMA next to a hole (case 1 - 3) or prev (4)? */ > > + next = vma_lookup(mm, end); > > Nit, can we have braces for comments that make the if/else look like > it's unguarded? > Ack will fix in respin. > > > > /* verify some invariant that must be enforced by the caller */ > > VM_WARN_ON(prev && addr <= prev->vm_start); > > @@ -959,11 +959,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > } > > } > > /* 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)) { > > + if (next && 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; > > } > > > > -- > > 2.39.2 > >
diff --git a/mm/mmap.c b/mm/mmap.c index c9834364ac98..6361baf75601 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -930,15 +930,15 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, if (vm_flags & VM_SPECIAL) return NULL; - curr = find_vma(mm, prev ? prev->vm_end : 0); - if (curr && curr->vm_end == end) /* cases 6, 7, 8 */ - next = find_vma(mm, curr->vm_end); - else - next = curr; + /* Does the input range span an existing VMA? (cases 5 - 8) */ + curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end); - /* In cases 1 - 4 there's no CCCC vma */ - if (curr && end <= curr->vm_start) - curr = NULL; + if (curr && end == curr->vm_end) + /* Is there is a VMA immediately adjacent (cases 6 - 8)? */ + next = vma_lookup(mm, curr->vm_end); + else if (!curr) + /* Is there a VMA next to a hole (case 1 - 3) or prev (4)? */ + next = vma_lookup(mm, end); /* verify some invariant that must be enforced by the caller */ VM_WARN_ON(prev && addr <= prev->vm_start); @@ -959,11 +959,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, } } /* 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)) { + if (next && 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; }
We are only interested in next if end == next->vm_start (in which case we check to see if we can set merge_next), so perform this check alongside checking whether curr should be set. This groups all of the simple range checks together and establishes the invariant that, if prev, curr or next are non-NULL then their positions are as expected. This has no functional impact. Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> --- mm/mmap.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)