[PATCH] remove unused line for mmap_region()

[PATCH] remove unused line for mmap_region()

[Huang Shijie]

	The variable pgoff is not used in the following codes.
	So, just remove the line.

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

diff --git a/mm/mmap.c b/mm/mmap.c
index 34579b2..1dd6aaa 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1210,7 +1210,6 @@ munmap_back:
 	 *         f_op->mmap method. -DaveM
 	 */
 	addr = vma->vm_start;
-	pgoff = vma->vm_pgoff;
 	vm_flags = vma->vm_flags;
 
 	if (vma_wants_writenotify(vma))
-- 
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] remove unused line for mmap_region()

[Hugh Dickins]

On Sun, 21 Jun 2009, Huang Shijie wrote:

> 	The variable pgoff is not used in the following codes.
> 	So, just remove the line.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>

Hmm, hmm, well, I suppose a grudging Ack, though I'd happily be overruled.

Of course you are right, so why am I so reluctant to acknowledge it?

Because it's exceptional for addr and pgoff to be different after
coming back from the file's ->mmap method, and if someone adds a
use for pgoff lower down (there was, of course, a use for pgoff
in vma_merge lower down, until Linus moved that higher in 2.6.29),
it's a fair bet that they'll forget to restore the update you're
removing, and a fair bet that nobody will notice that it's gone
wrong for a while.

However, what would be likely to need pgoff lower down, other than
another attempt at vma_merge?  And given how we're now working with
the vma_merge higher up (before pgoff had a chance to be adjusted),
I think we can conclude that anything needing to meddle with pgoff
would be setting some vm_flags that prevent merging anyway.

So I can just about bring myself to Ack your patch, but...

Hugh

> ---
>  mm/mmap.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 34579b2..1dd6aaa 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1210,7 +1210,6 @@ munmap_back:
>  	 *         f_op->mmap method. -DaveM
>  	 */
>  	addr = vma->vm_start;
> -	pgoff = vma->vm_pgoff;
>  	vm_flags = vma->vm_flags;
>  
>  	if (vma_wants_writenotify(vma))
> -- 
> 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] remove unused line for mmap_region()

[Huang Shijie]

> On Sun, 21 Jun 2009, Huang Shijie wrote:
>
>   
>> 	The variable pgoff is not used in the following codes.
>> 	So, just remove the line.
>>
>> Signed-off-by: Huang Shijie <shijie8@gmail.com>
>>     
>
> Hmm, hmm, well, I suppose a grudging Ack, though I'd happily be overruled.
>
> Of course you are right, so why am I so reluctant to acknowledge it?
>
> Because it's exceptional for addr and pgoff to be different after
> coming back from the file's ->mmap method, and if someone adds a
> use for pgoff lower down (there was, of course, a use for pgoff
> in vma_merge lower down, until Linus moved that higher in 2.6.29),
> it's a fair bet that they'll forget to restore the update you're
> removing, and a fair bet that nobody will notice that it's gone
> wrong for a while.
>
>   
I knew the file's ->mmap method can change addr and pgoff, but I think 
the comment of
DavidM is enough to remind the person who wants to use pgoff lower down 
that pgoff maybe
go wrong for a while.

But now, it (this line) looks like a big fly on 'Mona Lisa'. :)
> However, what would be likely to need pgoff lower down, other than
> another attempt at vma_merge?  And given how we're now working with
> the vma_merge higher up (before pgoff had a chance to be adjusted),
> I think we can conclude that anything needing to meddle with pgoff
> would be setting some vm_flags that prevent merging anyway.
>
>   
Could you tell some drivers which will meddle with pgoff?
If a driver changes pgoff,the work done by vma_merge higher up will be 
invalidated.
The process gets a wrong memory map.
> So I can just about bring myself to Ack your patch, but...
>
> Hugh
>
>   
>> ---
>>  mm/mmap.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 34579b2..1dd6aaa 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -1210,7 +1210,6 @@ munmap_back:
>>  	 *         f_op->mmap method. -DaveM
>>  	 */
>>  	addr = vma->vm_start;
>> -	pgoff = vma->vm_pgoff;
>>  	vm_flags = vma->vm_flags;
>>  
>>  	if (vma_wants_writenotify(vma))
>> -- 
>> 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] remove unused line for mmap_region()

