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

[Wanpeng Li <liwanp@linux.vnet.ibm.com>]

On Thu, Aug 15, 2013 at 07:15:25PM +0800, Xishi Qiu wrote:
>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.
>

Hi Xishi,

You miss my point, I mean nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES); not 
make sense since nr_pages can't larger then MAX_ORDER_NR_PAGES. 

Regards,
Wanpeng Li 

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