Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] mm/mmap.c: rb_parent is not necessary in __vma_link_list
@ 2019-08-13  3:26 Wei Yang
  2019-08-13  3:39 ` Matthew Wilcox
  0 siblings, 1 reply; 4+ messages in thread
From: Wei Yang @ 2019-08-13  3:26 UTC (permalink / raw)
  To: akpm, mgorman, vbabka; +Cc: linux-mm, linux-kernel, Wei Yang

Now we use rb_parent to get next, while this is not necessary.

When prev is NULL, this means vma should be the first element in the
list. Then next should be current first one (mm->mmap), no matter
whether we have parent or not.

After removing it, the code shows the beauty of symmetry.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 mm/internal.h | 2 +-
 mm/mmap.c     | 2 +-
 mm/nommu.c    | 2 +-
 mm/util.c     | 8 ++------
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index e32390802fd3..41a49574acc3 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -290,7 +290,7 @@ static inline bool is_data_mapping(vm_flags_t flags)
 
 /* mm/util.c */
 void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
-		struct vm_area_struct *prev, struct rb_node *rb_parent);
+		struct vm_area_struct *prev);
 
 #ifdef CONFIG_MMU
 extern long populate_vma_page_range(struct vm_area_struct *vma,
diff --git a/mm/mmap.c b/mm/mmap.c
index f7ed0afb994c..b8072630766f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -632,7 +632,7 @@ __vma_link(struct mm_struct *mm, struct vm_area_struct *vma,
 	struct vm_area_struct *prev, struct rb_node **rb_link,
 	struct rb_node *rb_parent)
 {
-	__vma_link_list(mm, vma, prev, rb_parent);
+	__vma_link_list(mm, vma, prev);
 	__vma_link_rb(mm, vma, rb_link, rb_parent);
 }
 
diff --git a/mm/nommu.c b/mm/nommu.c
index fed1b6e9c89b..12a66fbeb988 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -637,7 +637,7 @@ static void add_vma_to_mm(struct mm_struct *mm, struct vm_area_struct *vma)
 	if (rb_prev)
 		prev = rb_entry(rb_prev, struct vm_area_struct, vm_rb);
 
-	__vma_link_list(mm, vma, prev, parent);
+	__vma_link_list(mm, vma, prev);
 }
 
 /*
diff --git a/mm/util.c b/mm/util.c
index e6351a80f248..80632db29247 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -264,7 +264,7 @@ void *memdup_user_nul(const void __user *src, size_t len)
 EXPORT_SYMBOL(memdup_user_nul);
 
 void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
-		struct vm_area_struct *prev, struct rb_node *rb_parent)
+		struct vm_area_struct *prev)
 {
 	struct vm_area_struct *next;
 
@@ -273,12 +273,8 @@ void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
 		next = prev->vm_next;
 		prev->vm_next = vma;
 	} else {
+		next = mm->mmap;
 		mm->mmap = vma;
-		if (rb_parent)
-			next = rb_entry(rb_parent,
-					struct vm_area_struct, vm_rb);
-		else
-			next = NULL;
 	}
 	vma->vm_next = next;
 	if (next)
-- 
2.17.1



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/mmap.c: rb_parent is not necessary in __vma_link_list
  2019-08-13  3:26 [PATCH] mm/mmap.c: rb_parent is not necessary in __vma_link_list Wei Yang
@ 2019-08-13  3:39 ` Matthew Wilcox
  2019-08-13  5:25   ` Wei Yang
  2019-08-14  2:19   ` Wei Yang
  0 siblings, 2 replies; 4+ messages in thread
From: Matthew Wilcox @ 2019-08-13  3:39 UTC (permalink / raw)
  To: Wei Yang; +Cc: akpm, mgorman, vbabka, linux-mm, linux-kernel

On Tue, Aug 13, 2019 at 11:26:56AM +0800, Wei Yang wrote:
> Now we use rb_parent to get next, while this is not necessary.
> 
> When prev is NULL, this means vma should be the first element in the
> list. Then next should be current first one (mm->mmap), no matter
> whether we have parent or not.
> 
> After removing it, the code shows the beauty of symmetry.

Uhh ... did you test this?

> @@ -273,12 +273,8 @@ void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
>  		next = prev->vm_next;
>  		prev->vm_next = vma;
>  	} else {
> +		next = mm->mmap;
>  		mm->mmap = vma;
> -		if (rb_parent)
> -			next = rb_entry(rb_parent,
> -					struct vm_area_struct, vm_rb);
> -		else
> -			next = NULL;
>  	}

