Re: [PATCH] mm: mmap: Simplify the failure return working flow

Re: [PATCH] mm: mmap: Simplify the failure return working flow

[gchen gchen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3237 bytes --]

<br>On Tue, 18 Aug 2015 15:57:08 -0700 akpm@linux-foundation.org wrote:<br>&gt;<br>&gt; On Wed, 19 Aug 2015 06:27:58 +0800 Chen Gang<br>&gt; &lt;xili_gchen_5257@hotmail.com&gt; wrote:<br>&gt;<br>&gt;&gt; From: Chen Gang &lt;xili_gchen_5257@hotmail.com&gt;<br>&gt;<br>&gt; As sent, this patch is From:you@hotmail and Signed-off-by:you@gmail.<br>&gt;<br>&gt; This is peculiar.  I'm assuming that it should have been From:you@gmail and<br>&gt; I have made that change to my copy of the patch.<br>&gt;<br>&gt; You can do this yourself by putting an explicit From: line at the start<br>&gt; of the changelog.<br>&gt;<br><br>Yes, it is really peculiar, the reason is gmail is not stable in China.<br>I have to send mail in my hotmail address.<br><br>But I still want to use my gmail as Signed-off-by, since I have already<br>used it, and also its name is a little formal than my hotmail.<br><br>Welcome any ideas, suggestions and completions for it (e.g. if it is<br>necessary to let send mail and Signed-off-by mail be the same, I shall<br>try).<br><br>[...]<br><br>&gt; So,<br>&gt;<br>&gt; --- a/mm/mmap.c~mm-mmap-simplify-the-failure-return-working-flow-fix<br>&gt; +++ a/mm/mmap.c<br>&gt; @@ -2952,7 +2952,7 @@ struct vm_area_struct *copy_vma(struct v<br>&gt;         } else {<br>&gt;                 new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);<br>&gt;                 if (!new_vma)<br>&gt; -                       return NULL;<br>&gt; +                       goto out;<br>&gt;                 *new_vma = *vma;<br>&gt;                 new_vma-&gt;vm_start = addr;<br>&gt;                 new_vma-&gt;vm_end = addr + len;<br>&gt; @@ -2971,10 +2971,11 @@ struct vm_area_struct *copy_vma(struct v<br>&gt;         }<br>&gt;         return new_vma;<br>&gt;<br>&gt; - out_free_mempol:<br>&gt; +out_free_mempol:<br>&gt;         mpol_put(vma_policy(new_vma));<br>&gt; - out_free_vma:<br>&gt; +out_free_vma:<br>&gt;         kmem_cache_free(vm_area_cachep, new_vma);<br>&gt; +out:<br>&gt;         return NULL;<br>&gt;  }<br>&gt;<br><br>It is OK to 
me, thanks.<br><br>During these days (2-4 months), I shall try to make some patches for<br>Linux mm:<br><br> - I am learning Linux kernel mmu, so I can re-use part of code to user<br>   mode (add softmmu to qemu linux user in my working time). Then I can<br>   try some mm patches when I am reading related code.<br><br> - At present, cross-building various archs with allmodconfig looks OK<br>   (have no many issues), so for me, I can stop and start another parts<br>   (e.g. mmu, loongson machine of mips arch, ...).<br><br>Welcome any ideas, suggestions and completions for it:<br><br> - Assume I am not quite familiar with mmu -- in honest, I feel I am<br>   really not.<br><br> - Is it possible to build the related 'softmmu' as a module which can<br>   be used by both kernel mode and user mode (if really it is, I shall<br>   try to perform it -- I can do it in my working time).<br><br> - ...<br><br>Thanks.<br>--<br>Chen Gang<br><br>Open, share, and attitude like air, water, and life which God blessed<br> 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mm: mmap: Simplify the failure return working flow

[gchen gchen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2834 bytes --]


On Tue, 18 Aug 2015 15:57:08 -0700 akpm@linux-foundation.org wrote:
>
> On Wed, 19 Aug 2015 06:27:58 +0800 Chen Gang
> <xili_gchen_5257@hotmail.com> wrote:
>
>> From: Chen Gang <xili_gchen_5257@hotmail.com>
>
> As sent, this patch is From:you@hotmail and Signed-off-by:you@gmail.
>
> This is peculiar.  I'm assuming that it should have been From:you@gmail and
> I have made that change to my copy of the patch.
>
> You can do this yourself by putting an explicit From: line at the start
> of the changelog.
>

