2015-08-18 22:26:17

by Chen Gang

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

__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 <[email protected]>
---
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


2015-08-18 22:57:14

by Andrew Morton

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

On Wed, 19 Aug 2015 06:27:58 +0800 Chen Gang <[email protected]> wrote:

> From: Chen Gang <[email protected]>

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;
}

_

2015-08-20 01:34:29

by Chen Gang

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

<br>On Tue, 18 Aug 2015 15:57:08 -0700 [email protected] wrote:<br>&gt;<br>&gt; On Wed, 19 Aug 2015 06:27:58 +0800 Chen Gang<br>&gt; &lt;[email protected]&gt; wrote:<br>&gt;<br>&gt;&gt; From: Chen Gang &lt;[email protected]&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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-08-20 01:32:43

by Chen Gang

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


On Tue, 18 Aug 2015 15:57:08 -0700 [email protected] wrote:
>
> On Wed, 19 Aug 2015 06:27:58 +0800 Chen Gang
> <[email protected]> wrote:
>
>> From: Chen Gang <[email protected]>
>
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-08-20 07:45:26

by Michal Hocko

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

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

2015-08-20 08:48:26

by Chen Gang

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

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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-08-23 17:28:56

by Chen Gang

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

----------------------------------------
> From: [email protected]
> To: [email protected]
> CC: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?