| Message ID | 20190814021755.1977-3-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] mm/mmap.c: prev could be retrieved from vma->vm_prev | expand |
Btw, is there any good reason we don't use a list_head for vma linkage?
On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote:
>Btw, is there any good reason we don't use a list_head for vma linkage?
Not sure, maybe there is some historical reason?
On 8/14/19 8:57 AM, Wei Yang wrote: > On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: >>Btw, is there any good reason we don't use a list_head for vma linkage? > > Not sure, maybe there is some historical reason? Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma list be doubly linked") and I guess it was just simpler to add the vm_prev link. Conversion to list_head might be an interesting project for some "advanced beginner" in the kernel :)
On Wed, Aug 14, 2019 at 11:19:37AM +0200, Vlastimil Babka wrote: >On 8/14/19 8:57 AM, Wei Yang wrote: >> On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: >>>Btw, is there any good reason we don't use a list_head for vma linkage? >> >> Not sure, maybe there is some historical reason? > >Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma >list be doubly linked") and I guess it was just simpler to add the vm_prev link. > >Conversion to list_head might be an interesting project for some "advanced >beginner" in the kernel :) Seems it will touch many code ...
On Wed, Aug 14, 2019 at 11:19:37AM +0200, Vlastimil Babka wrote: > On 8/14/19 8:57 AM, Wei Yang wrote: > > On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: > >>Btw, is there any good reason we don't use a list_head for vma linkage? > > > > Not sure, maybe there is some historical reason? > > Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma > list be doubly linked") and I guess it was just simpler to add the vm_prev link. > > Conversion to list_head might be an interesting project for some "advanced > beginner" in the kernel :) I'm working to get rid of vm_prev and vm_next, so it would probably be wasted effort.
On Tue, Aug 20, 2019 at 10:26:29AM -0700, Matthew Wilcox wrote: >On Wed, Aug 14, 2019 at 11:19:37AM +0200, Vlastimil Babka wrote: >> On 8/14/19 8:57 AM, Wei Yang wrote: >> > On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: >> >>Btw, is there any good reason we don't use a list_head for vma linkage? >> > >> > Not sure, maybe there is some historical reason? >> >> Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma >> list be doubly linked") and I guess it was just simpler to add the vm_prev link. >> >> Conversion to list_head might be an interesting project for some "advanced >> beginner" in the kernel :) > >I'm working to get rid of vm_prev and vm_next, so it would probably be >wasted effort. You mean replace it with list_head?
On Wed, Aug 21, 2019 at 08:52:34AM +0800, Wei Yang wrote: > On Tue, Aug 20, 2019 at 10:26:29AM -0700, Matthew Wilcox wrote: > >On Wed, Aug 14, 2019 at 11:19:37AM +0200, Vlastimil Babka wrote: > >> On 8/14/19 8:57 AM, Wei Yang wrote: > >> > On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: > >> >>Btw, is there any good reason we don't use a list_head for vma linkage? > >> > > >> > Not sure, maybe there is some historical reason? > >> > >> Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma > >> list be doubly linked") and I guess it was just simpler to add the vm_prev link. > >> > >> Conversion to list_head might be an interesting project for some "advanced > >> beginner" in the kernel :) > > > >I'm working to get rid of vm_prev and vm_next, so it would probably be > >wasted effort. > > You mean replace it with list_head? No, replace the rbtree with a new tree. https://lwn.net/Articles/787629/
On Tue, Aug 20, 2019 at 05:54:17PM -0700, Matthew Wilcox wrote: >On Wed, Aug 21, 2019 at 08:52:34AM +0800, Wei Yang wrote: >> On Tue, Aug 20, 2019 at 10:26:29AM -0700, Matthew Wilcox wrote: >> >On Wed, Aug 14, 2019 at 11:19:37AM +0200, Vlastimil Babka wrote: >> >> On 8/14/19 8:57 AM, Wei Yang wrote: >> >> > On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: >> >> >>Btw, is there any good reason we don't use a list_head for vma linkage? >> >> > >> >> > Not sure, maybe there is some historical reason? >> >> >> >> Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma >> >> list be doubly linked") and I guess it was just simpler to add the vm_prev link. >> >> >> >> Conversion to list_head might be an interesting project for some "advanced >> >> beginner" in the kernel :) >> > >> >I'm working to get rid of vm_prev and vm_next, so it would probably be >> >wasted effort. >> >> You mean replace it with list_head? > >No, replace the rbtree with a new tree. https://lwn.net/Articles/787629/ Sounds interesting. While I am not sure the plan is settled down, and how long it would take to replace the rb_tree with maple tree. I guess it would probably take some time to get merged upstream. IMHO, it would be good to have this cleanup in current kernel. Do you agree?
On Wed, Aug 21, 2019 at 09:22:44AM +0800, Wei Yang wrote: > On Tue, Aug 20, 2019 at 05:54:17PM -0700, Matthew Wilcox wrote: > >On Wed, Aug 21, 2019 at 08:52:34AM +0800, Wei Yang wrote: > >> On Tue, Aug 20, 2019 at 10:26:29AM -0700, Matthew Wilcox wrote: > >> >On Wed, Aug 14, 2019 at 11:19:37AM +0200, Vlastimil Babka wrote: > >> >> On 8/14/19 8:57 AM, Wei Yang wrote: > >> >> > On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: > >> >> >>Btw, is there any good reason we don't use a list_head for vma linkage? > >> >> > > >> >> > Not sure, maybe there is some historical reason? > >> >> > >> >> Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma > >> >> list be doubly linked") and I guess it was just simpler to add the vm_prev link. > >> >> > >> >> Conversion to list_head might be an interesting project for some "advanced > >> >> beginner" in the kernel :) > >> > > >> >I'm working to get rid of vm_prev and vm_next, so it would probably be > >> >wasted effort. > >> > >> You mean replace it with list_head? > > > >No, replace the rbtree with a new tree. https://lwn.net/Articles/787629/ > > Sounds interesting. > > While I am not sure the plan is settled down, and how long it would take to > replace the rb_tree with maple tree. I guess it would probably take some time > to get merged upstream. > > IMHO, it would be good to have this cleanup in current kernel. Do you agree? The three cleanups you've posted are fine. Doing more work (ie the list_head) seems like wasted effort to me.
On Tue, Aug 20, 2019 at 06:59:39PM -0700, Matthew Wilcox wrote: >On Wed, Aug 21, 2019 at 09:22:44AM +0800, Wei Yang wrote: >> On Tue, Aug 20, 2019 at 05:54:17PM -0700, Matthew Wilcox wrote: >> >On Wed, Aug 21, 2019 at 08:52:34AM +0800, Wei Yang wrote: >> >> On Tue, Aug 20, 2019 at 10:26:29AM -0700, Matthew Wilcox wrote: >> >> >On Wed, Aug 14, 2019 at 11:19:37AM +0200, Vlastimil Babka wrote: >> >> >> On 8/14/19 8:57 AM, Wei Yang wrote: >> >> >> > On Tue, Aug 13, 2019 at 10:16:11PM -0700, Christoph Hellwig wrote: >> >> >> >>Btw, is there any good reason we don't use a list_head for vma linkage? >> >> >> > >> >> >> > Not sure, maybe there is some historical reason? >> >> >> >> >> >> Seems it was single-linked until 2010 commit 297c5eee3724 ("mm: make the vma >> >> >> list be doubly linked") and I guess it was just simpler to add the vm_prev link. >> >> >> >> >> >> Conversion to list_head might be an interesting project for some "advanced >> >> >> beginner" in the kernel :) >> >> > >> >> >I'm working to get rid of vm_prev and vm_next, so it would probably be >> >> >wasted effort. >> >> >> >> You mean replace it with list_head? >> > >> >No, replace the rbtree with a new tree. https://lwn.net/Articles/787629/ >> >> Sounds interesting. >> >> While I am not sure the plan is settled down, and how long it would take to >> replace the rb_tree with maple tree. I guess it would probably take some time >> to get merged upstream. >> >> IMHO, it would be good to have this cleanup in current kernel. Do you agree? > >The three cleanups you've posted are fine. Doing more work (ie the >list_head) seems like wasted effort to me. Ah, got your point. I misunderstand it.
diff --git a/mm/internal.h b/mm/internal.h index 41a49574acc3..4736aeb37dae 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -291,6 +291,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); +void __vma_unlink_list(struct mm_struct *mm, struct vm_area_struct *vma); #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 3fde0ec18554..aa66753b175e 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -677,18 +677,8 @@ static __always_inline void __vma_unlink_common(struct mm_struct *mm, struct vm_area_struct *vma, struct vm_area_struct *ignore) { - struct vm_area_struct *prev, *next; - vma_rb_erase_ignore(vma, &mm->mm_rb, ignore); - next = vma->vm_next; - prev = vma->vm_prev; - if (prev) - prev->vm_next = next; - else - mm->mmap = next; - if (next) - next->vm_prev = prev; - + __vma_unlink_list(mm, vma); /* Kill the cache */ vmacache_invalidate(mm); } diff --git a/mm/nommu.c b/mm/nommu.c index 12a66fbeb988..1a403f65b99e 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -673,13 +673,7 @@ static void delete_vma_from_mm(struct vm_area_struct *vma) /* remove from the MM's tree and list */ rb_erase(&vma->vm_rb, &mm->mm_rb); - if (vma->vm_prev) - vma->vm_prev->vm_next = vma->vm_next; - else - mm->mmap = vma->vm_next; - - if (vma->vm_next) - vma->vm_next->vm_prev = vma->vm_prev; + __vma_unlink_list(mm, vma); } /* diff --git a/mm/util.c b/mm/util.c index 80632db29247..5f113cd0acad 100644 --- a/mm/util.c +++ b/mm/util.c @@ -281,6 +281,20 @@ void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma, next->vm_prev = vma; } +void __vma_unlink_list(struct mm_struct *mm, struct vm_area_struct *vma) +{ + struct vm_area_struct *prev, *next; + + next = vma->vm_next; + prev = vma->vm_prev; + if (prev) + prev->vm_next = next; + else + mm->mmap = next; + if (next) + next->vm_prev = prev; +} + /* Check if the vma is being used as a stack by this task */ int vma_is_stack_for_current(struct vm_area_struct *vma) {
Just make the code a little easy to read. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- Note: For nommu part, the code is not tested. --- mm/internal.h | 1 + mm/mmap.c | 12 +----------- mm/nommu.c | 8 +------- mm/util.c | 14 ++++++++++++++ 4 files changed, 17 insertions(+), 18 deletions(-)