linux-kernel - Re: [PATCH] mm: skip the page buddy block instead of one page

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 15 Aug 2013 19:15:25 +0800
From:	Xishi Qiu <qiuxishi@...wei.com>
To:	Wanpeng Li <liwanp@...ux.vnet.ibm.com>
CC:	Minchan Kim <minchan@...nel.org>, Mel Gorman <mgorman@...e.de>,
	Andrew Morton <akpm@...ux-foundation.org>, <riel@...hat.com>,
	<aquini@...hat.com>, <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: skip the page buddy block instead of one page

On 2013/8/15 17:51, Wanpeng Li wrote:

> On Thu, Aug 15, 2013 at 03:45:11PM +0800, Xishi Qiu wrote:
>> On 2013/8/15 12:24, Minchan Kim wrote:
>>
>>>> Please read full thread in detail.
>>>>
>>>> Mel suggested following as
>>>>
>>>> if (PageBuddy(page)) {
>>>>         int nr_pages = (1 << page_order(page)) - 1;
>>>>         if (PageBuddy(page)) {
>>>>                 nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
>>>>                 low_pfn += nr_pages;
>>>>                 continue;
>>>>         }
>>>> }
>>>>
>>>> min(nr_pages, xxx) removes your concern but I think Mel's version
>>>> isn't right. It should be aligned with pageblock boundary so I 
>>>> suggested following.
>>>>
>>>> if (PageBuddy(page)) {
>>>> #ifdef CONFIG_MEMORY_ISOLATION
>>>> 	unsigned long order = page_order(page);
>>>> 	if (PageBuddy(page)) {
>>>> 		low_pfn += (1 << order) - 1;
>>>> 		low_pfn = min(low_pfn, end_pfn);
>>>> 	}
>>>> #endif
>>>> 	continue;
>>>> }
>>>>
>>
>> Hi Minchan,
>>
>> I understand now, but why use "end_pfn" here? 
>> Maybe like this:
>>
>> if (PageBuddy(page)) {
>> 	/*
>> 	 * page_order is racy without zone->lock but worst case
>> 	 * by the racing is just skipping pageblock_nr_pages.
>> 	 */
>> 	unsigned long nr_pages = 1 << page_order(page);
>> 	if (likely(PageBuddy(page))) {
>> 		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);
> 
> How much sense it make? nr_pages is still equal to itself since nr_pages can't 
> larger than MAX_ORDER_NR_PAGES.
> 

Hi Wanpeng,

Mel pointed "page_order cannot be used unless zone->lock is held".
"Even if the page is still page buddy, there is no guarantee that it's
the same page order as the first read. It could have be currently merging 
with adjacent buddies for example."

If someone use the page during the double PageBuddy check, the value
of private may be wrong. In my opinion, just keep the code unchanged.

Thanks,
Xishi Qiu

>>
>> 		/* Align with pageblock boundary */
>> 		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
>> 		    pageblock_nr_pages)
>> 			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
>> 		else
>> 			low_pfn += nr_pages - 1;
>> 	}
>> 	continue;
>> }
>>
>> Thanks,
>> Xishi Qiu
>>
>>>> so worst case is (pageblock_nr_pages - 1).
>>>> but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion
>>>> is following as.
>>>>
>>>> if (PageBuddy(page)) {
>>>> 	unsigned long order = page_order(page);
>>>> 	if (PageBuddy(page)) {
>>>> 		low_pfn += (1 << order) - 1;
>>>> 		low_pfn = min(low_pfn, end_pfn);
>>>
>>> Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).
>>>
>>>
>>>> 	}
>>>> 	continue;
>>>> }
>>>>



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