Yes, it is really peculiar, the reason is gmail is not stable in China.
I have to send mail in my hotmail address.

But I still want to use my gmail as Signed-off-by, since I have already
used it, and also its name is a little formal than my hotmail.

Welcome any ideas, suggestions and completions for it (e.g. if it is
necessary to let send mail and Signed-off-by mail be the same, I shall
try).

[...]

> So,
>
> --- a/mm/mmap.c~mm-mmap-simplify-the-failure-return-working-flow-fix
> +++ a/mm/mmap.c
> @@ -2952,7 +2952,7 @@ struct vm_area_struct *copy_vma(struct v
>         } else {
>                 new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
>                 if (!new_vma)
> -                       return NULL;
> +                       goto out;
>                 *new_vma = *vma;
>                 new_vma->vm_start = addr;
>                 new_vma->vm_end = addr + len;
> @@ -2971,10 +2971,11 @@ struct vm_area_struct *copy_vma(struct v
>         }
>         return new_vma;
>
> - out_free_mempol:
> +out_free_mempol:
>         mpol_put(vma_policy(new_vma));
> - out_free_vma:
> +out_free_vma:
>         kmem_cache_free(vm_area_cachep, new_vma);
> +out:
>         return NULL;
>  }
>

It is OK to me, thanks.

During these days (2-4 months), I shall try to make some patches for
Linux mm:

 - I am learning Linux kernel mmu, so I can re-use part of code to user
   mode (add softmmu to qemu linux user in my working time). Then I can
   try some mm patches when I am reading related code.

 - At present, cross-building various archs with allmodconfig looks OK
   (have no many issues), so for me, I can stop and start another parts
   (e.g. mmu, loongson machine of mips arch, ...).

Welcome any ideas, suggestions and completions for it:

 - Assume I am not quite familiar with mmu -- in honest, I feel I am
   really not.

 - Is it possible to build the related 'softmmu' as a module which can
   be used by both kernel mode and user mode (if really it is, I shall
   try to perform it -- I can do it in my working time).

 - ...

Thanks.
--
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mm: mmap: Simplify the failure return working flow

[Michal Hocko]

On Thu 20-08-15 09:27:42, gchen gchen wrote:
[...]
> Yes, it is really peculiar, the reason is gmail is not stable in China.
> I have to send mail in my hotmail address.
> 
> But I still want to use my gmail as Signed-off-by, since I have already
> used it, and also its name is a little formal than my hotmail.
> 
> Welcome any ideas, suggestions and completions for it (e.g. if it is
> necessary to let send mail and Signed-off-by mail be the same, I shall
> try).

You can do the following in your .git/config

[user]
	name = YOUR_NAME_FOR_S-O-B
	email = YOUR_GMAIL_ADDRESS
[sendemail]
	from = YOUR_STABLE_SENDER_ADDRESS
	envelopesender = YOUR_STABLE_SENDER_ADDRESS
	smtpserver = YOUR_STABLE_SMTP

[user] part will be used for s-o-b and Author email while the sendemail
will be used for git send-email to route the patch properly. If the two
differ it will add From: user.name <user.email> as suggested by Andrew.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: mmap: Simplify the failure return working flow

[gchen gchen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1492 bytes --]

On 2015Äê08ÔÂ20ÈÕ 15:45, Michal Hocko wrote:
> On Thu 20-08-15 09:27:42, gchen gchen wrote:
> [...]
>> Yes, it is really peculiar, the reason is gmail is not stable in China.
>> I have to send mail in my hotmail address.
>>
>> But I still want to use my gmail as Signed-off-by, since I have already
>> used it, and also its name is a little formal than my hotmail.
>>
>> Welcome any ideas, suggestions and completions for it (e.g. if it is
>> necessary to let send mail and Signed-off-by mail be the same, I shall
>> try).
>
> You can do the following in your .git/config
>
> [user]
>       name = YOUR_NAME_FOR_S-O-B
>       email = YOUR_GMAIL_ADDRESS
> [sendemail]
>       from = YOUR_STABLE_SENDER_ADDRESS
>       envelopesender = YOUR_STABLE_SENDER_ADDRESS
>       smtpserver = YOUR_STABLE_SMTP
>
> [user] part will be used for s-o-b and Author email while the sendemail
> will be used for git send-email to route the patch properly. If the two
> differ it will add From: user.name <user.email> as suggested by Andrew.
>

OK, thank your very much for your details information. :-)

