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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>