On 01/06/2015 03:30 PM, Michal Hocko wrote:
> 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@xxxxxxx>
>> Cc: Mel Gorman <mgorman@xxxxxxx>
>> Cc: Zhang Yanfei <zhangyanfei@xxxxxxxxxxxxxx>
>> Cc: Minchan Kim <minchan@xxxxxxxxxx>
>> Cc: David Rientjes <rientjes@xxxxxxxxxx>
>> Cc: Rik van Riel <riel@xxxxxxxxxx>
>> Cc: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxx>
>
> 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.
Well, we are returning instead. So there's no more code to follow by anyone
reading the function.
> Acked-by: Michal Hocko <mhocko@xxxxxxx>
>
> [...]
>> @@ -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));
Hm but here we wouldn't return page on success. I wonder if you overlooked the
return, hence your "not breaking out of the loop" remark?
> this_zone_full:
> if (IS_ENABLED(CONFIG_NUMA) && zlc_active)
> zlc_mark_zone_full(zonelist, z);
>
> [...]
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>