* [patch] mmap: feed back correct prev vma when finding vma
@ 2012-08-09 12:21 Hillf Danton
2012-08-10 1:26 ` Hugh Dickins
0 siblings, 1 reply; 5+ messages in thread
From: Hillf Danton @ 2012-08-09 12:21 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Hugh Dickins, Mikulas Patocka, Andrew Morton, LKML, Linux-MM,
Hillf Danton
After walking rb tree, if vma is determined, prev vma has to be determined
based on vma; and rb_prev should be considered only if no vma determined.
Signed-off-by: Hillf Danton <dhillf@gmail.com>
---
--- a/mm/mmap.c Fri Aug 3 07:38:10 2012
+++ b/mm/mmap.c Mon Aug 6 20:10:18 2012
@@ -385,9 +385,13 @@ find_vma_prepare(struct mm_struct *mm, u
}
}
- *pprev = NULL;
- if (rb_prev)
- *pprev = rb_entry(rb_prev, struct vm_area_struct, vm_rb);
+ if (vma) {
+ *pprev = vma->vm_prev;
+ } else {
+ *pprev = NULL;
+ if (rb_prev)
+ *pprev = rb_entry(rb_prev, struct vm_area_struct, vm_rb);
+ }
*rb_link = __rb_link;
*rb_parent = __rb_parent;
return vma;
--
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch] mmap: feed back correct prev vma when finding vma 2012-08-09 12:21 [patch] mmap: feed back correct prev vma when finding vma Hillf Danton @ 2012-08-10 1:26 ` Hugh Dickins 2012-08-10 12:00 ` Hillf Danton 0 siblings, 1 reply; 5+ messages in thread From: Hugh Dickins @ 2012-08-10 1:26 UTC (permalink / raw) To: Hillf Danton Cc: KOSAKI Motohiro, Mikulas Patocka, Andrew Morton, LKML, Linux-MM On Thu, 9 Aug 2012, Hillf Danton wrote: > After walking rb tree, if vma is determined, prev vma has to be determined > based on vma; and rb_prev should be considered only if no vma determined. Why? Because you think more code is better code? I disagree. If you have seen a bug here, please tell how to reproduce it. I have not heard of a bug here: I think you're saying, if the rbtree were inconsistent with the vma list, then you think it would be a good idea to believe the vma list instead of the rbtree where there's a choice. But the rbtree had better not be inconsistent with the vma list. Hugh > > Signed-off-by: Hillf Danton <dhillf@gmail.com> > --- > > --- a/mm/mmap.c Fri Aug 3 07:38:10 2012 > +++ b/mm/mmap.c Mon Aug 6 20:10:18 2012 > @@ -385,9 +385,13 @@ find_vma_prepare(struct mm_struct *mm, u > } > } > > - *pprev = NULL; > - if (rb_prev) > - *pprev = rb_entry(rb_prev, struct vm_area_struct, vm_rb); > + if (vma) { > + *pprev = vma->vm_prev; > + } else { > + *pprev = NULL; > + if (rb_prev) > + *pprev = rb_entry(rb_prev, struct vm_area_struct, vm_rb); > + } > *rb_link = __rb_link; > *rb_parent = __rb_parent; > return vma; > -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] mmap: feed back correct prev vma when finding vma 2012-08-10 1:26 ` Hugh Dickins @ 2012-08-10 12:00 ` Hillf Danton 2012-08-14 2:17 ` Hugh Dickins 0 siblings, 1 reply; 5+ messages in thread From: Hillf Danton @ 2012-08-10 12:00 UTC (permalink / raw) To: Hugh Dickins Cc: KOSAKI Motohiro, Mikulas Patocka, Andrew Morton, LKML, Linux-MM On Fri, Aug 10, 2012 at 9:26 AM, Hugh Dickins <hughd@google.com> wrote: > On Thu, 9 Aug 2012, Hillf Danton wrote: >> After walking rb tree, if vma is determined, prev vma has to be determined >> based on vma; and rb_prev should be considered only if no vma determined. > > Why? Because you think more code is better code? I disagree. s/more/correct/ Because feedback is incorrect if we return vma corresponding to the root node. Hillf ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] mmap: feed back correct prev vma when finding vma 2012-08-10 12:00 ` Hillf Danton @ 2012-08-14 2:17 ` Hugh Dickins 2012-08-14 11:56 ` Hillf Danton 0 siblings, 1 reply; 5+ messages in thread From: Hugh Dickins @ 2012-08-14 2:17 UTC (permalink / raw) To: Hillf Danton Cc: KOSAKI Motohiro, Mikulas Patocka, Andrew Morton, Benny Halevy, LKML, Linux-MM On Fri, 10 Aug 2012, Hillf Danton wrote: > On Fri, Aug 10, 2012 at 9:26 AM, Hugh Dickins <hughd@google.com> wrote: > > On Thu, 9 Aug 2012, Hillf Danton wrote: > >> After walking rb tree, if vma is determined, prev vma has to be determined > >> based on vma; and rb_prev should be considered only if no vma determined. > > > > Why? Because you think more code is better code? I disagree. > > s/more/correct/ > > Because feedback is incorrect if we return vma corresponding to > the root node. "feedback"? "root node"? I took another look, and a WARN_ON soon told me that you do have a point. It stirred memories, and I found an earlier thread from 2008, responsible for replacing the original "return vma" by a "break" in 2.6.27. I agree that find_vma_prepare() is confusing, but your vm_prev patch is not the right fix: let's un-confuse it this way, which barely needs comment. Hugh [PATCH] mm: replace find_vma_prepare by clearer find_vma_links People get confused by find_vma_prepare(), because it doesn't care about what it returns in its output args, when its callers won't be interested. Clarify by passing in end-of-range address too, and returning failure if any existing vma overlaps the new range: instead of returning an ambiguous vma which most callers then must check. find_vma_links() is a clearer name. This does revert 2.6.27's dfe195fb79e88 ("mm: fix uninitialized variables for find_vma_prepare callers"), but it looks like gcc 4.3.0 was one of those releases too eager to shout about uninitialized variables: only copy_vma() warns with 4.5.1 and 4.7.1, which a BUG on error silences. Signed-off-by: Hugh Dickins <hughd@google.com> Cc: Benny Halevy <bhalevy@tonian.com> Cc: Hillf Danton <dhillf@gmail.com> --- mm/mmap.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-) --- 3.6-rc1/mm/mmap.c 2012-08-03 08:31:27.064842271 -0700 +++ linux/mm/mmap.c 2012-08-13 12:23:35.862895633 -0700 @@ -356,17 +356,14 @@ void validate_mm(struct mm_struct *mm) #define validate_mm(mm) do { } while (0) #endif -static struct vm_area_struct * -find_vma_prepare(struct mm_struct *mm, unsigned long addr, - struct vm_area_struct **pprev, struct rb_node ***rb_link, - struct rb_node ** rb_parent) +static int find_vma_links(struct mm_struct *mm, unsigned long addr, + unsigned long end, struct vm_area_struct **pprev, + struct rb_node ***rb_link, struct rb_node **rb_parent) { - struct vm_area_struct * vma; - struct rb_node ** __rb_link, * __rb_parent, * rb_prev; + struct rb_node **__rb_link, *__rb_parent, *rb_prev; __rb_link = &mm->mm_rb.rb_node; rb_prev = __rb_parent = NULL; - vma = NULL; while (*__rb_link) { struct vm_area_struct *vma_tmp; @@ -375,9 +372,9 @@ find_vma_prepare(struct mm_struct *mm, u vma_tmp = rb_entry(__rb_parent, struct vm_area_struct, vm_rb); if (vma_tmp->vm_end > addr) { - vma = vma_tmp; - if (vma_tmp->vm_start <= addr) - break; + /* Fail if an existing vma overlaps the area */ + if (vma_tmp->vm_start < end) + return -ENOMEM; __rb_link = &__rb_parent->rb_left; } else { rb_prev = __rb_parent; @@ -390,7 +387,7 @@ find_vma_prepare(struct mm_struct *mm, u *pprev = rb_entry(rb_prev, struct vm_area_struct, vm_rb); *rb_link = __rb_link; *rb_parent = __rb_parent; - return vma; + return 0; } void __vma_link_rb(struct mm_struct *mm, struct vm_area_struct *vma, @@ -459,11 +456,12 @@ static void vma_link(struct mm_struct *m */ static void __insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma) { - struct vm_area_struct *__vma, *prev; + struct vm_area_struct *prev; struct rb_node **rb_link, *rb_parent; - __vma = find_vma_prepare(mm, vma->vm_start,&prev, &rb_link, &rb_parent); - BUG_ON(__vma && __vma->vm_start < vma->vm_end); + if (find_vma_links(mm, vma->vm_start, vma->vm_end, + &prev, &rb_link, &rb_parent)) + BUG(); __vma_link(mm, vma, prev, rb_link, rb_parent); mm->map_count++; } @@ -1229,8 +1227,7 @@ unsigned long mmap_region(struct file *f /* Clear old maps */ error = -ENOMEM; munmap_back: - vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent); - if (vma && vma->vm_start < addr + len) { + if (find_vma_links(mm, addr, addr + len, &prev, &rb_link, &rb_parent)) { if (do_munmap(mm, addr, len)) return -ENOMEM; goto munmap_back; @@ -2201,8 +2198,7 @@ static unsigned long do_brk(unsigned lon * Clear old maps. this also does some error checking for us */ munmap_back: - vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent); - if (vma && vma->vm_start < addr + len) { + if (find_vma_links(mm, addr, addr + len, &prev, &rb_link, &rb_parent)) { if (do_munmap(mm, addr, len)) return -ENOMEM; goto munmap_back; @@ -2316,10 +2312,10 @@ void exit_mmap(struct mm_struct *mm) * and into the inode's i_mmap tree. If vm_file is non-NULL * then i_mmap_mutex is taken here. */ -int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma) +int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma) { - struct vm_area_struct * __vma, * prev; - struct rb_node ** rb_link, * rb_parent; + struct vm_area_struct *prev; + struct rb_node **rb_link, *rb_parent; /* * The vm_pgoff of a purely anonymous vma should be irrelevant @@ -2337,8 +2333,8 @@ int insert_vm_struct(struct mm_struct * BUG_ON(vma->anon_vma); vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT; } - __vma = find_vma_prepare(mm,vma->vm_start,&prev,&rb_link,&rb_parent); - if (__vma && __vma->vm_start < vma->vm_end) + if (find_vma_links(mm, vma->vm_start, vma->vm_end, + &prev, &rb_link, &rb_parent)) return -ENOMEM; if ((vma->vm_flags & VM_ACCOUNT) && security_vm_enough_memory_mm(mm, vma_pages(vma))) @@ -2372,7 +2368,8 @@ struct vm_area_struct *copy_vma(struct v faulted_in_anon_vma = false; } - find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent); + if (find_vma_links(mm, addr, addr + len, &prev, &rb_link, &rb_parent)) + BUG(); new_vma = vma_merge(mm, prev, addr, addr + len, vma->vm_flags, vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma)); if (new_vma) { ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] mmap: feed back correct prev vma when finding vma 2012-08-14 2:17 ` Hugh Dickins @ 2012-08-14 11:56 ` Hillf Danton 0 siblings, 0 replies; 5+ messages in thread From: Hillf Danton @ 2012-08-14 11:56 UTC (permalink / raw) To: Hugh Dickins Cc: KOSAKI Motohiro, Mikulas Patocka, Andrew Morton, Benny Halevy, LKML, Linux-MM On Tue, Aug 14, 2012 at 10:17 AM, Hugh Dickins <hughd@google.com> wrote: > [PATCH] mm: replace find_vma_prepare by clearer find_vma_links > > People get confused by find_vma_prepare(), because it doesn't care about > what it returns in its output args, when its callers won't be interested. > > Clarify by passing in end-of-range address too, and returning failure if > any existing vma overlaps the new range: instead of returning an ambiguous > vma which most callers then must check. find_vma_links() is a clearer name. > > This does revert 2.6.27's dfe195fb79e88 ("mm: fix uninitialized variables > for find_vma_prepare callers"), but it looks like gcc 4.3.0 was one of > those releases too eager to shout about uninitialized variables: only > copy_vma() warns with 4.5.1 and 4.7.1, which a BUG on error silences. > > Signed-off-by: Hugh Dickins <hughd@google.com> > Cc: Benny Halevy <bhalevy@tonian.com> Acked-by: Hillf Danton <dhillf@gmail.com> > mm/mmap.c | 45 +++++++++++++++++++++------------------------ > 1 file changed, 21 insertions(+), 24 deletions(-) > > --- 3.6-rc1/mm/mmap.c 2012-08-03 08:31:27.064842271 -0700 > +++ linux/mm/mmap.c 2012-08-13 12:23:35.862895633 -0700 > @@ -356,17 +356,14 @@ void validate_mm(struct mm_struct *mm) > #define validate_mm(mm) do { } while (0) > #endif > > -static struct vm_area_struct * > -find_vma_prepare(struct mm_struct *mm, unsigned long addr, > - struct vm_area_struct **pprev, struct rb_node ***rb_link, > - struct rb_node ** rb_parent) > +static int find_vma_links(struct mm_struct *mm, unsigned long addr, > + unsigned long end, struct vm_area_struct **pprev, > + struct rb_node ***rb_link, struct rb_node **rb_parent) > { > - struct vm_area_struct * vma; > - struct rb_node ** __rb_link, * __rb_parent, * rb_prev; > + struct rb_node **__rb_link, *__rb_parent, *rb_prev; > Just a nitpick, we could further cut a couple of lines if rb_prev is replaced by vma. > __rb_link = &mm->mm_rb.rb_node; > rb_prev = __rb_parent = NULL; > - vma = NULL; > > while (*__rb_link) { > struct vm_area_struct *vma_tmp; > @@ -375,9 +372,9 @@ find_vma_prepare(struct mm_struct *mm, u > vma_tmp = rb_entry(__rb_parent, struct vm_area_struct, vm_rb); > > if (vma_tmp->vm_end > addr) { > - vma = vma_tmp; > - if (vma_tmp->vm_start <= addr) > - break; > + /* Fail if an existing vma overlaps the area */ > + if (vma_tmp->vm_start < end) > + return -ENOMEM; > __rb_link = &__rb_parent->rb_left; > } else { > rb_prev = __rb_parent; > @@ -390,7 +387,7 @@ find_vma_prepare(struct mm_struct *mm, u > *pprev = rb_entry(rb_prev, struct vm_area_struct, vm_rb); > *rb_link = __rb_link; > *rb_parent = __rb_parent; > - return vma; > + return 0; > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-08-14 11:56 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-09 12:21 [patch] mmap: feed back correct prev vma when finding vma Hillf Danton 2012-08-10 1:26 ` Hugh Dickins 2012-08-10 12:00 ` Hillf Danton 2012-08-14 2:17 ` Hugh Dickins 2012-08-14 11:56 ` Hillf Danton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).