| Message ID | a88bff4cee88232027bf6356a6c3e265600521d4.1679468982.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | further cleanup of vma_merge() | expand |
On 3/22/23 08:13, Lorenzo Stoakes wrote: > Previously, vma was an uninitialised variable which was only definitely > assigned as a result of the logic covering all possible input cases - for > it to have remained uninitialised, prev would have to be NULL, and next > would _have_ to be mergeable. > > The value of res defaults to NULL, so we can neatly eliminate the > assignment to res and vma in the if (prev) block and ensure that both res > and vma are both explicitly assigned, by just setting both to prev. > > In addition we add an explanation as to under what circumstances both might > change, and since we absolutely do rely on addr == curr->vm_start should > curr exist, assert that this is the case. Hm replied to v2 of this, sorry, so let me repeat that here: > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Nit suggestion below. > --- > mm/mmap.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index dbdbb92493b2..2a4f63716231 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -911,7 +911,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > { > pgoff_t pglen = (end - addr) >> PAGE_SHIFT; > pgoff_t vma_pgoff; > - struct vm_area_struct *curr, *next, *res = NULL; > + struct vm_area_struct *curr, *next, *res; > struct vm_area_struct *vma, *adjust, *remove, *remove2; > int err = -1; > bool merge_prev = false; > @@ -939,14 +939,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > else > next = NULL; /* case 5 */ > > - /* verify some invariant that must be enforced by the caller */ > + /* > + * By default, we return prev. Cases 3, 4, 8 will instead return next > + * and cases 3, 8 will also update vma to point at next. > + */ > + res = vma = prev; Later in the function there's a line: remove = remove2 = adjust = NULL; Now it would make sense to move it up here? > + > + /* Verify some invariant that must be enforced by the caller. */ > VM_WARN_ON(prev && addr <= prev->vm_start); > - VM_WARN_ON(curr && end > curr->vm_end); > + VM_WARN_ON(curr && (addr != curr->vm_start || end > curr->vm_end)); > VM_WARN_ON(addr >= end); > > if (prev) { > - res = prev; > - vma = prev; > vma_start = prev->vm_start; > vma_pgoff = prev->vm_pgoff; > /* Can we merge the predecessor? */ > @@ -957,6 +961,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > vma_prev(vmi); > } > } > + > /* Can we merge the successor? */ > if (next && mpol_equal(policy, vma_policy(next)) && > can_vma_merge_before(next, vm_flags, > @@ -997,6 +1002,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > adj_start = -(prev->vm_end - addr); > err = dup_anon_vma(next, prev); > } else { > + /* > + * Note that cases 3 and 8 are the ONLY ones where prev > + * is permitted to be (but is not necessarily) NULL. > + */ > vma = next; /* case 3 */ > vma_start = addr; > vma_end = next->vm_end;
* Lorenzo Stoakes <lstoakes@gmail.com> [230322 03:13]: > Previously, vma was an uninitialised variable which was only definitely > assigned as a result of the logic covering all possible input cases - for > it to have remained uninitialised, prev would have to be NULL, and next > would _have_ to be mergeable. > > The value of res defaults to NULL, so we can neatly eliminate the > assignment to res and vma in the if (prev) block and ensure that both res > and vma are both explicitly assigned, by just setting both to prev. > > In addition we add an explanation as to under what circumstances both might > change, and since we absolutely do rely on addr == curr->vm_start should > curr exist, assert that this is the case. > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com> > --- > mm/mmap.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index dbdbb92493b2..2a4f63716231 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -911,7 +911,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > { > pgoff_t pglen = (end - addr) >> PAGE_SHIFT; > pgoff_t vma_pgoff; > - struct vm_area_struct *curr, *next, *res = NULL; > + struct vm_area_struct *curr, *next, *res; > struct vm_area_struct *vma, *adjust, *remove, *remove2; > int err = -1; > bool merge_prev = false; > @@ -939,14 +939,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > else > next = NULL; /* case 5 */ > > - /* verify some invariant that must be enforced by the caller */ > + /* > + * By default, we return prev. Cases 3, 4, 8 will instead return next > + * and cases 3, 8 will also update vma to point at next. > + */ > + res = vma = prev; > + > + /* Verify some invariant that must be enforced by the caller. */ > VM_WARN_ON(prev && addr <= prev->vm_start); > - VM_WARN_ON(curr && end > curr->vm_end); > + VM_WARN_ON(curr && (addr != curr->vm_start || end > curr->vm_end)); > VM_WARN_ON(addr >= end); > > if (prev) { > - res = prev; > - vma = prev; > vma_start = prev->vm_start; > vma_pgoff = prev->vm_pgoff; > /* Can we merge the predecessor? */ > @@ -957,6 +961,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > vma_prev(vmi); > } > } > + > /* Can we merge the successor? */ > if (next && mpol_equal(policy, vma_policy(next)) && > can_vma_merge_before(next, vm_flags, > @@ -997,6 +1002,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > adj_start = -(prev->vm_end - addr); > err = dup_anon_vma(next, prev); > } else { > + /* > + * Note that cases 3 and 8 are the ONLY ones where prev > + * is permitted to be (but is not necessarily) NULL. > + */ > vma = next; /* case 3 */ > vma_start = addr; > vma_end = next->vm_end; > -- > 2.39.2 >
diff --git a/mm/mmap.c b/mm/mmap.c index dbdbb92493b2..2a4f63716231 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -911,7 +911,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, { pgoff_t pglen = (end - addr) >> PAGE_SHIFT; pgoff_t vma_pgoff; - struct vm_area_struct *curr, *next, *res = NULL; + struct vm_area_struct *curr, *next, *res; struct vm_area_struct *vma, *adjust, *remove, *remove2; int err = -1; bool merge_prev = false; @@ -939,14 +939,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, else next = NULL; /* case 5 */ - /* verify some invariant that must be enforced by the caller */ + /* + * By default, we return prev. Cases 3, 4, 8 will instead return next + * and cases 3, 8 will also update vma to point at next. + */ + res = vma = prev; + + /* Verify some invariant that must be enforced by the caller. */ VM_WARN_ON(prev && addr <= prev->vm_start); - VM_WARN_ON(curr && end > curr->vm_end); + VM_WARN_ON(curr && (addr != curr->vm_start || end > curr->vm_end)); VM_WARN_ON(addr >= end); if (prev) { - res = prev; - vma = prev; vma_start = prev->vm_start; vma_pgoff = prev->vm_pgoff; /* Can we merge the predecessor? */ @@ -957,6 +961,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, vma_prev(vmi); } } + /* Can we merge the successor? */ if (next && mpol_equal(policy, vma_policy(next)) && can_vma_merge_before(next, vm_flags, @@ -997,6 +1002,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, adj_start = -(prev->vm_end - addr); err = dup_anon_vma(next, prev); } else { + /* + * Note that cases 3 and 8 are the ONLY ones where prev + * is permitted to be (but is not necessarily) NULL. + */ vma = next; /* case 3 */ vma_start = addr; vma_end = next->vm_end;
Previously, vma was an uninitialised variable which was only definitely assigned as a result of the logic covering all possible input cases - for it to have remained uninitialised, prev would have to be NULL, and next would _have_ to be mergeable. The value of res defaults to NULL, so we can neatly eliminate the assignment to res and vma in the if (prev) block and ensure that both res and vma are both explicitly assigned, by just setting both to prev. In addition we add an explanation as to under what circumstances both might change, and since we absolutely do rely on addr == curr->vm_start should curr exist, assert that this is the case. Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> --- mm/mmap.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)