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

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

[qiuxishi]

On 2012/9/5 17:40, Mel Gorman wrote:

> 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

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))

Thanks
Xishi Qiu

>>
>> 				/* 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@kernel.org>
>
> 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
>>
>

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

[Minchan Kim]

Hello Xishi,

On Thu, Sep 06, 2012 at 10:35:39AM +0800, qiuxishi wrote:
> On 2012/9/5 17:40, Mel Gorman wrote:
> 
> > 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
> 
> 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))

I already explained why it was not good solution.
Again, here it goes from my previous reply.

"
Anyway, I don't like your approach which I already considered because it hurts hotpath
while the race is really unlikely. Get_pageblock_migratetype is never trivial.
We should avoid the overhead in hotpath and move into memory-hotplug itself.
Do you see my patch in https://patchwork.kernel.org/patch/1225081/ ?
"

-- 
Kind regards,
Minchan Kim

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

[jencce zhou]

2012/9/5 Minchan Kim <minchan@kernel.org>:
> 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
here
> move_freepages_block
> drain_all_pages
>
>                                 /* Page A could be in MIGRATE_MOVABLE of free_list. */
             why ?  should it has been moved to MIGRATE_ISOLATE 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));
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  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);
> +               }
>                 else if (page_count(page) == 0 &&
>                                 get_page_migratetype(page) == MIGRATE_ISOLATE)
>                         pfn += 1;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Minchan Kim]

On Thu, Sep 06, 2012 at 10:24:24AM +0100, Mel Gorman wrote:
> On Thu, Sep 06, 2012 at 01:49:03PM +0900, Minchan Kim wrote:
> > > > __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.
> > 
> > It is where __offline_isolated_pges.
> > 
> > ..
> >         while (pfn < end_pfn) {
> >                 if (!pfn_valid(pfn)) {
> >                         pfn++;
> >                         continue;
> >                 }    
> >                 page = pfn_to_page(pfn);
> >                 BUG_ON(page_count(page));
> >                 BUG_ON(!PageBuddy(page)); <---- HERE
> >                 order = page_order(page);
> > ...
> > 
> > Comment of offline_isolated_pages says following as.
> > 
> >         We cannot do rollback at this point
> > 
> > So if the comment is true, BUG_ON does make sense to me.
> 
> It's massive overkill. I see no reason why it cannot return EBUSY all the
> way back up to offline_pages() and retry with the migration step.  It would
> both remove that BUG_ON and improve reliability of memory hot-remove.
> 
> > But I don't see why we can't retry it as I look thorugh code.
> > Anyway, It's another story which isn't related to this patch.
> > 
> 
> True.
> 
> > > 
> > > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > > 
> > > At no point in the changelog do you actually say what he patch does :/
> > 
> > Argh, I will do.
> > 
> > > 
> > > > ---
> > > >  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.
> > 
> > I understand you mean move_freepages which I have missed. Right?
> 
> Yes.
> 
> > Then, I will fix it, too.
> > 
> > > 
> > > 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.
> > 
> > Sorry, I can't get your point. Could you elaborate it more?
> 
> You detect a PageBuddy page but it's on the wrong list. Instead of returning
> and failing memory-hotremove, move the free page to the correct list at
> the time it is detected.

Good idea.

> 
> > Is it related to this patch?
> 
> No, it's not important and was a suggestion on how it could be made
> better. However, retrying hot-remove would be even better again. I'm not
> suggesting it be done as part of this series.

Mel, Thanks for your review.

-- 
Kind regards,
Minchan Kim

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

[Mel Gorman]

On Thu, Sep 06, 2012 at 01:49:03PM +0900, Minchan Kim wrote:
> > > __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.
> 
> It is where __offline_isolated_pges.
> 
> ..
>         while (pfn < end_pfn) {
>                 if (!pfn_valid(pfn)) {
>                         pfn++;
>                         continue;
>                 }    
>                 page = pfn_to_page(pfn);
>                 BUG_ON(page_count(page));
>                 BUG_ON(!PageBuddy(page)); <---- HERE
>                 order = page_order(page);
> ...
> 
> Comment of offline_isolated_pages says following as.
> 
>         We cannot do rollback at this point
> 
> So if the comment is true, BUG_ON does make sense to me.

It's massive overkill. I see no reason why it cannot return EBUSY all the
way back up to offline_pages() and retry with the migration step.  It would
both remove that BUG_ON and improve reliability of memory hot-remove.

> But I don't see why we can't retry it as I look thorugh code.
> Anyway, It's another story which isn't related to this patch.
> 

True.

> > 
> > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > 
> > At no point in the changelog do you actually say what he patch does :/
> 
> Argh, I will do.
> 
> > 
> > > ---
> > >  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.
> 
> I understand you mean move_freepages which I have missed. Right?

Yes.

> Then, I will fix it, too.
> 
> > 
> > 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.
> 
> Sorry, I can't get your point. Could you elaborate it more?

You detect a PageBuddy page but it's on the wrong list. Instead of returning
and failing memory-hotremove, move the free page to the correct list at
the time it is detected.

> Is it related to this patch?

No, it's not important and was a suggestion on how it could be made
better. However, retrying hot-remove would be even better again. I'm not
suggesting it be done as part of this series.

-- 
Mel Gorman
SUSE Labs

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

[Minchan Kim]

On Wed, Sep 05, 2012 at 10:40:41AM +0100, Mel Gorman wrote:
> 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.

It is where __offline_isolated_pges.

..
        while (pfn < end_pfn) {
                if (!pfn_valid(pfn)) {
                        pfn++;
                        continue;
                }    
                page = pfn_to_page(pfn);
                BUG_ON(page_count(page));
                BUG_ON(!PageBuddy(page)); <---- HERE
                order = page_order(page);
...

Comment of offline_isolated_pages says following as.

        We cannot do rollback at this point

So if the comment is true, BUG_ON does make sense to me.
But I don't see why we can't retry it as I look thorugh code.
Anyway, It's another story which isn't related to this patch.

> 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> At no point in the changelog do you actually say what he patch does :/

Argh, I will do.

> 
> > ---
> >  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.

I understand you mean move_freepages which I have missed. Right?
Then, I will fix it, too.

> 
> 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.

Sorry, I can't get your point. Could you elaborate it more?
Is it related to this patch?


> 
> >  		else if (page_count(page) == 0 &&
> >  				get_page_migratetype(page) == MIGRATE_ISOLATE)
> >  			pfn += 1;
> > -- 
> > 1.7.9.5
> > 
> 
> -- 
> Mel Gorman
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

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

[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@kernel.org>

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

[PATCH 3/3] memory-hotplug: bug fix race between isolation and allocation

[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]);

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));

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 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);
+		}
 		else if (page_count(page) == 0 &&
 				get_page_migratetype(page) == MIGRATE_ISOLATE)
 			pfn += 1;
-- 
1.7.9.5