* [PATCH] mm/mmap: remove the mm parameter in vma_complete() @ 2024-01-29 7:53 Yajun Deng 2024-01-29 15:04 ` Liam R. Howlett 0 siblings, 1 reply; 5+ messages in thread From: Yajun Deng @ 2024-01-29 7:53 UTC (permalink / raw) To: akpm; +Cc: Liam.Howlett, linux-mm, linux-kernel, Yajun Deng There are vma_merge() and do_brk_flags() pass mm to vma_complete(), others would pass the vma->vm_mm. The following explains that the mm is the vma->vm_mm in vma_merge() and do_brk_flags(). All vma will point to the same mm struct if the vma_merge() is successful. So the mm and the vma->mm are the same. vm_brk_flags() and brk syscall will initialize vmi with current->mm, so the vma->vm_mm and the current->mm are the same if vma exists in do_brk_flags(). Remove the mm parameter in vma_complete() and get mm from the vma in vp. Signed-off-by: Yajun Deng <yajun.deng@linux.dev> --- mm/mmap.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/mm/mmap.c b/mm/mmap.c index e97b9144c61a..9b968d1edf55 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -509,11 +509,11 @@ static inline void vma_prepare(struct vma_prepare *vp) * * @vp: The vma_prepare struct * @vmi: The vma iterator - * @mm: The mm_struct */ -static inline void vma_complete(struct vma_prepare *vp, - struct vma_iterator *vmi, struct mm_struct *mm) +static inline void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi) { + struct mm_struct *mm = vp->vma->vm_mm; + if (vp->file) { if (vp->adj_next) vma_interval_tree_insert(vp->adj_next, @@ -666,7 +666,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma, vma_set_range(vma, start, end, pgoff); vma_iter_store(vmi, vma); - vma_complete(&vp, vmi, vma->vm_mm); + vma_complete(&vp, vmi); return 0; nomem: @@ -707,7 +707,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, vma_iter_clear(vmi); vma_set_range(vma, start, end, pgoff); - vma_complete(&vp, vmi, vma->vm_mm); + vma_complete(&vp, vmi); return 0; } @@ -1030,7 +1030,7 @@ static struct vm_area_struct } } - vma_complete(&vp, vmi, mm); + vma_complete(&vp, vmi); khugepaged_enter_vma(res, vm_flags); return res; @@ -2377,7 +2377,7 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, } /* vma_complete stores the new vma */ - vma_complete(&vp, vmi, vma->vm_mm); + vma_complete(&vp, vmi); /* Success. */ if (new_below) @@ -3145,7 +3145,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma, vm_flags_set(vma, VM_SOFTDIRTY); vma_iter_store(vmi, vma); - vma_complete(&vp, vmi, mm); + vma_complete(&vp, vmi); khugepaged_enter_vma(vma, flags); goto out; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: remove the mm parameter in vma_complete() 2024-01-29 7:53 [PATCH] mm/mmap: remove the mm parameter in vma_complete() Yajun Deng @ 2024-01-29 15:04 ` Liam R. Howlett 2024-02-22 10:26 ` Yajun Deng 0 siblings, 1 reply; 5+ messages in thread From: Liam R. Howlett @ 2024-01-29 15:04 UTC (permalink / raw) To: Yajun Deng; +Cc: akpm, linux-mm, linux-kernel * Yajun Deng <yajun.deng@linux.dev> [240129 02:53]: > There are vma_merge() and do_brk_flags() pass mm to vma_complete(), others > would pass the vma->vm_mm. The following explains that the mm is the > vma->vm_mm in vma_merge() and do_brk_flags(). > > All vma will point to the same mm struct if the vma_merge() is successful. > So the mm and the vma->mm are the same. Absolutely, they must be the same. I don't think vma_merge() checks this, but it is true. > > vm_brk_flags() and brk syscall will initialize vmi with current->mm, > so the vma->vm_mm and the current->mm are the same if vma exists in > do_brk_flags(). > > Remove the mm parameter in vma_complete() and get mm from the vma in vp. You have added a dereference to the two paths that don't need it to reduce the argument list from 3 to 2. It's the same number of lines as well. vma_shrink() is only used on process creation, but brk is more common. Note that this function is marked as inline. I'm not sure this change is worth making. > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > --- > mm/mmap.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index e97b9144c61a..9b968d1edf55 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -509,11 +509,11 @@ static inline void vma_prepare(struct vma_prepare *vp) > * > * @vp: The vma_prepare struct > * @vmi: The vma iterator > - * @mm: The mm_struct > */ > -static inline void vma_complete(struct vma_prepare *vp, > - struct vma_iterator *vmi, struct mm_struct *mm) > +static inline void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi) > { > + struct mm_struct *mm = vp->vma->vm_mm; > + > if (vp->file) { > if (vp->adj_next) > vma_interval_tree_insert(vp->adj_next, > @@ -666,7 +666,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma, > vma_set_range(vma, start, end, pgoff); > vma_iter_store(vmi, vma); > > - vma_complete(&vp, vmi, vma->vm_mm); > + vma_complete(&vp, vmi); > return 0; > > nomem: > @@ -707,7 +707,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, > > vma_iter_clear(vmi); > vma_set_range(vma, start, end, pgoff); > - vma_complete(&vp, vmi, vma->vm_mm); > + vma_complete(&vp, vmi); > return 0; > } > > @@ -1030,7 +1030,7 @@ static struct vm_area_struct > } > } > > - vma_complete(&vp, vmi, mm); > + vma_complete(&vp, vmi); > khugepaged_enter_vma(res, vm_flags); > return res; > > @@ -2377,7 +2377,7 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > } > > /* vma_complete stores the new vma */ > - vma_complete(&vp, vmi, vma->vm_mm); > + vma_complete(&vp, vmi); > > /* Success. */ > if (new_below) > @@ -3145,7 +3145,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma, > vm_flags_set(vma, VM_SOFTDIRTY); > vma_iter_store(vmi, vma); > > - vma_complete(&vp, vmi, mm); > + vma_complete(&vp, vmi); > khugepaged_enter_vma(vma, flags); > goto out; > } > -- > 2.25.1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: remove the mm parameter in vma_complete() 2024-01-29 15:04 ` Liam R. Howlett @ 2024-02-22 10:26 ` Yajun Deng 2024-02-22 16:22 ` Liam R. Howlett 0 siblings, 1 reply; 5+ messages in thread From: Yajun Deng @ 2024-02-22 10:26 UTC (permalink / raw) To: Liam R. Howlett; +Cc: akpm, linux-mm, linux-kernel, vbabka, lstoakes Adding Vlastimil and Lorenzo to discuss this patch. On 2024/1/29 23:04, Liam R. Howlett wrote: > * Yajun Deng <yajun.deng@linux.dev> [240129 02:53]: >> There are vma_merge() and do_brk_flags() pass mm to vma_complete(), others >> would pass the vma->vm_mm. The following explains that the mm is the >> vma->vm_mm in vma_merge() and do_brk_flags(). >> >> All vma will point to the same mm struct if the vma_merge() is successful. >> So the mm and the vma->mm are the same. > Absolutely, they must be the same. I don't think vma_merge() checks > this, but it is true. > >> vm_brk_flags() and brk syscall will initialize vmi with current->mm, >> so the vma->vm_mm and the current->mm are the same if vma exists in >> do_brk_flags(). >> >> Remove the mm parameter in vma_complete() and get mm from the vma in vp. > You have added a dereference to the two paths that don't need it to > reduce the argument list from 3 to 2. It's the same number of lines as > well. vma_shrink() is only used on process creation, but brk is more > common. Note that this function is marked as inline. > > I'm not sure this change is worth making. If we can make sure the mm is vma->vm_mm, I don't think we need to pass the mm. If we can't make sure that, this change is not worth it. >> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >> --- >> mm/mmap.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/mm/mmap.c b/mm/mmap.c >> index e97b9144c61a..9b968d1edf55 100644 >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -509,11 +509,11 @@ static inline void vma_prepare(struct vma_prepare *vp) >> * >> * @vp: The vma_prepare struct >> * @vmi: The vma iterator >> - * @mm: The mm_struct >> */ >> -static inline void vma_complete(struct vma_prepare *vp, >> - struct vma_iterator *vmi, struct mm_struct *mm) >> +static inline void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi) >> { >> + struct mm_struct *mm = vp->vma->vm_mm; >> + >> if (vp->file) { >> if (vp->adj_next) >> vma_interval_tree_insert(vp->adj_next, >> @@ -666,7 +666,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma, >> vma_set_range(vma, start, end, pgoff); >> vma_iter_store(vmi, vma); >> >> - vma_complete(&vp, vmi, vma->vm_mm); >> + vma_complete(&vp, vmi); >> return 0; >> >> nomem: >> @@ -707,7 +707,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, >> >> vma_iter_clear(vmi); >> vma_set_range(vma, start, end, pgoff); >> - vma_complete(&vp, vmi, vma->vm_mm); >> + vma_complete(&vp, vmi); >> return 0; >> } >> >> @@ -1030,7 +1030,7 @@ static struct vm_area_struct >> } >> } >> >> - vma_complete(&vp, vmi, mm); >> + vma_complete(&vp, vmi); >> khugepaged_enter_vma(res, vm_flags); >> return res; >> >> @@ -2377,7 +2377,7 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, >> } >> >> /* vma_complete stores the new vma */ >> - vma_complete(&vp, vmi, vma->vm_mm); >> + vma_complete(&vp, vmi); >> >> /* Success. */ >> if (new_below) >> @@ -3145,7 +3145,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma, >> vm_flags_set(vma, VM_SOFTDIRTY); >> vma_iter_store(vmi, vma); >> >> - vma_complete(&vp, vmi, mm); >> + vma_complete(&vp, vmi); >> khugepaged_enter_vma(vma, flags); >> goto out; >> } >> -- >> 2.25.1 >> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: remove the mm parameter in vma_complete() 2024-02-22 10:26 ` Yajun Deng @ 2024-02-22 16:22 ` Liam R. Howlett 2024-02-23 2:36 ` Yajun Deng 0 siblings, 1 reply; 5+ messages in thread From: Liam R. Howlett @ 2024-02-22 16:22 UTC (permalink / raw) To: Yajun Deng; +Cc: akpm, linux-mm, linux-kernel, vbabka, lstoakes * Yajun Deng <yajun.deng@linux.dev> [240222 05:26]: > Adding Vlastimil and Lorenzo to discuss this patch. > > > On 2024/1/29 23:04, Liam R. Howlett wrote: > > * Yajun Deng <yajun.deng@linux.dev> [240129 02:53]: > > > There are vma_merge() and do_brk_flags() pass mm to vma_complete(), others > > > would pass the vma->vm_mm. The following explains that the mm is the > > > vma->vm_mm in vma_merge() and do_brk_flags(). > > > > > > All vma will point to the same mm struct if the vma_merge() is successful. > > > So the mm and the vma->mm are the same. > > Absolutely, they must be the same. I don't think vma_merge() checks > > this, but it is true. > > > > > vm_brk_flags() and brk syscall will initialize vmi with current->mm, > > > so the vma->vm_mm and the current->mm are the same if vma exists in > > > do_brk_flags(). > > > > > > Remove the mm parameter in vma_complete() and get mm from the vma in vp. > > You have added a dereference to the two paths that don't need it to > > reduce the argument list from 3 to 2. It's the same number of lines as > > well. vma_shrink() is only used on process creation, but brk is more > > common. Note that this function is marked as inline. > > > > I'm not sure this change is worth making. > > If we can make sure the mm is vma->vm_mm, I don't think we need to pass the > mm. > > If we can't make sure that, this change is not worth it. We can be quite confident the mm struct is the same. The point is that you are causing more instructions for zero gain. There isn't a lot of arguments and this is marked inline. For most of the cases, we are already causing 1/2 the dereferences you are moving - except brk_flags(), which already has the pointer available. But instead of using the pointer already in a register, you are adding two new dereferences inside an inline function. This is like writing: struct mm_struct *mm = current->mm; struct vm_area_stuuct *vma = find_vma(mm, 0); ... use_the_mm(vma->vm_mm); .. only it's worse than that because the compiler will replace use_the_mm() with the actual code in use_the_mm(), so we have effectively told the compiler to set another register up by dereferencing twice instead of using the value already available. It's a change for the sake of changing. You are not reducing the code size, you are not increasing the readability. You are adding two dereferences to brk() and one to all other callers. Why do this change? > > > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > > > --- > > > mm/mmap.c | 16 ++++++++-------- > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > > index e97b9144c61a..9b968d1edf55 100644 > > > --- a/mm/mmap.c > > > +++ b/mm/mmap.c > > > @@ -509,11 +509,11 @@ static inline void vma_prepare(struct vma_prepare *vp) > > > * > > > * @vp: The vma_prepare struct > > > * @vmi: The vma iterator > > > - * @mm: The mm_struct > > > */ > > > -static inline void vma_complete(struct vma_prepare *vp, > > > - struct vma_iterator *vmi, struct mm_struct *mm) > > > +static inline void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi) > > > { > > > + struct mm_struct *mm = vp->vma->vm_mm; > > > + > > > if (vp->file) { > > > if (vp->adj_next) > > > vma_interval_tree_insert(vp->adj_next, > > > @@ -666,7 +666,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma, > > > vma_set_range(vma, start, end, pgoff); > > > vma_iter_store(vmi, vma); > > > - vma_complete(&vp, vmi, vma->vm_mm); > > > + vma_complete(&vp, vmi); > > > return 0; > > > nomem: > > > @@ -707,7 +707,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, > > > vma_iter_clear(vmi); > > > vma_set_range(vma, start, end, pgoff); > > > - vma_complete(&vp, vmi, vma->vm_mm); > > > + vma_complete(&vp, vmi); > > > return 0; > > > } > > > @@ -1030,7 +1030,7 @@ static struct vm_area_struct > > > } > > > } > > > - vma_complete(&vp, vmi, mm); > > > + vma_complete(&vp, vmi); > > > khugepaged_enter_vma(res, vm_flags); > > > return res; > > > @@ -2377,7 +2377,7 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > > > } > > > /* vma_complete stores the new vma */ > > > - vma_complete(&vp, vmi, vma->vm_mm); > > > + vma_complete(&vp, vmi); > > > /* Success. */ > > > if (new_below) > > > @@ -3145,7 +3145,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma, > > > vm_flags_set(vma, VM_SOFTDIRTY); > > > vma_iter_store(vmi, vma); > > > - vma_complete(&vp, vmi, mm); > > > + vma_complete(&vp, vmi); > > > khugepaged_enter_vma(vma, flags); > > > goto out; > > > } > > > -- > > > 2.25.1 > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: remove the mm parameter in vma_complete() 2024-02-22 16:22 ` Liam R. Howlett @ 2024-02-23 2:36 ` Yajun Deng 0 siblings, 0 replies; 5+ messages in thread From: Yajun Deng @ 2024-02-23 2:36 UTC (permalink / raw) To: Liam R. Howlett, akpm, linux-mm, linux-kernel, vbabka, lstoakes On 2024/2/23 00:22, Liam R. Howlett wrote: > * Yajun Deng <yajun.deng@linux.dev> [240222 05:26]: >> Adding Vlastimil and Lorenzo to discuss this patch. >> >> >> On 2024/1/29 23:04, Liam R. Howlett wrote: >>> * Yajun Deng <yajun.deng@linux.dev> [240129 02:53]: >>>> There are vma_merge() and do_brk_flags() pass mm to vma_complete(), others >>>> would pass the vma->vm_mm. The following explains that the mm is the >>>> vma->vm_mm in vma_merge() and do_brk_flags(). >>>> >>>> All vma will point to the same mm struct if the vma_merge() is successful. >>>> So the mm and the vma->mm are the same. >>> Absolutely, they must be the same. I don't think vma_merge() checks >>> this, but it is true. >>> >>>> vm_brk_flags() and brk syscall will initialize vmi with current->mm, >>>> so the vma->vm_mm and the current->mm are the same if vma exists in >>>> do_brk_flags(). >>>> >>>> Remove the mm parameter in vma_complete() and get mm from the vma in vp. >>> You have added a dereference to the two paths that don't need it to >>> reduce the argument list from 3 to 2. It's the same number of lines as >>> well. vma_shrink() is only used on process creation, but brk is more >>> common. Note that this function is marked as inline. >>> >>> I'm not sure this change is worth making. >> If we can make sure the mm is vma->vm_mm, I don't think we need to pass the >> mm. >> >> If we can't make sure that, this change is not worth it. > We can be quite confident the mm struct is the same. The point is that > you are causing more instructions for zero gain. There isn't a lot of > arguments and this is marked inline. For most of the cases, we are > already causing 1/2 the dereferences you are moving - except > brk_flags(), which already has the pointer available. But instead of > using the pointer already in a register, you are adding two new > dereferences inside an inline function. > > This is like writing: > > struct mm_struct *mm = current->mm; > struct vm_area_stuuct *vma = find_vma(mm, 0); > > ... > > use_the_mm(vma->vm_mm); > > .. only it's worse than that because the compiler will replace > use_the_mm() with the actual code in use_the_mm(), so we have > effectively told the compiler to set another register up by > dereferencing twice instead of using the value already available. > > It's a change for the sake of changing. > > You are not reducing the code size, you are not increasing the > readability. You are adding two dereferences to brk() and one to all > other callers. Why do this change? Thank you for your explanation. >>>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >>>> --- >>>> mm/mmap.c | 16 ++++++++-------- >>>> 1 file changed, 8 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/mm/mmap.c b/mm/mmap.c >>>> index e97b9144c61a..9b968d1edf55 100644 >>>> --- a/mm/mmap.c >>>> +++ b/mm/mmap.c >>>> @@ -509,11 +509,11 @@ static inline void vma_prepare(struct vma_prepare *vp) >>>> * >>>> * @vp: The vma_prepare struct >>>> * @vmi: The vma iterator >>>> - * @mm: The mm_struct >>>> */ >>>> -static inline void vma_complete(struct vma_prepare *vp, >>>> - struct vma_iterator *vmi, struct mm_struct *mm) >>>> +static inline void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi) >>>> { >>>> + struct mm_struct *mm = vp->vma->vm_mm; >>>> + >>>> if (vp->file) { >>>> if (vp->adj_next) >>>> vma_interval_tree_insert(vp->adj_next, >>>> @@ -666,7 +666,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma, >>>> vma_set_range(vma, start, end, pgoff); >>>> vma_iter_store(vmi, vma); >>>> - vma_complete(&vp, vmi, vma->vm_mm); >>>> + vma_complete(&vp, vmi); >>>> return 0; >>>> nomem: >>>> @@ -707,7 +707,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, >>>> vma_iter_clear(vmi); >>>> vma_set_range(vma, start, end, pgoff); >>>> - vma_complete(&vp, vmi, vma->vm_mm); >>>> + vma_complete(&vp, vmi); >>>> return 0; >>>> } >>>> @@ -1030,7 +1030,7 @@ static struct vm_area_struct >>>> } >>>> } >>>> - vma_complete(&vp, vmi, mm); >>>> + vma_complete(&vp, vmi); >>>> khugepaged_enter_vma(res, vm_flags); >>>> return res; >>>> @@ -2377,7 +2377,7 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, >>>> } >>>> /* vma_complete stores the new vma */ >>>> - vma_complete(&vp, vmi, vma->vm_mm); >>>> + vma_complete(&vp, vmi); >>>> /* Success. */ >>>> if (new_below) >>>> @@ -3145,7 +3145,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma, >>>> vm_flags_set(vma, VM_SOFTDIRTY); >>>> vma_iter_store(vmi, vma); >>>> - vma_complete(&vp, vmi, mm); >>>> + vma_complete(&vp, vmi); >>>> khugepaged_enter_vma(vma, flags); >>>> goto out; >>>> } >>>> -- >>>> 2.25.1 >>>> >>>> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-23 2:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-29 7:53 [PATCH] mm/mmap: remove the mm parameter in vma_complete() Yajun Deng 2024-01-29 15:04 ` Liam R. Howlett 2024-02-22 10:26 ` Yajun Deng 2024-02-22 16:22 ` Liam R. Howlett 2024-02-23 2:36 ` Yajun Deng
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).