[PATCH] mmap : save some cycles for the shared anonymous mapping

[PATCH] mmap : save some cycles for the shared anonymous mapping

[Huang Shijie]

The shmem_zere_setup() does not change vm_start, pgoff or vm_flags,
only some drivers change them (such as /driver/video/bfin-t350mcqb-fb.c).

Moving these codes to a more proper place to save cycles for shared anonymous mapping.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 mm/mmap.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 8101de4..840e91e 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1195,21 +1195,21 @@ munmap_back:
 			goto unmap_and_free_vma;
 		if (vm_flags & VM_EXECUTABLE)
 			added_exe_file_vma(mm);
+
+		/* Can addr have changed??
+		 *
+		 * Answer: Yes, several device drivers can do it in their
+		 *         f_op->mmap method. -DaveM
+		 */
+		addr = vma->vm_start;
+		pgoff = vma->vm_pgoff;
+		vm_flags = vma->vm_flags;
 	} else if (vm_flags & VM_SHARED) {
 		error = shmem_zero_setup(vma);
 		if (error)
 			goto free_vma;
 	}
 
-	/* Can addr have changed??
-	 *
-	 * Answer: Yes, several device drivers can do it in their
-	 *         f_op->mmap method. -DaveM
-	 */
-	addr = vma->vm_start;
-	pgoff = vma->vm_pgoff;
-	vm_flags = vma->vm_flags;
-
 	if (vma_wants_writenotify(vma))
 		vma->vm_page_prot = vm_get_page_prot(vm_flags & ~VM_SHARED);
 
-- 
1.6.0.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mmap : save some cycles for the shared anonymous mapping

[Minchan Kim]

On Fri, Sep 11, 2009 at 10:52 AM, Huang Shijie <shijie8@gmail.com> wrote:
> The shmem_zere_setup() does not change vm_start, pgoff or vm_flags,
> only some drivers change them (such as /driver/video/bfin-t350mcqb-fb.c).
>
> Moving these codes to a more proper place to save cycles for shared anonymous mapping.
>
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
Reviewed-by: Minchan Kim<minchan.kim@gmail.com>

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mmap : save some cycles for the shared anonymous mapping

[Andrew Morton]

On Fri, 11 Sep 2009 09:52:46 +0800
Huang Shijie <shijie8@gmail.com> wrote:

> The shmem_zere_setup() does not change vm_start, pgoff or vm_flags,
> only some drivers change them (such as /driver/video/bfin-t350mcqb-fb.c).
> 
> Moving these codes to a more proper place to save cycles for shared anonymous mapping.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
> ---
>  mm/mmap.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 8101de4..840e91e 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1195,21 +1195,21 @@ munmap_back:
>  			goto unmap_and_free_vma;
>  		if (vm_flags & VM_EXECUTABLE)
>  			added_exe_file_vma(mm);
> +
> +		/* Can addr have changed??
> +		 *
> +		 * Answer: Yes, several device drivers can do it in their
> +		 *         f_op->mmap method. -DaveM
> +		 */
> +		addr = vma->vm_start;
> +		pgoff = vma->vm_pgoff;
> +		vm_flags = vma->vm_flags;
>  	} else if (vm_flags & VM_SHARED) {
>  		error = shmem_zero_setup(vma);
>  		if (error)
>  			goto free_vma;
>  	}
>  
> -	/* Can addr have changed??
> -	 *
> -	 * Answer: Yes, several device drivers can do it in their
> -	 *         f_op->mmap method. -DaveM
> -	 */
> -	addr = vma->vm_start;
> -	pgoff = vma->vm_pgoff;
> -	vm_flags = vma->vm_flags;
> -
>  	if (vma_wants_writenotify(vma))
>  		vma->vm_page_prot = vm_get_page_prot(vm_flags & ~VM_SHARED);
>  

hm, maybe we should nuke those locals and just use vma->foo everywhere.

Local variable pgoff never gets used again anyway.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mmap : save some cycles for the shared anonymous mapping

[Hugh Dickins]

On Fri, 11 Sep 2009, Andrew Morton wrote:
> On Fri, 11 Sep 2009 09:52:46 +0800
> Huang Shijie <shijie8@gmail.com> wrote:
> 
> > The shmem_zere_setup() does not change vm_start, pgoff or vm_flags,
> > only some drivers change them (such as /driver/video/bfin-t350mcqb-fb.c).
> > 
> > Moving these codes to a more proper place to save cycles for shared
> > anonymous mapping.