I shall try to use git to send/recv mails instead of current thunderbird
client (hope I can do it next time, although I am not quite sure).


Thanks.
--
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] mm: mmap: Simplify the failure return working flow

[Chen Gang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2343 bytes --]

----------------------------------------
> From: xili_gchen_5257@hotmail.com
> To: mhocko@kernel.org
> CC: akpm@linux-foundation.org; linux-kernel@vger.kernel.org; kirill.shutemov@linux.intel.com; riel@redhat.com; sasha.levin@oracle.com; linux-mm@kvack.org
> Subject: Re: [PATCH] mm: mmap: Simplify the failure return working flow
> Date: Thu, 20 Aug 2015 16:48:21 +0800
>
> On 2015年08月20日 15:45, Michal Hocko wrote:
>> On Thu 20-08-15 09:27:42, gchen gchen wrote:
>> [...]
>>> Yes, it is really peculiar, the reason is gmail is not stable in China.
>>> I have to send mail in my hotmail address.
>>>
>>> But I still want to use my gmail as Signed-off-by, since I have already
>>> used it, and also its name is a little formal than my hotmail.
>>>
>>> Welcome any ideas, suggestions and completions for it (e.g. if it is
>>> necessary to let send mail and Signed-off-by mail be the same, I shall
>>> try).
>>
>> You can do the following in your .git/config
>>
>> [user]
>> name = YOUR_NAME_FOR_S-O-B
>> email = YOUR_GMAIL_ADDRESS
>> [sendemail]
>> from = YOUR_STABLE_SENDER_ADDRESS
>> envelopesender = YOUR_STABLE_SENDER_ADDRESS
>> smtpserver = YOUR_STABLE_SMTP
>>
>> [user] part will be used for s-o-b and Author email while the sendemail
>> will be used for git send-email to route the patch properly. If the two
>> differ it will add From: user.name <user.email> as suggested by Andrew.
>>
>

Oh, sorry, it seems, I have to send mail in hotmail website (I can send gmail
neither under client nor under website).

linux kernel mailing list does not accept QQ mail. Either at present, I can
not send hotmail from client (thunderbird client, git client), I guess the
reason is the hotmail is blocked in China (but QQ is of cause OK in China).

So ... it is a bad news to us all. :-(  Welcome any related ideas, suggestions
an completions.

Thanks.

> OK, thank your very much for your details information. :-)
>
> I shall try to use git to send/recv mails instead of current thunderbird
> client (hope I can do it next time, although I am not quite sure).
>
>
> Thanks.
> --
> Chen Gang
>
> Open, share, and attitude like air, water, and life which God blessed
>
 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mm: mmap: Simplify the failure return working flow

[Andrew Morton]

On Wed, 19 Aug 2015 06:27:58 +0800 Chen Gang <xili_gchen_5257@hotmail.com> wrote:

> From: Chen Gang <xili_gchen_5257@hotmail.com>

As sent, this patch is From:you@hotmail and Signed-off-by:you@gmail.

This is peculiar.  I'm assuming that it should have been From:you@gmail and
I have made that change to my copy of the patch.

You can do this yourself by putting an explicit From: line at the start
of the changelog.


> @@ -2958,23 +2957,23 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>  		*need_rmap_locks = (new_vma->vm_pgoff <= vma->vm_pgoff);
>  	} else {
>  		new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
> -		if (new_vma) {
> -			*new_vma = *vma;
> -			new_vma->vm_start = addr;
> -			new_vma->vm_end = addr + len;
> -			new_vma->vm_pgoff = pgoff;
> -			if (vma_dup_policy(vma, new_vma))
> -				goto out_free_vma;
> -			INIT_LIST_HEAD(&new_vma->anon_vma_chain);
> -			if (anon_vma_clone(new_vma, vma))
> -				goto out_free_mempol;
> -			if (new_vma->vm_file)
> -				get_file(new_vma->vm_file);
> -			if (new_vma->vm_ops && new_vma->vm_ops->open)
> -				new_vma->vm_ops->open(new_vma);
> -			vma_link(mm, new_vma, prev, rb_link, rb_parent);
> -			*need_rmap_locks = false;
> -		}
> +		if (!new_vma)
> +			return NULL;
> +		*new_vma = *vma;
> +		new_vma->vm_start = addr;
> +		new_vma->vm_end = addr + len;
> +		new_vma->vm_pgoff = pgoff;
> +		if (vma_dup_policy(vma, new_vma))
> +			goto out_free_vma;
> +		INIT_LIST_HEAD(&new_vma->anon_vma_chain);
> +		if (anon_vma_clone(new_vma, vma))
> +			goto out_free_mempol;
> +		if (new_vma->vm_file)
> +			get_file(new_vma->vm_file);
> +		if (new_vma->vm_ops && new_vma->vm_ops->open)
> +			new_vma->vm_ops->open(new_vma);
> +		vma_link(mm, new_vma, prev, rb_link, rb_parent);
> +		*need_rmap_locks = false;
>  	}
>  	return new_vma;

