Re: [PATCH V4 1/4] mm: set page->pfmemalloc in prep_new_page()

[Michal Hocko <mhocko@suse.cz>]

On Mon 05-01-15 18:17:40, Vlastimil Babka wrote:
> The function prep_new_page() sets almost everything in the struct page of the
> page being allocated, except page->pfmemalloc. This is not obvious and has at
> least once led to a bug where page->pfmemalloc was forgotten to be set
> correctly, see commit 8fb74b9fb2b1 ("mm: compaction: partially revert capture
> of suitable high-order page").
> 
> This patch moves the pfmemalloc setting to prep_new_page(), which means it
> needs to gain alloc_flags parameter. The call to prep_new_page is moved from
> buffered_rmqueue() to get_page_from_freelist(), which also leads to simpler
> code. An obsolete comment for buffered_rmqueue() is replaced.
> 
> In addition to better maintainability there is a small reduction of code and
> stack usage for get_page_from_freelist(), which inlines the other functions
> involved.
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-145 (-145)
> function                                     old     new   delta
> get_page_from_freelist                      2670    2525    -145
> 
> Stack usage is reduced from 184 to 168 bytes.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Michal Hocko <mhocko@suse.cz>

get_page_from_freelist has grown too hairy. I agree that it is tiny less
confusing now because we are not breaking out of the loop in the
successful case.

Acked-by: Michal Hocko <mhocko@suse.cz>

[...]
> @@ -2177,25 +2181,16 @@ zonelist_scan:
>  try_this_zone:
>  		page = buffered_rmqueue(preferred_zone, zone, order,
>  						gfp_mask, migratetype);
> -		if (page)
> -			break;
> +		if (page) {
> +			if (prep_new_page(page, order, gfp_mask, alloc_flags))
> +				goto try_this_zone;
> +			return page;
> +		}

I would probably liked `do {} while ()' more because it wouldn't use the
goto, but this is up to you:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1bb65e6f48dd..1682d766cb8e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2175,10 +2175,11 @@ zonelist_scan:
 		}
 
 try_this_zone:
-		page = buffered_rmqueue(preferred_zone, zone, order,
+		do {
+			page = buffered_rmqueue(preferred_zone, zone, order,
 						gfp_mask, migratetype);
-		if (page)
-			break;
+		} while (page && prep_new_page(page, order, gfp_mask,
+					       alloc_flags));
 this_zone_full:
 		if (IS_ENABLED(CONFIG_NUMA) && zlc_active)
 			zlc_mark_zone_full(zonelist, z);

[...]
-- 
Michal Hocko
SUSE Labs

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