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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]