| Message ID | d359dcf73a7a868f1b126cb73368fea64aec1f25.1628174413.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some cleanup for page migration | expand |
On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > Remove redundant goto labels to simplify the code. TBH I don't see too much benefit. The "goto" makes the functions have a single exit point. > > Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > --- > mm/migrate.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 0ab364f..ed74fda 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page, > */ > VM_BUG_ON_PAGE(!PageIsolated(page), page); > if (!PageMovable(page)) { > - rc = MIGRATEPAGE_SUCCESS; > __ClearPageIsolated(page); > - goto out; > + return MIGRATEPAGE_SUCCESS; > } > > rc = mapping->a_ops->migratepage(mapping, newpage, > @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page, > flush_dcache_page(newpage); > > } > -out: > + > return rc; > } > > @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page, > newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE), > HPAGE_PMD_ORDER); > if (!newpage) > - goto out; > + return NULL; > > prep_transhuge_page(newpage); > > -out: > return newpage; > } > > -- > 1.8.3.1 > >
Hi Yang, > On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang > <baolin.wang@linux.alibaba.com> wrote: >> >> Remove redundant goto labels to simplify the code. > > TBH I don't see too much benefit. The "goto" makes the functions have > a single exit point. Yes, I agree that the 'goto' statement can make things easier when a function exits from multiple locations and some common work such as cleanup has to be done, as well as introducing complexity to reading the code. So per the coding style documentation, "If there is no cleanup needed then just return directly", which can make code more readable I think :) But I have no strong opinion on this, I can drop this patch if you still think this is unnecessary. Thanks for your review and comments. >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >> --- >> mm/migrate.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 0ab364f..ed74fda 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page, >> */ >> VM_BUG_ON_PAGE(!PageIsolated(page), page); >> if (!PageMovable(page)) { >> - rc = MIGRATEPAGE_SUCCESS; >> __ClearPageIsolated(page); >> - goto out; >> + return MIGRATEPAGE_SUCCESS; >> } >> >> rc = mapping->a_ops->migratepage(mapping, newpage, >> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page, >> flush_dcache_page(newpage); >> >> } >> -out: >> + >> return rc; >> } >> >> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page, >> newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE), >> HPAGE_PMD_ORDER); >> if (!newpage) >> - goto out; >> + return NULL; >> >> prep_transhuge_page(newpage); >> >> -out: >> return newpage; >> } >> >> -- >> 1.8.3.1 >> >>
On Thu, Aug 5, 2021 at 8:19 PM Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > Hi Yang, > > > On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang > > <baolin.wang@linux.alibaba.com> wrote: > >> > >> Remove redundant goto labels to simplify the code. > > > > TBH I don't see too much benefit. The "goto" makes the functions have > > a single exit point. > > Yes, I agree that the 'goto' statement can make things easier when a > function exits from multiple locations and some common work such as > cleanup has to be done, as well as introducing complexity to reading the > code. So per the coding style documentation, "If there is no cleanup > needed then just return directly", which can make code more readable I > think :) > > But I have no strong opinion on this, I can drop this patch if you still > think this is unnecessary. Thanks for your review and comments. Thanks, IMHO I'd like to drop it for now. > > >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > >> --- > >> mm/migrate.c | 8 +++----- > >> 1 file changed, 3 insertions(+), 5 deletions(-) > >> > >> diff --git a/mm/migrate.c b/mm/migrate.c > >> index 0ab364f..ed74fda 100644 > >> --- a/mm/migrate.c > >> +++ b/mm/migrate.c > >> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page, > >> */ > >> VM_BUG_ON_PAGE(!PageIsolated(page), page); > >> if (!PageMovable(page)) { > >> - rc = MIGRATEPAGE_SUCCESS; > >> __ClearPageIsolated(page); > >> - goto out; > >> + return MIGRATEPAGE_SUCCESS; > >> } > >> > >> rc = mapping->a_ops->migratepage(mapping, newpage, > >> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page, > >> flush_dcache_page(newpage); > >> > >> } > >> -out: > >> + > >> return rc; > >> } > >> > >> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page, > >> newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE), > >> HPAGE_PMD_ORDER); > >> if (!newpage) > >> - goto out; > >> + return NULL; > >> > >> prep_transhuge_page(newpage); > >> > >> -out: > >> return newpage; > >> } > >> > >> -- > >> 1.8.3.1 > >> > >>
> On Thu, Aug 5, 2021 at 8:19 PM Baolin Wang > <baolin.wang@linux.alibaba.com> wrote: >> >> Hi Yang, >> >>> On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang >>> <baolin.wang@linux.alibaba.com> wrote: >>>> >>>> Remove redundant goto labels to simplify the code. >>> >>> TBH I don't see too much benefit. The "goto" makes the functions have >>> a single exit point. >> >> Yes, I agree that the 'goto' statement can make things easier when a >> function exits from multiple locations and some common work such as >> cleanup has to be done, as well as introducing complexity to reading the >> code. So per the coding style documentation, "If there is no cleanup >> needed then just return directly", which can make code more readable I >> think :) >> >> But I have no strong opinion on this, I can drop this patch if you still >> think this is unnecessary. Thanks for your review and comments. > > Thanks, IMHO I'd like to drop it for now. OK, will do. Thanks.
diff --git a/mm/migrate.c b/mm/migrate.c index 0ab364f..ed74fda 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page, */ VM_BUG_ON_PAGE(!PageIsolated(page), page); if (!PageMovable(page)) { - rc = MIGRATEPAGE_SUCCESS; __ClearPageIsolated(page); - goto out; + return MIGRATEPAGE_SUCCESS; } rc = mapping->a_ops->migratepage(mapping, newpage, @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page, flush_dcache_page(newpage); } -out: + return rc; } @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page, newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE), HPAGE_PMD_ORDER); if (!newpage) - goto out; + return NULL; prep_transhuge_page(newpage); -out: return newpage; }
Remove redundant goto labels to simplify the code. Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- mm/migrate.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)