[PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma

[PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma

[linmiaohe]

From: Miaohe Lin <linmiaohe@huawei.com>

The odd jump label try_prev and none is not really need
in func find_mergeable_anon_vma, eliminate them to
improve readability.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/mmap.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 4d4db76a07da..ab980d468a10 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1276,25 +1276,21 @@ static struct anon_vma *reusable_anon_vma(struct vm_area_struct *old, struct vm_
  */
 struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *vma)
 {
-	struct anon_vma *anon_vma;
+	struct anon_vma *anon_vma = NULL;
 	struct vm_area_struct *near;
 
 	near = vma->vm_next;
-	if (!near)
-		goto try_prev;
-
-	anon_vma = reusable_anon_vma(near, vma, near);
+	if (near)
+		anon_vma = reusable_anon_vma(near, vma, near);
 	if (anon_vma)
 		return anon_vma;
-try_prev:
-	near = vma->vm_prev;
-	if (!near)
-		goto none;
 
-	anon_vma = reusable_anon_vma(near, near, vma);
+	near = vma->vm_prev;
+	if (near)
+		anon_vma = reusable_anon_vma(near, near, vma);
 	if (anon_vma)
 		return anon_vma;
-none:
+
 	/*
 	 * There's no absolute need to look only at touching neighbours:
 	 * we could search further afield for "compatible" anon_vmas.
-- 
2.19.1

Re: [PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma

[Qian Cai]

> On Nov 15, 2019, at 1:36 AM, linmiaohe <linmiaohe@huawei.com> wrote:
> 
> The odd jump label try_prev and none is not really need
> in func find_mergeable_anon_vma, eliminate them to
> improve readability.

Warning: subtle code don’t touch especially with little gains like this.

Re: [PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma

[David Hildenbrand]

On 15.11.19 07:36, linmiaohe wrote:
> From: Miaohe Lin <linmiaohe@huawei.com>

I'm pro removing unnecessary jump labels.

Subject: "mm: get rid of jump labels in find_mergeable_anon_vma()"

> 
> The odd jump label try_prev and none is not really need

s/odd jump label/jump labels/

s/is/are/

> in func find_mergeable_anon_vma, eliminate them to
> improve readability.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/mmap.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 4d4db76a07da..ab980d468a10 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1276,25 +1276,21 @@ static struct anon_vma *reusable_anon_vma(struct vm_area_struct *old, struct vm_
>   */
>  struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *vma)
>  {
> -	struct anon_vma *anon_vma;
> +	struct anon_vma *anon_vma = NULL;
>  	struct vm_area_struct *near;
>  
>  	near = vma->vm_next;
> -	if (!near)
> -		goto try_prev;
> -
> -	anon_vma = reusable_anon_vma(near, vma, near);
> +	if (near)
> +		anon_vma = reusable_anon_vma(near, vma, near);>  	if (anon_vma)
>  		return anon_vma;

Let me suggest the following instead:

/* Try next first */
near = vma->vm_next;
if (near) {
	anon_vma = reusable_anon_vma(near, vma, near);
	if (anon_vma)
		return anon_vma;
}
/* Try prev next */
near = vma->vm_prev;
if (near) {
	anon_vma = reusable_anon_vma(near, vma, near);
	if (anon_vma)
		return anon_vma;
}

> -try_prev:
> -	near = vma->vm_prev;
> -	if (!near)
> -		goto none;
>  
> -	anon_vma = reusable_anon_vma(near, near, vma);
> +	near = vma->vm_prev;
> +	if (near)
> +		anon_vma = reusable_anon_vma(near, near, vma);
>  	if (anon_vma)
>  		return anon_vma;
> -none:
> +
>  	/*
>  	 * There's no absolute need to look only at touching neighbours:
>  	 * we could search further afield for "compatible" anon_vmas.
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma

[John Hubbard]

On 11/15/19 4:58 AM, David Hildenbrand wrote:
> On 15.11.19 07:36, linmiaohe wrote:
>> From: Miaohe Lin <linmiaohe@huawei.com>
> 
> I'm pro removing unnecessary jump labels.

Thank you, simpler code is good--all other things being equal. 
The  tradeoff is, as Qian points out: code churn and risk in critical 
code paths.

In this case, I'd claim it's OK to improve this one, because we
can likely get it right by visual inspection, and the pre-existing
code is quite poor. And being in the kernel does not necessarily give
weak code a free pass to remain there and incur maintenance and annoyance
costs until the end of time. :)

However, please spend equal time when you write your commit descriptions,
because that's also very important. Commit logs should also be clear!

> 
> Subject: "mm: get rid of jump labels in find_mergeable_anon_vma()"
> 
>>
>> The odd jump label try_prev and none is not really need

s/need/needed/

> 
> s/odd jump label/jump labels/
> 
> s/is/are/
> 
>> in func find_mergeable_anon_vma, eliminate them to
>> improve readability.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/mmap.c | 18 +++++++-----------
>>  1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 4d4db76a07da..ab980d468a10 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -1276,25 +1276,21 @@ static struct anon_vma *reusable_anon_vma(struct vm_area_struct *old, struct vm_
>>   */
>>  struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *vma)
>>  {
>> -	struct anon_vma *anon_vma;
>> +	struct anon_vma *anon_vma = NULL;
>>  	struct vm_area_struct *near;
>>  
>>  	near = vma->vm_next;
>> -	if (!near)
>> -		goto try_prev;
>> -
>> -	anon_vma = reusable_anon_vma(near, vma, near);
>> +	if (near)
>> +		anon_vma = reusable_anon_vma(near, vma, near);>  	if (anon_vma)
>>  		return anon_vma;
> 
> Let me suggest the following instead:
> 
> /* Try next first */
> near = vma->vm_next;
> if (near) {
> 	anon_vma = reusable_anon_vma(near, vma, near);
> 	if (anon_vma)
> 		return anon_vma;
> }
> /* Try prev next */
> near = vma->vm_prev;
> if (near) {
> 	anon_vma = reusable_anon_vma(near, vma, near);
> 	if (anon_vma)
> 		return anon_vma;
> }
> 

Actually, it can be further simplified, because you don't need
that last "if" statement, because you're returning NULL after this.

So just return anon_vma there. (And adjust the comment block at the 
end, so that it's clear that anon_vma might be null.)


thanks,
-- 
John Hubbard
NVIDIA

>> -try_prev:
>> -	near = vma->vm_prev;
>> -	if (!near)
>> -		goto none;
>>  
>> -	anon_vma = reusable_anon_vma(near, near, vma);
>> +	near = vma->vm_prev;
>> +	if (near)
>> +		anon_vma = reusable_anon_vma(near, near, vma);
>>  	if (anon_vma)
>>  		return anon_vma;
>> -none:
>> +
>>  	/*
>>  	 * There's no absolute need to look only at touching neighbours:
>>  	 * we could search further afield for "compatible" anon_vmas.
>>
> 
> 

Re: [PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma

[Wei Yang]

On Fri, Nov 15, 2019 at 01:58:02PM +0100, David Hildenbrand wrote:
>On 15.11.19 07:36, linmiaohe wrote:
>> From: Miaohe Lin <linmiaohe@huawei.com>
>
>I'm pro removing unnecessary jump labels.
>
>Subject: "mm: get rid of jump labels in find_mergeable_anon_vma()"
>
>> 
>> The odd jump label try_prev and none is not really need
>
>s/odd jump label/jump labels/
>
>s/is/are/
>
>> in func find_mergeable_anon_vma, eliminate them to
>> improve readability.
>> 
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/mmap.c | 18 +++++++-----------
>>  1 file changed, 7 insertions(+), 11 deletions(-)
>> 
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 4d4db76a07da..ab980d468a10 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -1276,25 +1276,21 @@ static struct anon_vma *reusable_anon_vma(struct vm_area_struct *old, struct vm_
>>   */
>>  struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *vma)
>>  {
>> -	struct anon_vma *anon_vma;
>> +	struct anon_vma *anon_vma = NULL;
>>  	struct vm_area_struct *near;
>>  
>>  	near = vma->vm_next;
>> -	if (!near)
>> -		goto try_prev;
>> -
>> -	anon_vma = reusable_anon_vma(near, vma, near);
>> +	if (near)
>> +		anon_vma = reusable_anon_vma(near, vma, near);>  	if (anon_vma)
>>  		return anon_vma;
>
>Let me suggest the following instead:
>
>/* Try next first */
>near = vma->vm_next;
>if (near) {
>	anon_vma = reusable_anon_vma(near, vma, near);
>	if (anon_vma)
>		return anon_vma;
>}
>/* Try prev next */
>near = vma->vm_prev;
>if (near) {
>	anon_vma = reusable_anon_vma(near, vma, near);
>	if (anon_vma)
>		return anon_vma;
>}
>

Can we have an array with vma->vm_next and vma->vm_prev, then iterate on the
array?

>> -try_prev:
>> -	near = vma->vm_prev;
>> -	if (!near)
>> -		goto none;
>>  
>> -	anon_vma = reusable_anon_vma(near, near, vma);
>> +	near = vma->vm_prev;
>> +	if (near)
>> +		anon_vma = reusable_anon_vma(near, near, vma);
>>  	if (anon_vma)
>>  		return anon_vma;
>> -none:
>> +
>>  	/*
>>  	 * There's no absolute need to look only at touching neighbours:
>>  	 * we could search further afield for "compatible" anon_vmas.
>> 
>
>
>-- 
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: get rid of odd jump label in find_mergeable_anon_vma

[linmiaohe]

> On 11/15/19 4:58 AM, David Hildenbrand wrote:
>> On 15.11.19 07:36, linmiaohe wrote:
>>> From: Miaohe Lin <linmiaohe@huawei.com>
>> 
>> I'm pro removing unnecessary jump labels.
>
>Thank you, simpler code is good--all other things being equal. 
>The  tradeoff is, as Qian points out: code churn and risk in critical code paths.
>
>In this case, I'd claim it's OK to improve this one, because we can likely get it right by visual inspection, and the pre-existing code is quite poor. And being in the kernel does not necessarily give weak code a free pass to remain there and incur maintenance and annoyance costs until the end of time. :)
>
>However, please spend equal time when you write your commit descriptions, because that's also very important. Commit logs should also be clear!
>
>> 
>> Subject: "mm: get rid of jump labels in find_mergeable_anon_vma()"
>> 
>>>
>>> The odd jump label try_prev and none is not really need
>
>s/need/needed/
>
>> 
>> s/odd jump label/jump labels/
>> 
>> s/is/are/
>> 
>>> in func find_mergeable_anon_vma, eliminate them to improve 
>>> readability.
>>>
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>> ---
>>>  mm/mmap.c | 18 +++++++-----------
>>>  1 file changed, 7 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>> index 4d4db76a07da..ab980d468a10 100644
>> 
>> Let me suggest the following instead:
>> 
>> /* Try next first */
>> near = vma->vm_next;
>> if (near) {
>> 	anon_vma = reusable_anon_vma(near, vma, near);
>> 	if (anon_vma)
>> 		return anon_vma;
>> }
>> /* Try prev next */
>> near = vma->vm_prev;
>> if (near) {
>> 	anon_vma = reusable_anon_vma(near, vma, near);
>> 	if (anon_vma)
>> 		return anon_vma;
>> }
>> 
>
>Actually, it can be further simplified, because you don't need that last "if" statement, because you're returning NULL after this.
>
>So just return anon_vma there. (And adjust the comment block at the end, so that it's clear that anon_vma might be null.)
>
>

Many Thanks for all of your precious advice. I will fix my patch and send a v2 patch. Thanks again.