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

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

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