Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1209519 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: mmap: Simplify the failure return working flow Chen Gang <xili_gchen_5257@hotmail.com> - 2015-08-19 00:30 +0200
    Re: [PATCH] mm: mmap: Simplify the failure return working flow Andrew Morton <akpm@linux-foundation.org> - 2015-08-19 01:00 +0200

#1209519 — [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 <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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1209532

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web