* [PATCH] mm, hugetlb: move the error handle logic out of normal code path
@ 2014-05-14 7:10 Jianyu Zhan
2014-05-15 9:01 ` Michal Hocko
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jianyu Zhan @ 2014-05-14 7:10 UTC (permalink / raw)
To: akpm, iamjoonsoo.kim, aneesh.kumar, n-horiguchi, mhocko,
aarcange, steve.capper, davidlohr, kirill.shutemov, dave.hansen
Cc: linux-mm, linux-kernel, nasa4836
alloc_huge_page() now mixes normal code path with error handle logic.
This patches move out the error handle logic, to make normal code
path more clean and redue code duplicate.
Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
mm/hugetlb.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 26b1464..e81c69e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1246,24 +1246,17 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
return ERR_PTR(-ENOSPC);
ret = hugetlb_cgroup_charge_cgroup(idx, pages_per_huge_page(h), &h_cg);
- if (ret) {
- if (chg || avoid_reserve)
- hugepage_subpool_put_pages(spool, 1);
- return ERR_PTR(-ENOSPC);
- }
+ if (ret)
+ goto out_subpool_put;
+
spin_lock(&hugetlb_lock);
page = dequeue_huge_page_vma(h, vma, addr, avoid_reserve, chg);
if (!page) {
spin_unlock(&hugetlb_lock);
page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
- if (!page) {
- hugetlb_cgroup_uncharge_cgroup(idx,
- pages_per_huge_page(h),
- h_cg);
- if (chg || avoid_reserve)
- hugepage_subpool_put_pages(spool, 1);
- return ERR_PTR(-ENOSPC);
- }
+ if (!page)
+ goto out_uncharge_cgroup;
+
spin_lock(&hugetlb_lock);
list_move(&page->lru, &h->hugepage_activelist);
/* Fall through */
@@ -1275,6 +1268,13 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
vma_commit_reservation(h, vma, addr);
return page;
+
+out_uncharge_cgroup:
+ hugetlb_cgroup_uncharge_cgroup(idx, pages_per_huge_page(h), h_cg);
+out_subpool_put:
+ if (chg || avoid_reserve)
+ hugepage_subpool_put_pages(spool, 1);
+ return ERR_PTR(-ENOSPC);
}
/*
--
2.0.0-rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugetlb: move the error handle logic out of normal code path
2014-05-14 7:10 [PATCH] mm, hugetlb: move the error handle logic out of normal code path Jianyu Zhan
@ 2014-05-15 9:01 ` Michal Hocko
2014-05-15 22:36 ` Andrew Morton
2014-05-15 22:38 ` Davidlohr Bueso
2014-05-18 17:27 ` Aneesh Kumar K.V
2 siblings, 1 reply; 6+ messages in thread
From: Michal Hocko @ 2014-05-15 9:01 UTC (permalink / raw)
To: Jianyu Zhan
Cc: akpm, iamjoonsoo.kim, aneesh.kumar, n-horiguchi, aarcange,
steve.capper, davidlohr, kirill.shutemov, dave.hansen, linux-mm,
linux-kernel
On Wed 14-05-14 15:10:59, Jianyu Zhan wrote:
> alloc_huge_page() now mixes normal code path with error handle logic.
> This patches move out the error handle logic, to make normal code
> path more clean and redue code duplicate.
I don't know. Part of the function returns and cleans up on its own and
other part relies on clean up labels. This is not so much nicer than the
previous state.
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
> ---
> mm/hugetlb.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 26b1464..e81c69e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1246,24 +1246,17 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
> return ERR_PTR(-ENOSPC);
>
> ret = hugetlb_cgroup_charge_cgroup(idx, pages_per_huge_page(h), &h_cg);
> - if (ret) {
> - if (chg || avoid_reserve)
> - hugepage_subpool_put_pages(spool, 1);
> - return ERR_PTR(-ENOSPC);
> - }
> + if (ret)
> + goto out_subpool_put;
> +
> spin_lock(&hugetlb_lock);
> page = dequeue_huge_page_vma(h, vma, addr, avoid_reserve, chg);
> if (!page) {
> spin_unlock(&hugetlb_lock);
> page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
> - if (!page) {
> - hugetlb_cgroup_uncharge_cgroup(idx,
> - pages_per_huge_page(h),
> - h_cg);
> - if (chg || avoid_reserve)
> - hugepage_subpool_put_pages(spool, 1);
> - return ERR_PTR(-ENOSPC);
> - }
> + if (!page)
> + goto out_uncharge_cgroup;
> +
> spin_lock(&hugetlb_lock);
> list_move(&page->lru, &h->hugepage_activelist);
> /* Fall through */
> @@ -1275,6 +1268,13 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
>
> vma_commit_reservation(h, vma, addr);
> return page;
> +
> +out_uncharge_cgroup:
> + hugetlb_cgroup_uncharge_cgroup(idx, pages_per_huge_page(h), h_cg);
> +out_subpool_put:
> + if (chg || avoid_reserve)
> + hugepage_subpool_put_pages(spool, 1);
> + return ERR_PTR(-ENOSPC);
> }
>
> /*
> --
> 2.0.0-rc3
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugetlb: move the error handle logic out of normal code path
2014-05-15 9:01 ` Michal Hocko
@ 2014-05-15 22:36 ` Andrew Morton
2014-05-16 7:57 ` Michal Hocko
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2014-05-15 22:36 UTC (permalink / raw)
To: Michal Hocko
Cc: Jianyu Zhan, iamjoonsoo.kim, aneesh.kumar, n-horiguchi, aarcange,
steve.capper, davidlohr, kirill.shutemov, dave.hansen, linux-mm,
linux-kernel
On Thu, 15 May 2014 11:01:42 +0200 Michal Hocko <mhocko@suse.cz> wrote:
> On Wed 14-05-14 15:10:59, Jianyu Zhan wrote:
> > alloc_huge_page() now mixes normal code path with error handle logic.
> > This patches move out the error handle logic, to make normal code
> > path more clean and redue code duplicate.
>
> I don't know. Part of the function returns and cleans up on its own and
> other part relies on clean up labels. This is not so much nicer than the
> previous state.
That's actually a common pattern:
foo()
{
if (check which doesn't change any state)
return -Efoo;
if (another check which doesn't change any state)
return -Ebar;
do_something_which_changes_state()
if (another check)
goto undo_that_state_chage;
...
undo_that_state_change:
...
}
This ties into the main reason why we use all these gotos: to support
evolution of the code. With multiple return points we risk later
adding resource leaks and locking errors. Plus the code becomes more
and more duplicative and spaghettified.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugetlb: move the error handle logic out of normal code path
2014-05-14 7:10 [PATCH] mm, hugetlb: move the error handle logic out of normal code path Jianyu Zhan
2014-05-15 9:01 ` Michal Hocko
@ 2014-05-15 22:38 ` Davidlohr Bueso
2014-05-18 17:27 ` Aneesh Kumar K.V
2 siblings, 0 replies; 6+ messages in thread
From: Davidlohr Bueso @ 2014-05-15 22:38 UTC (permalink / raw)
To: Jianyu Zhan
Cc: akpm, iamjoonsoo.kim, aneesh.kumar, n-horiguchi, mhocko,
aarcange, steve.capper, kirill.shutemov, dave.hansen, linux-mm,
linux-kernel
On Wed, 2014-05-14 at 15:10 +0800, Jianyu Zhan wrote:
> alloc_huge_page() now mixes normal code path with error handle logic.
> This patches move out the error handle logic, to make normal code
> path more clean and redue code duplicate.
>
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
Acked-by: Davidlohr Bueso <davidlohr@hp.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugetlb: move the error handle logic out of normal code path
2014-05-15 22:36 ` Andrew Morton
@ 2014-05-16 7:57 ` Michal Hocko
0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2014-05-16 7:57 UTC (permalink / raw)
To: Andrew Morton
Cc: Jianyu Zhan, iamjoonsoo.kim, aneesh.kumar, n-horiguchi, aarcange,
steve.capper, davidlohr, kirill.shutemov, dave.hansen, linux-mm,
linux-kernel
On Thu 15-05-14 15:36:20, Andrew Morton wrote:
> On Thu, 15 May 2014 11:01:42 +0200 Michal Hocko <mhocko@suse.cz> wrote:
>
> > On Wed 14-05-14 15:10:59, Jianyu Zhan wrote:
> > > alloc_huge_page() now mixes normal code path with error handle logic.
> > > This patches move out the error handle logic, to make normal code
> > > path more clean and redue code duplicate.
> >
> > I don't know. Part of the function returns and cleans up on its own and
> > other part relies on clean up labels. This is not so much nicer than the
> > previous state.
>
> That's actually a common pattern:
>
> foo()
> {
> if (check which doesn't change any state)
> return -Efoo;
> if (another check which doesn't change any state)
> return -Ebar;
>
> do_something_which_changes_state()
>
> if (another check)
> goto undo_that_state_chage;
> ...
>
> undo_that_state_change:
> ...
> }
Right. I have misread the previous vma_needs_reservation and
hugepage_subpool_get_pages error path and already considered it as a
changing state one. Sorry about the confusion.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugetlb: move the error handle logic out of normal code path
2014-05-14 7:10 [PATCH] mm, hugetlb: move the error handle logic out of normal code path Jianyu Zhan
2014-05-15 9:01 ` Michal Hocko
2014-05-15 22:38 ` Davidlohr Bueso
@ 2014-05-18 17:27 ` Aneesh Kumar K.V
2 siblings, 0 replies; 6+ messages in thread
From: Aneesh Kumar K.V @ 2014-05-18 17:27 UTC (permalink / raw)
To: Jianyu Zhan, akpm, iamjoonsoo.kim, n-horiguchi, mhocko, aarcange,
steve.capper, davidlohr, kirill.shutemov, dave.hansen
Cc: linux-mm, linux-kernel, nasa4836
Jianyu Zhan <nasa4836@gmail.com> writes:
> alloc_huge_page() now mixes normal code path with error handle logic.
> This patches move out the error handle logic, to make normal code
> path more clean and redue code duplicate.
>
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> mm/hugetlb.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 26b1464..e81c69e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1246,24 +1246,17 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
> return ERR_PTR(-ENOSPC);
>
> ret = hugetlb_cgroup_charge_cgroup(idx, pages_per_huge_page(h), &h_cg);
> - if (ret) {
> - if (chg || avoid_reserve)
> - hugepage_subpool_put_pages(spool, 1);
> - return ERR_PTR(-ENOSPC);
> - }
> + if (ret)
> + goto out_subpool_put;
> +
> spin_lock(&hugetlb_lock);
> page = dequeue_huge_page_vma(h, vma, addr, avoid_reserve, chg);
> if (!page) {
> spin_unlock(&hugetlb_lock);
> page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
> - if (!page) {
> - hugetlb_cgroup_uncharge_cgroup(idx,
> - pages_per_huge_page(h),
> - h_cg);
> - if (chg || avoid_reserve)
> - hugepage_subpool_put_pages(spool, 1);
> - return ERR_PTR(-ENOSPC);
> - }
> + if (!page)
> + goto out_uncharge_cgroup;
> +
> spin_lock(&hugetlb_lock);
> list_move(&page->lru, &h->hugepage_activelist);
> /* Fall through */
> @@ -1275,6 +1268,13 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
>
> vma_commit_reservation(h, vma, addr);
> return page;
> +
> +out_uncharge_cgroup:
> + hugetlb_cgroup_uncharge_cgroup(idx, pages_per_huge_page(h), h_cg);
> +out_subpool_put:
> + if (chg || avoid_reserve)
> + hugepage_subpool_put_pages(spool, 1);
> + return ERR_PTR(-ENOSPC);
> }
>
> /*
> --
> 2.0.0-rc3
>
> --
> 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-18 17:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-14 7:10 [PATCH] mm, hugetlb: move the error handle logic out of normal code path Jianyu Zhan
2014-05-15 9:01 ` Michal Hocko
2014-05-15 22:36 ` Andrew Morton
2014-05-16 7:57 ` Michal Hocko
2014-05-15 22:38 ` Davidlohr Bueso
2014-05-18 17:27 ` Aneesh Kumar K.V
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).