[RFC 0/3] memory-hotplug: handle page race between allocation and isolation

[RFC 0/3] memory-hotplug: handle page race between allocation and isolation

[Minchan Kim]

Memory hotplug has a subtle race problem so this patchset fixes the problem
(Look at [3/3] for detail and please confirm the problem before review
other patches in this series.)

 [1/3] is just clean up and help for [2/3].
 [2/3] keeps the migratetype information to freed page's index field
       and [3/3] uses the information.
 [3/3] fixes the race problem with [2/3]'s information.

After applying [2/3], migratetype argument in __free_one_page
and free_one_page is redundant so we can remove it but I decide
to not touch them because it increases code size about 50 byte.

Minchan Kim (3):
  mm: use get_page_migratetype instead of page_private
  mm: remain migratetype in freed page
  memory-hotplug: bug fix race between isolation and allocation

 include/linux/mm.h  |   12 ++++++++++++
 mm/page_alloc.c     |   16 ++++++++++------
 mm/page_isolation.c |    7 +++++--
 3 files changed, 27 insertions(+), 8 deletions(-)

-- 
1.7.9.5

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

[RFC 1/3] mm: use get_page_migratetype instead of page_private

[Minchan Kim]

page allocator uses set_page_private and page_private for handling
migratetype when it frees page. Let's replace them with [set|get]
_page_migratetype to make it more clear.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/mm.h  |   10 ++++++++++
 mm/page_alloc.c     |   11 +++++++----
 mm/page_isolation.c |    2 +-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5c76634..86d61d6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -249,6 +249,16 @@ struct inode;
 #define page_private(page)		((page)->private)
 #define set_page_private(page, v)	((page)->private = (v))
 
+static inline void set_page_migratetype(struct page *page, int migratetype)
+{
+	set_page_private(page, migratetype);
+}
+
+static inline int get_page_migratetype(struct page *page)
+{
+	return page_private(page);
+}
+
 /*
  * FIXME: take this include out, include page-flags.h in
  * files which need it (119 of them)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 710d91c..103ba66 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -671,8 +671,10 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
 			/* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
-			__free_one_page(page, zone, 0, page_private(page));
-			trace_mm_page_pcpu_drain(page, 0, page_private(page));
+			__free_one_page(page, zone, 0,
+				get_page_migratetype(page));
+			trace_mm_page_pcpu_drain(page, 0,
+				get_page_migratetype(page));
 		} while (--to_free && --batch_free && !list_empty(list));
 	}
 	__mod_zone_page_state(zone, NR_FREE_PAGES, count);
@@ -731,6 +733,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	__count_vm_events(PGFREE, 1 << order);
 	free_one_page(page_zone(page), page, order,
 					get_pageblock_migratetype(page));
+
 	local_irq_restore(flags);
 }
 
@@ -1134,7 +1137,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 			if (!is_migrate_cma(mt) && mt != MIGRATE_ISOLATE)
 				mt = migratetype;
 		}
-		set_page_private(page, mt);
+		set_page_migratetype(page, mt);
 		list = &page->lru;
 	}
 	__mod_zone_page_state(zone, NR_FREE_PAGES, -(i << order));
@@ -1301,7 +1304,7 @@ void free_hot_cold_page(struct page *page, int cold)
 		return;
 
 	migratetype = get_pageblock_migratetype(page);
-	set_page_private(page, migratetype);
+	set_page_migratetype(page, migratetype);
 	local_irq_save(flags);
 	if (unlikely(wasMlocked))
 		free_page_mlock(page);
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 64abb33..acf65a7 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -199,7 +199,7 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
 		if (PageBuddy(page))
 			pfn += 1 << page_order(page);
 		else if (page_count(page) == 0 &&
-				page_private(page) == MIGRATE_ISOLATE)
+				get_page_migratetype(page) == MIGRATE_ISOLATE)
 			pfn += 1;
 		else
 			break;
-- 
1.7.9.5

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

[RFC 2/3] mm: remain migratetype in freed page

[Minchan Kim]

Page allocator doesn't keep migratetype information to page
when the page is freed. This patch remains the information
to freed page's index field which isn't used by free/alloc
preparing so it shouldn't change any behavir except below one.

This patch adds a new call site in __free_pages_ok so it might be
overhead a bit but it's for high order allocation.
So I believe damage isn't hurt.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/mm.h |    6 ++++--
 mm/page_alloc.c    |    7 ++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 86d61d6..8fd32da 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -251,12 +251,14 @@ struct inode;
 
 static inline void set_page_migratetype(struct page *page, int migratetype)
 {
-	set_page_private(page, migratetype);
+	VM_BUG_ON((unsigned int)migratetype >= MIGRATE_TYPES);
+	page->index = migratetype;
 }
 
 static inline int get_page_migratetype(struct page *page)
 {
-	return page_private(page);
+	VM_BUG_ON((unsigned int)page->index >= MIGRATE_TYPES);
+	return page->index;
 }
 
 /*
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 103ba66..32985dd 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -723,6 +723,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 {
 	unsigned long flags;
 	int wasMlocked = __TestClearPageMlocked(page);
+	int migratetype;
 
 	if (!free_pages_prepare(page, order))
 		return;
@@ -731,9 +732,9 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	if (unlikely(wasMlocked))
 		free_page_mlock(page);
 	__count_vm_events(PGFREE, 1 << order);
-	free_one_page(page_zone(page), page, order,
-					get_pageblock_migratetype(page));
-
+	migratetype = get_pageblock_migratetype(page);
+	set_page_migratetype(page, migratetype);
+	free_one_page(page_zone(page), page, order, migratetype);
 	local_irq_restore(flags);
 }
 
-- 
1.7.9.5

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

[RFC 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>
---
I found this problem during code review so please confirm it.
Kame?

 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)) {
 			pfn += 1 << page_order(page);
+			if (get_page_migratetype(page) != MIGRATE_ISOLATE)
+				break;
+		}
 		else if (page_count(page) == 0 &&
 				get_page_migratetype(page) == MIGRATE_ISOLATE)
 			pfn += 1;
-- 
1.7.9.5

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

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

[Bob Liu]

Hi Minchan,

On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> I found this problem during code review so please confirm it.
> Kame?
>
>  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)) {
>                         pfn += 1 << page_order(page);
> +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> +                               break;
> +               }

test_page_isolated() already have check
get_pageblock_migratetype(page) != MIGRATE_ISOLATE.

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


-- 
Regards,
--Bob

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

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

[Minchan Kim]

Hi Bob,

On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> Hi Minchan,
> 
> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> > I found this problem during code review so please confirm it.
> > Kame?
> >
> >  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)) {
> >                         pfn += 1 << page_order(page);
> > +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> > +                               break;
> > +               }
> 
> test_page_isolated() already have check
> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> 

That's why I send a patch.
As I describe in description, pageblock migration type of get_page_migratetype(page)
is inconsistent with free_list[migrationtype].
I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.

-- 
Kind regards,
Minchan Kim

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

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

[Bob Liu]

On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> Hi Bob,
>
> On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> Hi Minchan,
>>
>> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >
>> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> > ---
>> > I found this problem during code review so please confirm it.
>> > Kame?
>> >
>> >  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)) {
>> >                         pfn += 1 << page_order(page);
>> > +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> > +                               break;
>> > +               }
>>
>> test_page_isolated() already have check
>> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>>
>
> That's why I send a patch.
> As I describe in description, pageblock migration type of get_page_migratetype(page)
> is inconsistent with free_list[migrationtype].
> I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>

Sorry, I'm still not get the situation how this race happens.

set_pageblock_isolate
move_freepages_block
drain_all_pages

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

I think move_freepages_block() will call list_move() to move Page A to
free_list[MIGRATE_ISOLATE], so this case can't happen?

-- 
Thanks,
--Bob

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

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

[Minchan Kim]

On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> > Hi Bob,
> >
> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> Hi Minchan,
> >>
> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >
> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> > ---
> >> > I found this problem during code review so please confirm it.
> >> > Kame?
> >> >
> >> >  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)) {
> >> >                         pfn += 1 << page_order(page);
> >> > +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> > +                               break;
> >> > +               }
> >>
> >> test_page_isolated() already have check
> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >>
> >
> > That's why I send a patch.
> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> > is inconsistent with free_list[migrationtype].
> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >
> 
> Sorry, I'm still not get the situation how this race happens.
> 
> set_pageblock_isolate
> move_freepages_block
> drain_all_pages
> 
>                                 /* Page A could be in MIGRATE_MOVABLE
> of free_list. */
> 
> I think move_freepages_block() will call list_move() to move Page A to
> free_list[MIGRATE_ISOLATE], so this case can't happen?

move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.

> 
> -- 
> Thanks,
> --Bob
> 
> --
> 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

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

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

[Bob Liu]

On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
>> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
>> > Hi Bob,
>> >
>> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> >> Hi Minchan,
>> >>
>> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >> >
>> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> >> > ---
>> >> > I found this problem during code review so please confirm it.
>> >> > Kame?
>> >> >
>> >> >  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)) {
>> >> >                         pfn += 1 << page_order(page);
>> >> > +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> >> > +                               break;
>> >> > +               }
>> >>
>> >> test_page_isolated() already have check
>> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>> >>
>> >
>> > That's why I send a patch.
>> > As I describe in description, pageblock migration type of get_page_migratetype(page)
>> > is inconsistent with free_list[migrationtype].
>> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
>> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>> >
>>
>> Sorry, I'm still not get the situation how this race happens.
>>
>> set_pageblock_isolate
>> move_freepages_block
>> drain_all_pages
>>
>>                                 /* Page A could be in MIGRATE_MOVABLE
>> of free_list. */
>>
>> I think move_freepages_block() will call list_move() to move Page A to
>> free_list[MIGRATE_ISOLATE], so this case can't happen?
>
> move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>

Got it, then why not just drain pcp pages before move_freepages_block() ?

And I didn't see the effect by adding the check if
(get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
Since set_pageblock_isolate() have been called by CPU A, this check
will always false which cause CPU A still consider Page A isolated,
then PAGE A still can be allocated by CPU B from pcp.

-- 
Regards,
--Bob

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

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

[Minchan Kim]

On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> > Hi Bob,
> >> >
> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> >> Hi Minchan,
> >> >>
> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >> >
> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> >> > ---
> >> >> > I found this problem during code review so please confirm it.
> >> >> > Kame?
> >> >> >
> >> >> >  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)) {
> >> >> >                         pfn += 1 << page_order(page);
> >> >> > +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> >> > +                               break;
> >> >> > +               }
> >> >>
> >> >> test_page_isolated() already have check
> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >> >>
> >> >
> >> > That's why I send a patch.
> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> >> > is inconsistent with free_list[migrationtype].
> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >> >
> >>
> >> Sorry, I'm still not get the situation how this race happens.
> >>
> >> set_pageblock_isolate
> >> move_freepages_block
> >> drain_all_pages
> >>
> >>                                 /* Page A could be in MIGRATE_MOVABLE
> >> of free_list. */
> >>
> >> I think move_freepages_block() will call list_move() to move Page A to
> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
> >
> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
> >
> 
> Got it, then why not just drain pcp pages before move_freepages_block() ?

        CPU A                   CPU B

drain_all_pages
lock(zone->lock);
                                free_hot_cold_page
                                MIGRATE_MOVABLE = get_pageblock_migratetype(page);
                                list_add(&page->lru, &pcp->lists[migratetype])
set_pageblock_isolate
move_free_pages_block
unlock(zone->lock);

We can't make it atomic.

> 
> And I didn't see the effect by adding the check if
> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
> Since set_pageblock_isolate() have been called by CPU A, this check
> will always false which cause CPU A still consider Page A isolated,
> then PAGE A still can be allocated by CPU B from pcp.

Please don't confuse get_page_migratetype and get_pageblock_migratetype.
get_page_migratetype returns migratetype inforamtion of *page* which is
in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.

> 
> -- 
> Regards,
> --Bob
> 
> --
> 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

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

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

[Bob Liu]

On Wed, Jul 18, 2012 at 11:56 AM, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
>> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
>> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
>> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
>> >> > Hi Bob,
>> >> >
>> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> >> >> Hi Minchan,
>> >> >>
>> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >> >> >
>> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> >> >> > ---
>> >> >> > I found this problem during code review so please confirm it.
>> >> >> > Kame?
>> >> >> >
>> >> >> >  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)) {
>> >> >> >                         pfn += 1 << page_order(page);
>> >> >> > +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> >> >> > +                               break;
>> >> >> > +               }
>> >> >>
>> >> >> test_page_isolated() already have check
>> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>> >> >>
>> >> >
>> >> > That's why I send a patch.
>> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
>> >> > is inconsistent with free_list[migrationtype].
>> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
>> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>> >> >
>> >>
>> >> Sorry, I'm still not get the situation how this race happens.
>> >>
>> >> set_pageblock_isolate
>> >> move_freepages_block
>> >> drain_all_pages
>> >>
>> >>                                 /* Page A could be in MIGRATE_MOVABLE
>> >> of free_list. */
>> >>
>> >> I think move_freepages_block() will call list_move() to move Page A to
>> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
>> >
>> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>> >
>>
>> Got it, then why not just drain pcp pages before move_freepages_block() ?
>
>         CPU A                   CPU B
>
> drain_all_pages
> lock(zone->lock);
>                                 free_hot_cold_page
>                                 MIGRATE_MOVABLE = get_pageblock_migratetype(page);
>                                 list_add(&page->lru, &pcp->lists[migratetype])
> set_pageblock_isolate
> move_free_pages_block
> unlock(zone->lock);
>
> We can't make it atomic.
>
>>
>> And I didn't see the effect by adding the check if
>> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
>> Since set_pageblock_isolate() have been called by CPU A, this check
>> will always false which cause CPU A still consider Page A isolated,
>> then PAGE A still can be allocated by CPU B from pcp.
>
> Please don't confuse get_page_migratetype and get_pageblock_migratetype.
> get_page_migratetype returns migratetype inforamtion of *page* which is
> in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
>

Oh, yes i mixed them up.

Last question, it's better to break only before add pfn.
like:

-               if (PageBuddy(page))
+               if (PageBuddy(page)) {
+                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
+                               break;
                          pfn += 1 << page_order(page);
+               }

-- 
Regards,
--Bob

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

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

[Minchan Kim]

On Wed, Jul 18, 2012 at 01:29:47PM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 11:56 AM, Minchan Kim <minchan@kernel.org> wrote:
> > On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
> >> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> >> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> >> > Hi Bob,
> >> >> >
> >> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> >> >> Hi Minchan,
> >> >> >>
> >> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >> >> >
> >> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> >> >> > ---
> >> >> >> > I found this problem during code review so please confirm it.
> >> >> >> > Kame?
> >> >> >> >
> >> >> >> >  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)) {
> >> >> >> >                         pfn += 1 << page_order(page);
> >> >> >> > +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> >> >> > +                               break;
> >> >> >> > +               }
> >> >> >>
> >> >> >> test_page_isolated() already have check
> >> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >> >> >>
> >> >> >
> >> >> > That's why I send a patch.
> >> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> >> >> > is inconsistent with free_list[migrationtype].
> >> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> >> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >> >> >
> >> >>
> >> >> Sorry, I'm still not get the situation how this race happens.
> >> >>
> >> >> set_pageblock_isolate
> >> >> move_freepages_block
> >> >> drain_all_pages
> >> >>
> >> >>                                 /* Page A could be in MIGRATE_MOVABLE
> >> >> of free_list. */
> >> >>
> >> >> I think move_freepages_block() will call list_move() to move Page A to
> >> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
> >> >
> >> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
> >> >
> >>
> >> Got it, then why not just drain pcp pages before move_freepages_block() ?
> >
> >         CPU A                   CPU B
> >
> > drain_all_pages
> > lock(zone->lock);
> >                                 free_hot_cold_page
> >                                 MIGRATE_MOVABLE = get_pageblock_migratetype(page);
> >                                 list_add(&page->lru, &pcp->lists[migratetype])
> > set_pageblock_isolate
> > move_free_pages_block
> > unlock(zone->lock);
> >
> > We can't make it atomic.
> >
> >>
> >> And I didn't see the effect by adding the check if
> >> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
> >> Since set_pageblock_isolate() have been called by CPU A, this check
> >> will always false which cause CPU A still consider Page A isolated,
> >> then PAGE A still can be allocated by CPU B from pcp.
> >
> > Please don't confuse get_page_migratetype and get_pageblock_migratetype.
> > get_page_migratetype returns migratetype inforamtion of *page* which is
> > in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
> >
> 
> Oh, yes i mixed them up.
> 
> Last question, it's better to break only before add pfn.
> like:
> 
> -               if (PageBuddy(page))
> +               if (PageBuddy(page)) {
> +                       if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> +                               break;
>                           pfn += 1 << page_order(page);
> +               }

Sure thing!

-- 
Kind regards,
Minchan Kim

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