The full context is:

        if (prev) {
                next = prev->vm_next;
                prev->vm_next = vma;
        } else {
                mm->mmap = vma;
                if (rb_parent)
                        next = rb_entry(rb_parent,
                                        struct vm_area_struct, vm_rb);
                else
                        next = NULL;
        }

Let's imagine we have a small tree with three ranges in it.

A: 5-7
B: 8-10
C: 11-13

I would imagine an rbtree for this case has B at the top with A
to its left and B to its right.

Now we're going to add range D at 3-4.  'next' should clearly be range A.
It will have NULL prev.  Your code is going to make 'B' next, not A.
Right?


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/mmap.c: rb_parent is not necessary in __vma_link_list
  2019-08-13  3:39 ` Matthew Wilcox
@ 2019-08-13  5:25   ` Wei Yang
  2019-08-14  2:19   ` Wei Yang
  1 sibling, 0 replies; 4+ messages in thread
From: Wei Yang @ 2019-08-13  5:25 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Wei Yang, akpm, mgorman, vbabka, linux-mm, linux-kernel

On Mon, Aug 12, 2019 at 08:39:58PM -0700, Matthew Wilcox wrote:
>On Tue, Aug 13, 2019 at 11:26:56AM +0800, Wei Yang wrote:
>> Now we use rb_parent to get next, while this is not necessary.
>> 
>> When prev is NULL, this means vma should be the first element in the
>> list. Then next should be current first one (mm->mmap), no matter
>> whether we have parent or not.
>> 
>> After removing it, the code shows the beauty of symmetry.
>
>Uhh ... did you test this?
>

I reboot successfully with this patch.

>> @@ -273,12 +273,8 @@ void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
>>  		next = prev->vm_next;
>>  		prev->vm_next = vma;
>>  	} else {
>> +		next = mm->mmap;
>>  		mm->mmap = vma;
>> -		if (rb_parent)
>> -			next = rb_entry(rb_parent,
>> -					struct vm_area_struct, vm_rb);
>> -		else
>> -			next = NULL;
>>  	}
>
>The full context is:
>
>        if (prev) {
>                next = prev->vm_next;
>                prev->vm_next = vma;
>        } else {
>                mm->mmap = vma;
>                if (rb_parent)
>                        next = rb_entry(rb_parent,
>                                        struct vm_area_struct, vm_rb);
>                else
>                        next = NULL;
>        }
>
>Let's imagine we have a small tree with three ranges in it.
>
>A: 5-7
>B: 8-10
>C: 11-13
>
>I would imagine an rbtree for this case has B at the top with A
>to its left and B to its right.
>
>Now we're going to add range D at 3-4.  'next' should clearly be range A.
>It will have NULL prev.  Your code is going to make 'B' next, not A.
>Right?

mm->mmap is not the rb_root.

mm->mmap is the first element in the ordered list, if my understanding is
correct.

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/mmap.c: rb_parent is not necessary in __vma_link_list
  2019-08-13  3:39 ` Matthew Wilcox
  2019-08-13  5:25   ` Wei Yang
@ 2019-08-14  2:19   ` Wei Yang
  1 sibling, 0 replies; 4+ messages in thread
From: Wei Yang @ 2019-08-14  2:19 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Wei Yang, akpm, mgorman, vbabka, linux-mm, linux-kernel

On Mon, Aug 12, 2019 at 08:39:58PM -0700, Matthew Wilcox wrote:
>On Tue, Aug 13, 2019 at 11:26:56AM +0800, Wei Yang wrote:
>> Now we use rb_parent to get next, while this is not necessary.
>> 
>> When prev is NULL, this means vma should be the first element in the
>> list. Then next should be current first one (mm->mmap), no matter
>> whether we have parent or not.
>> 
>> After removing it, the code shows the beauty of symmetry.
>
>Uhh ... did you test this?
>

I have enabled DEBUG_VM_RB, system looks good with this.

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13  3:26 [PATCH] mm/mmap.c: rb_parent is not necessary in __vma_link_list Wei Yang
2019-08-13  3:39 ` Matthew Wilcox
2019-08-13  5:25   ` Wei Yang
2019-08-14  2:19   ` Wei Yang

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org linux-mm@archiver.kernel.org
	public-inbox-index linux-mm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox