memory-hotplug: bug fix race between isolation and allocation

Quoted 18 lines · Minchan Kim

Like below, memory-hotplug makes race between page-isolation and page-allocation so it can hit BUG_ON in __offline_isolated_pages.

	CPU A					CPU B

start_isolate_page_range
set_migratetype_isolate
spin_lock_irqsave(zone->lock)

				free_hot_cold_page(Page A)
				/* without zone->lock */
				migratetype = get_pageblock_migratetype(Page A);
				/*
				 * Page could be moved into MIGRATE_MOVABLE
				 * of per_cpu_pages
				 */
				list_add_tail(&page->lru, &pcp->lists[migratetype]);

Quoted 3 lines

set_pageblock_isolate
move_freepages_block
drain_all_pages

Quoted 14 lines

				/* Page A could be in MIGRATE_MOVABLE of free_list. */

check_pages_isolated
__test_page_isolated_in_pageblock
/*
* We can't catch freed page which
* is free_list[MIGRATE_MOVABLE]
*/
if (PageBuddy(page A))

	pfn += 1 << page_order(page A);

				/* So, Page A could be allocated */

Quoted 7 lines

__offline_isolated_pages
/*
* BUG_ON hit or offline page
* which is used by someone
*/
BUG_ON(!PageBuddy(page A));

offline_page calling BUG_ON because someone allocated the page is ridiculous. I did not spot where that check is but it should be changed. The correct action is to retry the isolation.

Quoted 1 line

Signed-off-by Minchan Kim <[email protected]>

At no point in the changelog do you actually say what he patch does :/

Quoted 9 lines

---
 mm/page_isolation.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

mm/page_isolation.c +0 -0

index acf65a7..4699d1f 100644

mm/page_isolation.c __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn) · line 203

Quoted 9 lines

mm/page_isolation.c +4 -1

@@ -196,8 +196,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
 			continue;
 		}
 		page = pfn_to_page(pfn);
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+			if (get_page_migratetype(page) != MIGRATE_ISOLATE)
+				break;
 			pfn += 1 << page_order(page);
+		}

It is possible the page is moved to the MIGRATE_ISOLATE list between when the page was freed to the buddy allocator and this check was made. The page->index information is stale and the impact is that the hotplug operation fails when it could have succeeded. That said, I think it is a very unlikely race that will never happen in practice.

More importantly, the effect of this path is that EBUSY gets bubbled all the way up and the hotplug operations fails. This is fine but as the page is free at the time this problem is detected you also have the option of moving the PageBuddy page to the MIGRATE_ISOLATE list at this time if you take the zone lock. This will mean you need to change the name of test_pages_isolated() of course.

Quoted 6 lines

mm/page_isolation.c +0 -1

 		else if (page_count(page) == 0 &&
 				get_page_migratetype(page) == MIGRATE_ISOLATE)
 			pfn += 1;
--

1.7.9.5

Quoted 19 lines · Mel Gorman

Quoted 18 lines · Minchan Kim

Like below, memory-hotplug makes race between page-isolation and page-allocation so it can hit BUG_ON in __offline_isolated_pages.

	CPU A					CPU B

start_isolate_page_range
set_migratetype_isolate
spin_lock_irqsave(zone->lock)

				free_hot_cold_page(Page A)
				/* without zone->lock */
				migratetype = get_pageblock_migratetype(Page A);
				/*
				 * Page could be moved into MIGRATE_MOVABLE
				 * of per_cpu_pages
				 */
				list_add_tail(&page->lru, &pcp->lists[migratetype]);

Quoted 3 lines

Quoted 3 lines

set_pageblock_isolate
move_freepages_block
drain_all_pages

I think here is the problem you want to fix, it is not sure that pcp will be moved into MIGRATE_ISOLATE list. They may be moved into MIGRATE_MOVABLE list because page_private() maybe 2, it uses page_private() not get_pageblock_migratetype()

So when finish migrating pages, the free pages from pcp may be allocated again, and failed in check_pages_isolated().

drain_all_pages()

	drain_local_pages()
		drain_pages()
			free_pcppages_bulk()
				__free_one_page(page, zone, 0, page_private(page))

I reported this problem too. http://marc.info/?l=linux-mm&m=134555113706068&w=2
How about this change:
free_pcppages_bulk()
__free_one_page(page, zone, 0, get_pageblock_migratetype(page))