[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e3b1dddf-57e0-f1fb-66fc-cf08c94c5dc8@linux.dev>
Date: Fri, 23 Feb 2024 10:36:09 +0800
From: Yajun Deng <yajun.deng@...ux.dev>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>, akpm@...ux-foundation.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org, vbabka@...e.cz,
lstoakes@...il.com
Subject: Re: [PATCH] mm/mmap: remove the mm parameter in vma_complete()
On 2024/2/23 00:22, Liam R. Howlett wrote:
> * Yajun Deng <yajun.deng@...ux.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@...ux.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@...ux.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
>>>>
>>>>
Powered by blists - more mailing lists