Embedding a return deep inside the function isn't good.  It can lead to
resource leaks, locking leaks etc as the code evolves.  This is the
main reason why the kernel uses goto, IMO: single-entry, single-exit.

So,

--- a/mm/mmap.c~mm-mmap-simplify-the-failure-return-working-flow-fix
+++ a/mm/mmap.c
@@ -2952,7 +2952,7 @@ struct vm_area_struct *copy_vma(struct v
 	} else {
 		new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
 		if (!new_vma)
-			return NULL;
+			goto out;
 		*new_vma = *vma;
 		new_vma->vm_start = addr;
 		new_vma->vm_end = addr + len;
@@ -2971,10 +2971,11 @@ struct vm_area_struct *copy_vma(struct v
 	}
 	return new_vma;
 
- out_free_mempol:
+out_free_mempol:
 	mpol_put(vma_policy(new_vma));
- out_free_vma:
+out_free_vma:
 	kmem_cache_free(vm_area_cachep, new_vma);
+out:
 	return NULL;
 }
 
_

[PATCH] mm: mmap: Simplify the failure return working flow

[Chen Gang]

__split_vma() doesn't need out_err label, neither need initializing err.

copy_vma() can return NULL directly when kmem_cache_alloc() fails.

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

diff --git a/mm/mmap.c b/mm/mmap.c
index 8e0366e..35a4376 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2461,7 +2461,7 @@ static int __split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
 	      unsigned long addr, int new_below)
 {
 	struct vm_area_struct *new;
-	int err = -ENOMEM;
+	int err;
 
 	if (is_vm_hugetlb_page(vma) && (addr &
 					~(huge_page_mask(hstate_vma(vma)))))
@@ -2469,7 +2469,7 @@ static int __split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
 
 	new = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
 	if (!new)
-		goto out_err;
+		return -ENOMEM;
 
 	/* most fields are the same, copy all, and then fixup */
 	*new = *vma;
@@ -2517,7 +2517,6 @@ static int __split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
 	mpol_put(vma_policy(new));
  out_free_vma:
 	kmem_cache_free(vm_area_cachep, new);
- out_err:
 	return err;
 }
 
@@ -2958,23 +2957,23 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 		*need_rmap_locks = (new_vma->vm_pgoff <= vma->vm_pgoff);
 	} else {
 		new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
-		if (new_vma) {
-			*new_vma = *vma;
-			new_vma->vm_start = addr;
-			new_vma->vm_end = addr + len;
-			new_vma->vm_pgoff = pgoff;
-			if (vma_dup_policy(vma, new_vma))
-				goto out_free_vma;
-			INIT_LIST_HEAD(&new_vma->anon_vma_chain);
-			if (anon_vma_clone(new_vma, vma))
-				goto out_free_mempol;
-			if (new_vma->vm_file)
-				get_file(new_vma->vm_file);
-			if (new_vma->vm_ops && new_vma->vm_ops->open)
-				new_vma->vm_ops->open(new_vma);
-			vma_link(mm, new_vma, prev, rb_link, rb_parent);
-			*need_rmap_locks = false;
-		}
+		if (!new_vma)
+			return NULL;
+		*new_vma = *vma;
+		new_vma->vm_start = addr;
+		new_vma->vm_end = addr + len;
+		new_vma->vm_pgoff = pgoff;
+		if (vma_dup_policy(vma, new_vma))
+			goto out_free_vma;
+		INIT_LIST_HEAD(&new_vma->anon_vma_chain);
+		if (anon_vma_clone(new_vma, vma))
+			goto out_free_mempol;
+		if (new_vma->vm_file)
+			get_file(new_vma->vm_file);
+		if (new_vma->vm_ops && new_vma->vm_ops->open)
+			new_vma->vm_ops->open(new_vma);
+		vma_link(mm, new_vma, prev, rb_link, rb_parent);
+		*need_rmap_locks = false;
 	}
 	return new_vma;
 
-- 
1.9.3