As Vlastimil Babka expalin, this patch is useless and working not correctly.
Thank you for your review :)
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>