(Actually it's saving them for any !file mapping.
 Though I doubt it's a significant saving myself.)

> > 
> > Signed-off-by: Huang Shijie <shijie8@gmail.com>

Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>

> > ---
> >  mm/mmap.c |   18 +++++++++---------
> >  1 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 8101de4..840e91e 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1195,21 +1195,21 @@ munmap_back:
> >  			goto unmap_and_free_vma;
> >  		if (vm_flags & VM_EXECUTABLE)
> >  			added_exe_file_vma(mm);
> > +
> > +		/* Can addr have changed??
> > +		 *
> > +		 * Answer: Yes, several device drivers can do it in their
> > +		 *         f_op->mmap method. -DaveM
> > +		 */
> > +		addr = vma->vm_start;
> > +		pgoff = vma->vm_pgoff;
> > +		vm_flags = vma->vm_flags;
> >  	} else if (vm_flags & VM_SHARED) {
> >  		error = shmem_zero_setup(vma);
> >  		if (error)
> >  			goto free_vma;
> >  	}
> >  
> > -	/* Can addr have changed??
> > -	 *
> > -	 * Answer: Yes, several device drivers can do it in their
> > -	 *         f_op->mmap method. -DaveM
> > -	 */
> > -	addr = vma->vm_start;
> > -	pgoff = vma->vm_pgoff;
> > -	vm_flags = vma->vm_flags;
> > -
> >  	if (vma_wants_writenotify(vma))
> >  		vma->vm_page_prot = vm_get_page_prot(vm_flags & ~VM_SHARED);
> >  
> 
> hm, maybe we should nuke those locals and just use vma->foo everywhere.
> 
> Local variable pgoff never gets used again anyway.

I think it was me who Nak'ed an earlier patch to remove the update
of pgoff, out of fear that we might add a later reference sometime
in future, and not notice for a long time that it then needed that
update again.

addr and pgoff start off as args to do_mmap_pgoff(), so we'd better
not nuke them!  And if we changed all the lines below that point to
refer to vma->vm_start and vma->vm_flags, I think there's still a
danger we'd unthinkingly add a reference to addr or vm_flags later.

If any change is to be made here, I think I prefer Shijie's:
shmem_zero_setup isn't likely to change to modify any of those,
and that patch has the great virtue of retaining DaveM's comment,
which draws attention to the issue.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mmap : save some cycles for the shared anonymous mapping

[Huang Shijie]

Hugh Dickins write:
> On Fri, 11 Sep 2009, Andrew Morton wrote:
>   
>> On Fri, 11 Sep 2009 09:52:46 +0800
>> Huang Shijie <shijie8@gmail.com> wrote:
>>
>>     
>>> The shmem_zere_setup() does not change vm_start, pgoff or vm_flags,
>>> only some drivers change them (such as /driver/video/bfin-t350mcqb-fb.c).
>>>
>>> Moving these codes to a more proper place to save cycles for shared
>>> anonymous mapping.
>>>       
>
> (Actually it's saving them for any !file mapping.
>  Though I doubt it's a significant saving myself.)
>
>   
yes.
>>> Signed-off-by: Huang Shijie <shijie8@gmail.com>
>>>       
>
> Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
>
>   
>>> ---
>>>  mm/mmap.c |   18 +++++++++---------
>>>  1 files changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>> index 8101de4..840e91e 100644
>>> --- a/mm/mmap.c
>>> +++ b/mm/mmap.c
>>> @@ -1195,21 +1195,21 @@ munmap_back:
>>>  			goto unmap_and_free_vma;
>>>  		if (vm_flags & VM_EXECUTABLE)
>>>  			added_exe_file_vma(mm);
>>> +
>>> +		/* Can addr have changed??
>>> +		 *
>>> +		 * Answer: Yes, several device drivers can do it in their
>>> +		 *         f_op->mmap method. -DaveM
>>> +		 */
>>> +		addr = vma->vm_start;
>>> +		pgoff = vma->vm_pgoff;
>>> +		vm_flags = vma->vm_flags;
>>>  	} else if (vm_flags & VM_SHARED) {
>>>  		error = shmem_zero_setup(vma);
>>>  		if (error)
>>>  			goto free_vma;
>>>  	}
>>>  
>>> -	/* Can addr have changed??
>>> -	 *
>>> -	 * Answer: Yes, several device drivers can do it in their
>>> -	 *         f_op->mmap method. -DaveM
>>> -	 */
>>> -	addr = vma->vm_start;
>>> -	pgoff = vma->vm_pgoff;
>>> -	vm_flags = vma->vm_flags;
>>> -
>>>  	if (vma_wants_writenotify(vma))
>>>  		vma->vm_page_prot = vm_get_page_prot(vm_flags & ~VM_SHARED);
>>>  
>>>       
>> hm, maybe we should nuke those locals and just use vma->foo everywhere.
>>
>> Local variable pgoff never gets used again anyway.
>>     
>
> I think it was me who Nak'ed an earlier patch to remove the update
> of pgoff, out of fear that we might add a later reference sometime
> in future, and not notice for a long time that it then needed that
> update again.
>
>   
my patch.
> addr and pgoff start off as args to do_mmap_pgoff(), so we'd better
> not nuke them!  And if we changed all the lines below that point to
> refer to vma->vm_start and vma->vm_flags, I think there's still a
> danger we'd unthinkingly add a reference to addr or vm_flags later.
>
>   
If we nuke them, there is potential problem to notice :
    Some drivers change the vm->vm_end, so (vm->vm_end - vm->vm_start) 
changes against the length of MMAP.

> If any change is to be made here, I think I prefer Shijie's:
> shmem_zero_setup isn't likely to change to modify any of those,
> and that patch has the great virtue of retaining DaveM's comment,
> which draws attention to the issue.
>
> Hugh
>
>   

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>