Re: [PATCH] fix: decrease NR_FREE_PAGES when isolate page from buddy

[Vlastimil Babka <vbabka@suse.cz>]

On 3.7.2015 9:15, "e1?e? 1/4 e2 1/2 /iGBP 1/4 i??i??eu!i??/SW Platform(i??)AOTi??
(minkyung88.kim@lge.com)" wrote:
> As Vlastimil Babka expalin, this patch is useless and working not correctly.
> Thank you for your review :)

Thanks for reminding us of the code that should be cleaned up :)
I have the patches almost ready and will send them later on.

Thanks.

> 2015-07-02 i??i?? 6:52i?? Vlastimil Babka i?'(e??) i?' e,?:
>> [+CC Joonsoo and Minchan]
>>
>> On 07/01/2015 03:17 AM, minkyung88.kim@lge.com wrote:
>>> From: "minkyung88.kim" <minkyung88.kim@lge.com>
>>>
>>> NR_FREEPAGE should be decreased when pages are isolated from buddy.
>>> Therefore fix the count.
>>
>> Did you really observe an accounting bug and this patch fixed it, or 
>> is it just because of code inspection?
>>
>> The patched code has this comment:
>>
>> /*
>>  * If race between isolatation and allocation happens,
>>  * some free pages could be in MIGRATE_MOVABLE list
>>  * although pageblock's migratation type of the page
>>  * is MIGRATE_ISOLATE. Catch it and move the page into
>>  * MIGRATE_ISOLATE list.
>>  */
>>
>> This is from 2012 and I'm not sure if it still applies. Joonsoo's 
>> series last year was to eliminate these races, see e.g. 51bb1a4093 
>> ("mm/page_alloc: add freepage on isolate pageblock to correct buddy 
>> list").
>>
>> So I think that this piece of code shouldn't be useful anymore. Well, 
>> actually I think it can trigger, but it's a false positive and (before 
>> your patch) result in basically a no-op. The reason is that the value 
>> of get_freepage_migratetype(page) is a just an optimization used only 
>> for pages on pcplists. It's not guaranteed to be correct for pages in 
>> the buddy free lists (and it can get stale even on the pcplists).
>>
>> Now, the code from Joonsoo's patch mentioned above does this in
>> free_pcppages_bulk():
>>
>> mt = get_freepage_migratetype(page);
>> if (unlikely(has_isolate_pageblock(zone)))
>>         mt = get_pageblock_migratetype(page);
>>
>> /* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
>> __free_one_page(page, page_to_pfn(page), zone, 0, mt);
>>
>> So if get_freepage_migratetype(page) returns e.g. MIGRATE_MOVABLE but 
>> the pageblock is MIGRATE_ISOLATE, it will catch this and tell 
>> __free_one_page() the correct migratetype. However, nothing will 
>> update the freepage's migratetype by set_freepage_migratetype(), 
>> because it would be a pointless waste of CPU cycles. The page however 
>> goes to the correct MIGRATE_ISOLATE list. (note that this is likely 
>> not the only way how freepage_migratetype can be perceived as incorrect)
>>
>> That means the code you are patching can really find the page where 
>> get_freepage_migratetype(page) will return MIGRATE_MOVABLE, i.e. != 
>> MIGRATE_ISOLATE will be true. But the move_freepages() call would be a 
>> no-op, as the page is already on the correct list and the accounting 
>> of freepages is correct.
>>
>> So my conclusion is that after your patch, the freepage accounting 
>> could actually get broken, not fixed. But I may be wrong. Hopefully 
>> Joonsoo can verify this :)
>>
>> If that's true, then the whole test you are patching should be 
>> dropped. Also we should make it clearer that 
>> get_freepage_migratetype() is only used for pages on pcplists (and 
>> even there it may differ from pageblock's migratetype and also from 
>> the pcplist the page is actually on, in cases of page stealing), as 
>> this is not the first confusion.
>> We should also drop the usage set_freepage_migratetype() from 
>> move_freepages() while at it.
>> Now the last usage of get_freepage_migratetype() outside of 
>> page_alloc.c is the page isolation code and I argue it's wrong. So 
>> after that is removed, we can actually also make the functions 
>> internal to page_alloc.c.
>>
>>> Signed-off-by: minkyung88.kim <minkyung88.kim@lge.com>
>>> ---
>>>   mm/page_isolation.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>> index 303c908..16cc172 100644
>>> --- a/mm/page_isolation.c
>>> +++ b/mm/page_isolation.c
>>> @@ -233,10 +233,14 @@ __test_page_isolated_in_pageblock(unsigned long 
>>> pfn, unsigned long end_pfn,
>>>                */
>>>               if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
>>>                   struct page *end_page;
>>> +                struct zone *zone = page_zone(page);
>>> +                int mt = get_freepage_migratetype(page);
>>> +                unsigned long nr_pages;
>>>
>>>                   end_page = page + (1 << page_order(page)) - 1;
>>> -                move_freepages(page_zone(page), page, end_page,
>>> +                nr_pages = move_freepages(zone, page, end_page,
>>>                           MIGRATE_ISOLATE);
>>> +                __mod_zone_freepage_state(zone, -nr_pages, mt);
>>>               }
>>>               pfn += 1 << page_order(page);
>>>           }
>>>
>>
>>
> 
> --
> 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>