[PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Chen Gang]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]


>From b12fa5a9263cf4c044988e59f0071f4bcc132215 Mon Sep 17 00:00:00 2001
From: Chen Gang <gang.chen.5i5j@gmail.com>
Date: Sat, 5 Sep 2015 21:49:56 +0800
Subject: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in
 find_vma()

Before the main looping, vma is already is NULL, so need not set it to
NULL, again.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 mm/mmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index df6d5f0..4db7cf0 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2054,7 +2054,6 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 		return vma;
 
 	rb_node = mm->mm_rb.rb_node;
-	vma = NULL;
 
 	while (rb_node) {
 		struct vm_area_struct *tmp;
-- 
1.9.3

 		 	   		  

[-- Attachment #2: 0001-mm-mmap.c-Remove-useless-statement-vma-NULL-in-find_.patch --]
[-- Type: application/octet-stream, Size: 746 bytes --]

From b12fa5a9263cf4c044988e59f0071f4bcc132215 Mon Sep 17 00:00:00 2001
From: Chen Gang <gang.chen.5i5j@gmail.com>
Date: Sat, 5 Sep 2015 21:49:56 +0800
Subject: [PATCH 1/2] mm/mmap.c: Remove useless statement "vma = NULL" in
 find_vma()

Before the main looping, vma is already is NULL, so need not set it to
NULL, again.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 mm/mmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index df6d5f0..4db7cf0 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2054,7 +2054,6 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 		return vma;
 
 	rb_node = mm->mm_rb.rb_node;
-	vma = NULL;
 
 	while (rb_node) {
 		struct vm_area_struct *tmp;
-- 
1.9.3

Re: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Oleg Nesterov]

On 09/05, Chen Gang wrote:
>
> From b12fa5a9263cf4c044988e59f0071f4bcc132215 Mon Sep 17 00:00:00 2001
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> Date: Sat, 5 Sep 2015 21:49:56 +0800
> Subject: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in
>  find_vma()
>
> Before the main looping, vma is already is NULL, so need not set it to
> NULL, again.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  mm/mmap.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index df6d5f0..4db7cf0 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2054,7 +2054,6 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>  		return vma;
>  
>  	rb_node = mm->mm_rb.rb_node;
> -	vma = NULL;
>  
>  	while (rb_node) {
>  		struct vm_area_struct *tmp;
> -- 
> 1.9.3
>
>


--
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] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Chen Gang]

On 9/7/15 20:36, Oleg Nesterov wrote:
> On 09/05, Chen Gang wrote:
>>
>> From b12fa5a9263cf4c044988e59f0071f4bcc132215 Mon Sep 17 00:00:00 2001
>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>> Date: Sat, 5 Sep 2015 21:49:56 +0800
>> Subject: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in
>> find_vma()
>>
>> Before the main looping, vma is already is NULL, so need not set it to
>> NULL, again.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>

OK, thanks.


I also want to consult: the comments of find_vma() says:

"Look up the first VMA which satisfies addr < vm_end, ..."

Is it OK? (why not "vm_start <= addr < vm_end"), need we let "vma = tmp"
in "if (tmp->vm_start <= addr)"? -- it looks the comments is not match
the implementation, precisely (maybe not 1st VMA).


Thanks.


>> ---
>> mm/mmap.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index df6d5f0..4db7cf0 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -2054,7 +2054,6 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>> return vma;
>>
>> rb_node = mm->mm_rb.rb_node;
>> - vma = NULL;
>>
>> while (rb_node) {
>> struct vm_area_struct *tmp;
>> --
>> 1.9.3
>>
>>
>
>

--
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  

Re: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Oleg Nesterov]

On 09/08, Chen Gang wrote:
>
> I also want to consult: the comments of find_vma() says:

Sorry, I don't understand the question ;)

> "Look up the first VMA which satisfies addr < vm_end, ..."
>
> Is it OK?

Why not?

> (why not "vm_start <= addr < vm_end"),

Because this some callers actually want to find the 1st vma which
satisfies addr < vm_end? For example, shift_arg_pages().

OTOH, I think that another helper,

	find_vma_xxx(mm, addr)
	{
		vma = find_vma(...)
		if (vma && vma->vm_start > addr)
			vma = NULL;
		return vma;
	}

makes sense. It can have a lot of users.

> need we let "vma = tmp"
> in "if (tmp->vm_start <= addr)"? -- it looks the comments is not match
> the implementation, precisely (maybe not 1st VMA).

This contradicts with above... I mean, it is not clear what exactly do
you blame, semantics or implementation.

The implementation looks correct. Why do you think it can be not 1st vma?

Oleg.

--
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] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Chen Gang]

On 9/10/15 00:26, Oleg Nesterov wrote:
> On 09/08, Chen Gang wrote:
>>
>> I also want to consult: the comments of find_vma() says:
>
> Sorry, I don't understand the question ;)
>
>> "Look up the first VMA which satisfies addr < vm_end, ..."
>>
>> Is it OK?
>
> Why not?
>

We will continue discuss about it below. Please help check, thanks.

