On 3.7.2015 9:15, "김민경/주임연구원/SW Platform(연)AOT팀
(minkyung88.kim@xxxxxxx)" 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 오후 6:52에 Vlastimil Babka 이(가) 쓴 글:
>> [+CC Joonsoo and Minchan]
>>
>> On 07/01/2015 03:17 AM, minkyung88.kim@xxxxxxx wrote:
>>> From: "minkyung88.kim" <minkyung88.kim@xxxxxxx>
>>>
>>> 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@xxxxxxx>
>>> ---
>>> 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@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
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>