[Hugh Dickins]

On Mon, 22 Jun 2009, Huang Shijie wrote:
> >   
> I knew the file's ->mmap method can change addr and pgoff,
> but I think the comment of DavidM is enough to remind the
> person who wants to use pgoff lower down that pgoff maybe
> go wrong for a while.

Indeed it's a very helpful comment from DaveM there.  Unfortunately,
we can't really repeat that comment on every line below where
somebody might in future want to insert a use of pgoff, and not all
of us are as good at actually reading nearby comments as you are!

> 
> But now, it (this line) looks like a big fly on 'Mona Lisa'. :)
> > However, what would be likely to need pgoff lower down, other than
> > another attempt at vma_merge?  And given how we're now working with
> > the vma_merge higher up (before pgoff had a chance to be adjusted),
> > I think we can conclude that anything needing to meddle with pgoff
> > would be setting some vm_flags that prevent merging anyway.
> >
> Could you tell some drivers which will meddle with pgoff?

I can't name a list of drivers offhand, no (but note VM_PFNMAP areas
have a particular use for vm_pgoff, so all those drivers are likely
to be on the list).  May I please leave that investigation to you?

What I expect you to find in the end is that every driver which does
meddle with pgoff in its ->mmap, also has some other characteristic
(e.g. sets VM_IO or VM_DONTEXPAND or VM_RESERVED or VM_PFNMAP, or
even some other flag which the new vm_flags wouldn't have set),
which will prevent its vmas being merged anyway.

> If a driver changes pgoff,the work done by vma_merge higher
> up will be invalidated. The process gets a wrong memory map.

It's probably all okay; but I won't be astonished if you discover
one or two cases which ought to be fixed up, probably by adding
one of those flags.

Thanks,
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] remove unused line for mmap_region()

[Huang Shijie]

 
> I can't name a list of drivers offhand, no (but note VM_PFNMAP areas
> have a particular use for vm_pgoff, so all those drivers are likely
> to be on the list).  May I please leave that investigation to you?
>
>   
ok.
> What I expect you to find in the end is that every driver which does
> meddle with pgoff in its ->mmap, also has some other characteristic
> (e.g. sets VM_IO or VM_DONTEXPAND or VM_RESERVED or VM_PFNMAP, or
> even some other flag which the new vm_flags wouldn't have set),
> which will prevent its vmas being merged anyway.
>
>   
Unfortunately,the driver's -->mmap is called below the vma_merge(),
so even if the driver sets the VM_SPECIAL flag, it does not prevent the
vmas being merged actually.

--
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] remove unused line for mmap_region()

[Hugh Dickins]

On Wed, 24 Jun 2009, Huang Shijie wrote:
> > What I expect you to find in the end is that every driver which does
> > meddle with pgoff in its ->mmap, also has some other characteristic
> > (e.g. sets VM_IO or VM_DONTEXPAND or VM_RESERVED or VM_PFNMAP, or
> > even some other flag which the new vm_flags wouldn't have set),
> > which will prevent its vmas being merged anyway.
> >
> Unfortunately,the driver's -->mmap is called below the vma_merge(),
> so even if the driver sets the VM_SPECIAL flag, it does not prevent the
> vmas being merged actually.

It does not prevent it by virtue of being VM_SPECIAL at that point,
you're right, but I believe it does prevent it by virtue of presenting
a different vm_flags.  Collapsing a definition to make it clearer, see

	if ((vma->vm_flags ^ vm_flags) & ~VM_CAN_NONLINEAR)
		return 0;

at the beginning of is_mergeable_vma().  We have to make an exception
of VM_CAN_NONLINEAR precisely because it gets set by a normal ->mmap,
but cannot be predicted by mmap_region() before its vma_merge().

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>