On 06/05/2012 10:59 AM, Minchan Kim wrote:

> On 06/05/2012 05:22 AM, KOSAKI Motohiro wrote:
> 
>>> +/*
>>> + * Returns true if MIGRATE_UNMOVABLE pageblock can be successfully
>>> + * converted to MIGRATE_MOVABLE type, false otherwise.
>>> + */
>>> +static bool can_rescue_unmovable_pageblock(struct page *page, bool
>>> locked)
>>> +{
>>> +    unsigned long pfn, start_pfn, end_pfn;
>>> +    struct page *start_page, *end_page, *cursor_page;
>>> +
>>> +    pfn = page_to_pfn(page);
>>> +    start_pfn = pfn&  ~(pageblock_nr_pages - 1);
>>> +    end_pfn = start_pfn + pageblock_nr_pages - 1;
>>> +
>>> +    start_page = pfn_to_page(start_pfn);
>>> +    end_page = pfn_to_page(end_pfn);
>>> +
>>> +    for (cursor_page = start_page, pfn = start_pfn; cursor_page<=
>>> end_page;
>>> +        pfn++, cursor_page++) {
>>> +        struct zone *zone = page_zone(start_page);
>>> +        unsigned long flags;
>>> +
>>> +        if (!pfn_valid_within(pfn))
>>> +            continue;
>>> +
>>> +        /* Do not deal with pageblocks that overlap zones */
>>> +        if (page_zone(cursor_page) != zone)
>>> +            return false;
>>> +
>>> +        if (!locked)
>>> +            spin_lock_irqsave(&zone->lock, flags);
>>> +
>>> +        if (PageBuddy(cursor_page)) {
>>> +            int order = page_order(cursor_page);
>>>
>>> -/* Returns true if the page is within a block suitable for migration
>>> to */
>>> -static bool suitable_migration_target(struct page *page)
>>> +            pfn += (1<<  order) - 1;
>>> +            cursor_page += (1<<  order) - 1;
>>> +
>>> +            if (!locked)
>>> +                spin_unlock_irqrestore(&zone->lock, flags);
>>> +            continue;
>>> +        } else if (page_count(cursor_page) == 0 ||
>>> +               PageLRU(cursor_page)) {
>>> +            if (!locked)
>>> +                spin_unlock_irqrestore(&zone->lock, flags);
>>> +            continue;
>>> +        }
>>> +
>>> +        if (!locked)
>>> +            spin_unlock_irqrestore(&zone->lock, flags);
>>> +
>>> +        return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>
>> Minchan, are you interest this patch? If yes, can you please rewrite it?
> 
> 
> Can do it but I want to give credit to Bartlomiej.
> Bartlomiej, if you like my patch, could you resend it as formal patch after you do broad testing?
> 
> 
>> This one are
>> not fixed our pointed issue and can_rescue_unmovable_pageblock() still
>> has plenty bugs.
>> We can't ack it.
>>
>> -- 
> 
> 
> Frankly speaking, I don't want to merge it without any data which prove it's really good for real practice.
> 
> When the patch firstly was submitted, it wasn't complicated so I was okay at that time but it has been complicated
> than my expectation. So if Andrew might pass the decision to me, I'm totally NACK if author doesn't provide
> any real data or VOC of some client.
> 
> 1) Any comment?
> 
> Anyway, I fixed some bugs and clean up something I found during review.
> 
> Minor thing.
> 1. change smt_result naming - I never like such long non-consistent naming. How about this?
> 2. fix can_rescue_unmovable_pageblock 
>    2.1 pfn valid check for page_zone
> 
> Major thing.
> 
>    2.2 add lru_lock for stablizing PageLRU
>        If we don't hold lru_lock, there is possibility that unmovable(non-LRU) page can put in movable pageblock.
>        It can make compaction/CMA's regression. But there is a concern about deadlock between lru_lock and lock.
>        As I look the code, I can't find allocation trial with holding lru_lock so it might be safe(but not sure,
>        I didn't test it. It need more careful review/testing) but it makes new locking dependency(not sure, too.
>        We already made such rule but I didn't know that until now ;-) ) Why I thought so is we can allocate
>        GFP_ATOMIC with holding lru_lock, logically which might be crazy idea.
> 
>    2.3 remove zone->lock in first phase.
>        We do rescue unmovable pageblock by 2-phase. In first-phase, we just peek pages so we don't need locking.
>        If we see non-stablizing value, it would be caught by 2-phase with needed lock or 
>        can_rescue_unmovable_pageblock can return out of loop by stale page_order(cursor_page).
>        It couldn't make unmovable pageblock to movable but we can do it next time, again.
>        It's not critical.
> 
> 2) Any comment?
> 
> Now I can't inline the code so sorry but attach patch.
> It's not a formal patch/never tested.
> 


Attached patch has a BUG in can_rescue_unmovable_pageblock.
Resend. I hope it is fixed.

 



-- 
Kind regards,
Minchan Kim