| Message ID | 20190817105102.11732-1-lpf.vector@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page_alloc: cleanup __alloc_pages_direct_compact() | expand |
On 8/17/19 12:51 PM, Pengfei Li wrote: > This patch cleans up the if(page). > > No functional change. > > Signed-off-by: Pengfei Li <lpf.vector@gmail.com> I don't see much benefit here. The indentation wasn't that bad that it had to be reduced using goto. But the patch is not incorrect so I'm not NACKing. > --- > mm/page_alloc.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 272c6de1bf4e..51f056ac09f5 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3890,6 +3890,7 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, > enum compact_priority prio, enum compact_result *compact_result) > { > struct page *page = NULL; > + struct zone *zone; > unsigned long pflags; > unsigned int noreclaim_flag; > > @@ -3911,23 +3912,26 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, > */ > count_vm_event(COMPACTSTALL); > > - /* Prep a captured page if available */ > - if (page) > + if (page) { > + /* Prep a captured page if available */ > prep_new_page(page, order, gfp_mask, alloc_flags); > - > - /* Try get a page from the freelist if available */ > - if (!page) > + } else { > + /* Try get a page from the freelist if available */ > page = get_page_from_freelist(gfp_mask, order, alloc_flags, ac); > > - if (page) { > - struct zone *zone = page_zone(page); > - > - zone->compact_blockskip_flush = false; > - compaction_defer_reset(zone, order, true); > - count_vm_event(COMPACTSUCCESS); > - return page; > + if (!page) > + goto failed; > } > > + zone = page_zone(page); > + zone->compact_blockskip_flush = false; > + compaction_defer_reset(zone, order, true); > + > + count_vm_event(COMPACTSUCCESS); > + > + return page; > + > +failed: > /* > * It's bad if compaction run occurs and fails. The most likely reason > * is that pages exist, but not enough to satisfy watermarks. >
On Mon, Aug 19, 2019 at 9:50 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 8/17/19 12:51 PM, Pengfei Li wrote: > > This patch cleans up the if(page). > > > > No functional change. > > > > Signed-off-by: Pengfei Li <lpf.vector@gmail.com> > > I don't see much benefit here. The indentation wasn't that bad that it > had to be reduced using goto. But the patch is not incorrect so I'm not > NACKing. > Thanks for your review and comments. This patch reduces the number of times the if(page) (as the compiler does), and the downside is that there is a goto. If this improves readability, accept it. Otherwise, leave it as it is. Thanks again. --- Pengfei > > --- > > mm/page_alloc.c | 28 ++++++++++++++++------------ > > 1 file changed, 16 insertions(+), 12 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 272c6de1bf4e..51f056ac09f5 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3890,6 +3890,7 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, > > enum compact_priority prio, enum compact_result *compact_result) > > { > > struct page *page = NULL; > > + struct zone *zone; > > unsigned long pflags; > > unsigned int noreclaim_flag; > > > > @@ -3911,23 +3912,26 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, > > */ > > count_vm_event(COMPACTSTALL); > > > > - /* Prep a captured page if available */ > > - if (page) > > + if (page) { > > + /* Prep a captured page if available */ > > prep_new_page(page, order, gfp_mask, alloc_flags); > > - > > - /* Try get a page from the freelist if available */ > > - if (!page) > > + } else { > > + /* Try get a page from the freelist if available */ > > page = get_page_from_freelist(gfp_mask, order, alloc_flags, ac); > > > > - if (page) { > > - struct zone *zone = page_zone(page); > > - > > - zone->compact_blockskip_flush = false; > > - compaction_defer_reset(zone, order, true); > > - count_vm_event(COMPACTSUCCESS); > > - return page; > > + if (!page) > > + goto failed; > > } > > > > + zone = page_zone(page); > > + zone->compact_blockskip_flush = false; > > + compaction_defer_reset(zone, order, true); > > + > > + count_vm_event(COMPACTSUCCESS); > > + > > + return page; > > + > > +failed: > > /* > > * It's bad if compaction run occurs and fails. The most likely reason > > * is that pages exist, but not enough to satisfy watermarks. > > >
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 272c6de1bf4e..51f056ac09f5 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3890,6 +3890,7 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, enum compact_priority prio, enum compact_result *compact_result) { struct page *page = NULL; + struct zone *zone; unsigned long pflags; unsigned int noreclaim_flag; @@ -3911,23 +3912,26 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, */ count_vm_event(COMPACTSTALL); - /* Prep a captured page if available */ - if (page) + if (page) { + /* Prep a captured page if available */ prep_new_page(page, order, gfp_mask, alloc_flags); - - /* Try get a page from the freelist if available */ - if (!page) + } else { + /* Try get a page from the freelist if available */ page = get_page_from_freelist(gfp_mask, order, alloc_flags, ac); - if (page) { - struct zone *zone = page_zone(page); - - zone->compact_blockskip_flush = false; - compaction_defer_reset(zone, order, true); - count_vm_event(COMPACTSUCCESS); - return page; + if (!page) + goto failed; } + zone = page_zone(page); + zone->compact_blockskip_flush = false; + compaction_defer_reset(zone, order, true); + + count_vm_event(COMPACTSUCCESS); + + return page; + +failed: /* * It's bad if compaction run occurs and fails. The most likely reason * is that pages exist, but not enough to satisfy watermarks.
This patch cleans up the if(page). No functional change. Signed-off-by: Pengfei Li <lpf.vector@gmail.com> --- mm/page_alloc.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)