Message-ID: <4FCD6806.7070609@kernel.org>
Date:	Tue, 05 Jun 2012 10:59:34 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	KOSAKI Motohiro <kosaki.motohiro@...il.com>
CC:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	Hugh Dickins <hughd@...gle.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...hat.com>,
	Dave Jones <davej@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Cong Wang <amwang@...hat.com>,
	Markus Trippelsdorf <markus@...ppelsdorf.de>
Subject: Re: [PATCH v9] mm: compaction: handle incorrect MIGRATE_UNMOVABLE
 type pageblocks

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.





-- 
Kind regards,
Minchan Kim

View attachment "1.patch" of type "text/x-patch" (13392 bytes)

Powered by blists - more mailing lists