>> (why not "vm_start <= addr < vm_end"),
>
> Because this some callers actually want to find the 1st vma which
> satisfies addr < vm_end? For example, shift_arg_pages().
>
> OTOH, I think that another helper,
>
> find_vma_xxx(mm, addr)
> {
> vma = find_vma(...)
> if (vma && vma->vm_start> addr)
> vma = NULL;
> return vma;
> }
>
> makes sense. It can have a lot of users.
>

OK. thank you very much. :-)

>> need we let "vma = tmp"
>> in "if (tmp->vm_start <= addr)"? -- it looks the comments is not match
>> the implementation, precisely (maybe not 1st VMA).
>
> This contradicts with above... I mean, it is not clear what exactly do
> you blame, semantics or implementation.
>
> The implementation looks correct. Why do you think it can be not 1st vma?
>

It is in while (rb_node) {...}.

- When we set "vma = tmp", it is alreay match "addr < vm_end".

- If "addr>= vm_start", we return this vma (else continue searching).

If "the first left" is the real first, when "addr>= vm_start", it
will return (may not return 1st left matched vma).

If "the first find" is the real first, when "addr < vm_start", it
will continue searching (may not return 1st find matched vma).

For me, if we only focus on "addr < vm_end", we need remove "vm_start <=
addr" checking). If we have to consider about "addr>= vm_start", we may
need additional parameter or implement a new function for it.


Welcome any ideas, suggestions and completions.


Thanks.
--
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  

Re: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Oleg Nesterov]

On 09/10, Chen Gang wrote:
>
> On 9/10/15 00:26, Oleg Nesterov wrote:
> >
> > The implementation looks correct. Why do you think it can be not 1st vma?
> >
>
> It is in while (rb_node) {...}.
>
> - When we set "vma = tmp", it is alreay match "addr < vm_end".

Yes,

> - If "addr>= vm_start", we return this vma (else continue searching).

This is optimization, we can stop the search because in this case
vma == tmp is obviously the 1st vma with "addr < vm_end".

I simply can't understand your concerns. Perhaps you can make a
patch, then it will be more clear what me-or-you have missed.

Oleg.

--
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] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Chen Gang]

On 9/11/15 02:19, Oleg Nesterov wrote:
> On 09/10, Chen Gang wrote:
>> - If "addr>= vm_start", we return this vma (else continue searching).
> 
> This is optimization, we can stop the search because in this case
> vma == tmp is obviously the 1st vma with "addr < vm_end".
> 

OK, thanks.

I guess if we have additional comments for "if (tmp->vm_start <= addr)",
the code will be more readable for readers (especially for newbies).

@@ -2064,7 +2064,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
                if (tmp->vm_end > addr) {
                        vma = tmp;
                        if (tmp->vm_start <= addr)
-                               break;
+                               break; /* It must be 1st "addr < vm_end" */
                        rb_node = rb_node->rb_left;
                } else
                        rb_node = rb_node->rb_right;


> I simply can't understand your concerns. Perhaps you can make a
> patch, then it will be more clear what me-or-you have missed.
> 

I guess, we need not (it is my missing). :-)


Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

--
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] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 478 bytes --]

On Sat, 5 Sep 2015, Chen Gang wrote:

> 
> From b12fa5a9263cf4c044988e59f0071f4bcc132215 Mon Sep 17 00:00:00 2001
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> Date: Sat, 5 Sep 2015 21:49:56 +0800
> Subject: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in
>  find_vma()
> 
> Before the main looping, vma is already is NULL, so need not set it to
> NULL, again.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Acked-by: David Rientjes <rientjes@google.com>

[PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[gang.chen.5i5j]

From: Chen Gang <gang.chen.5i5j@gmail.com>

Before the main looping, vma is already is NULL, so need not set it to
NULL, again.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 mm/mmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index df6d5f0..4db7cf0 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2054,7 +2054,6 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 		return vma;
 
 	rb_node = mm->mm_rb.rb_node;
-	vma = NULL;
 
 	while (rb_node) {
 		struct vm_area_struct *tmp;
-- 
1.9.3

Re: [PATCH] mm/mmap.c: Remove useless statement "vma = NULL" in find_vma()

[Chen Gang]

Hello all:

I also want to consult: the comments of find_vma() says:

  "Look up the first VMA which satisfies  addr < vm_end, ..."

Is it OK? (why not "vm_start <= addr < vm_end"), need we let "vma = tmp"
in "if (tmp->vm_start <= addr)"? -- it looks the comments is not match
the implementation, precisely (maybe not 1st VMA).


Thanks.


On 9/3/15 11:52, gang.chen.5i5j@gmail.com wrote:
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> Before the main looping, vma is already is NULL, so need not set it to
> NULL, again.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  mm/mmap.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index df6d5f0..4db7cf0 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2054,7 +2054,6 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>  		return vma;
>  
>  	rb_node = mm->mm_rb.rb_node;
> -	vma = NULL;
>  
>  	while (rb_node) {
>  		struct vm_area_struct *tmp;
> 

-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed