linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mel Gorman <mgorman@suse.de>
To: Minchan Kim <minchan@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
	Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>,
	Xishi Qiu <qiuxishi@huawei.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] memory-hotplug: bug fix race between isolation and allocation
Date: Wed, 5 Sep 2012 10:40:41 +0100	[thread overview]
Message-ID: <20120905094041.GF11266@suse.de> (raw)
In-Reply-To: <1346829962-31989-4-git-send-email-minchan@kernel.org>

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

  reply	other threads:[~2012-09-05  9:40 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-05  7:25 [PATCH 0/3] memory-hotplug: handle page race between allocation and isolation Minchan Kim
2012-09-05  7:26 ` [PATCH 1/3] mm: use get_page_migratetype instead of page_private Minchan Kim
2012-09-05  9:09   ` Mel Gorman
2012-09-06  2:17     ` Minchan Kim
2012-09-06  2:02   ` Kamezawa Hiroyuki
2012-09-06  2:19     ` Minchan Kim
2012-09-05  7:26 ` [PATCH 2/3] mm: remain migratetype in freed page Minchan Kim
2012-09-05  9:25   ` Mel Gorman
2012-09-06  2:28     ` Minchan Kim
2012-09-05  7:26 ` [PATCH 3/3] memory-hotplug: bug fix race between isolation and allocation Minchan Kim
2012-09-05  9:40   ` Mel Gorman [this message]
2012-09-06  4:49     ` Minchan Kim
2012-09-06  9:24       ` Mel Gorman
2012-09-06 23:32         ` Minchan Kim
2012-09-07  6:26   ` jencce zhou
2012-09-06  2:35 qiuxishi
2012-09-06  2:59 ` Minchan Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120905094041.GF11266@suse.de \
    --to=mgorman@suse.de \
    --cc=akpm@linux-foundation.org \
    --cc=isimatu.yasuaki@jp.fujitsu.com \
    --cc=kamezawa.hiroyu@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=minchan@kernel.org \
    --cc=qiuxishi@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).