linux-kernel - Re: [PATCH V4 1/4] mm: set page->pfmemalloc in prep_new

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 06 Jan 2015 22:10:55 +0100
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Michal Hocko <mhocko@...e.cz>
CC:	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Mel Gorman <mgorman@...e.de>,
	Zhang Yanfei <zhangyanfei@...fujitsu.com>,
	Minchan Kim <minchan@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Rik van Riel <riel@...hat.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH V4 1/4] mm: set page->pfmemalloc in prep_new_page()

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@...e.cz>
>> Cc: Mel Gorman <mgorman@...e.de>
>> Cc: Zhang Yanfei <zhangyanfei@...fujitsu.com>
>> Cc: Minchan Kim <minchan@...nel.org>
>> Cc: David Rientjes <rientjes@...gle.com>
>> Cc: Rik van Riel <riel@...hat.com>
>> Cc: "Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
>> Cc: Johannes Weiner <hannes@...xchg.org>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
>> Cc: Michal Hocko <mhocko@...e.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.

Well, we are returning instead. So there's no more code to follow by anyone
reading the function.

> Acked-by: Michal Hocko <mhocko@...e.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));

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/