[+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>