Re: [PATCH 3/3] memory-hotplug: bug fix race between isolation andallocation

[Mel Gorman]

On Wed, Sep 05, 2012 at 04:26:02PM +0900, Minchan Kim wrote:
> 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]);
> 
> set_pageblock_isolate
> move_freepages_block
> drain_all_pages
> 
> 				/* 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 */
> 
> __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.

> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>

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

> ---
>  mm/page_isolation.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index acf65a7..4699d1f 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -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.

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

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/