Re: [RFC PATCH 0/3 V1] mm: add new migrate type and online_movable for hotplug

Re: [RFC PATCH 0/3 V1] mm: add new migrate type and online_movable for hotplug

[Lai Jiangshan]

On 07/04/2012 03:35 PM, Minchan Kim wrote:
> Hello,
> 
> I am not sure when I can review this series by urgent other works.
> At a glance, it seems to attract me.
> But unfortunately, when I read description in cover-letter, I can't
> find "What's the problem?".
> If you provide that, it could help too many your Ccing people who can
> judge  "whether I dive into code or not"

This patchset adds a stable-movable-migrate-type for memory-management,
It is used for anti-fragmentation(hugepage, big-order alloction...) and
hot-removal-of-memory(virtualization, power-conserve, move memory between systems).
it likes ZONE_MOVABLE, but it is more elastic.

Beside it, it fixes some code of CMA.

Thanks,
Lai


> 
> Thanks!
> 
> Side-Note: What's the "--" of email addresses?

Wrong script, I will resent it.

> 
> On Wed, Jul 4, 2012 at 4:26 PM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
>> The 1st patch fixes the allocation of CMA and prepares for movable-like types.
>>
>> The 2nd patch add a new migrate type which stands for the movable types which
>> pages will not be changed to the other type.
>>
>> I chose the name MIGRATE_HOTREMOVE from MIGRATE_HOTREMOVE
>> and MIGRATE_MOVABLE_STABLE, it just because the first usecase of this new type
>> is for hotremove.
>>
>> The 3th path introduces online_movable. When a memoryblock is onlined
>> by "online_movable", the kernel will not have directly reference to the page
>> of the memoryblock, thus we can remove that memory any time when needed.
>>
>> Different from ZONE_MOVABLE: it can be used for any given memroyblock.
>>
>> Lai Jiangshan (3):
>>   use __rmqueue_smallest when borrow memory from MIGRATE_CMA
>>   add MIGRATE_HOTREMOVE type
>>   add online_movable
>>
>>  arch/tile/mm/init.c            |    2 +-
>>  drivers/acpi/acpi_memhotplug.c |    3 +-
>>  drivers/base/memory.c          |   24 +++++++----
>>  include/linux/memory.h         |    1 +
>>  include/linux/memory_hotplug.h |    4 +-
>>  include/linux/mmzone.h         |   37 +++++++++++++++++
>>  include/linux/page-isolation.h |    2 +-
>>  mm/compaction.c                |    6 +-
>>  mm/memory-failure.c            |    8 +++-
>>  mm/memory_hotplug.c            |   36 +++++++++++++---
>>  mm/page_alloc.c                |   86 ++++++++++++++++-----------------------
>>  mm/vmstat.c                    |    3 +
>>  12 files changed, 136 insertions(+), 76 deletions(-)
>>
>> --
>> 1.7.4.4
>>
> 
> 
> 

--
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 PATCH 0/3 V1] mm: add new migrate type and online_movable for hotplug

[Lai Jiangshan]

On 07/04/2012 04:23 PM, Lai Jiangshan wrote:
> On 07/04/2012 03:35 PM, Minchan Kim wrote:
>> Hello,
>>
>> I am not sure when I can review this series by urgent other works.
>> At a glance, it seems to attract me.

Patches are resent with updated cover-letter and correct cc list.

Thank you very much.
Lai


>> But unfortunately, when I read description in cover-letter, I can't
>> find "What's the problem?".
>> If you provide that, it could help too many your Ccing people who can
>> judge  "whether I dive into code or not"
> 
> This patchset adds a stable-movable-migrate-type for memory-management,
> It is used for anti-fragmentation(hugepage, big-order alloction...) and
> hot-removal-of-memory(virtualization, power-conserve, move memory between systems).
> it likes ZONE_MOVABLE, but it is more elastic.
> 
> Beside it, it fixes some code of CMA.
> 
> Thanks,
> Lai
> 

--
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 PATCH 1/3 V1] mm, page_alloc: use __rmqueue_smallest when borrow memory from MIGRATE_CMA

[Mel Gorman]

On Wed, Jul 04, 2012 at 03:26:16PM +0800, Lai Jiangshan wrote:
> The pages of MIGRATE_CMA can't not be changed to the other type,
> nor be moved to the other free list. 
> 
> ==>
> So when we use __rmqueue_fallback() to borrow memory from MIGRATE_CMA,
> one of the highest order page is borrowed and it is split.
> But the free pages resulted by splitting can NOT
> be moved to MIGRATE_MOVABLE.
> 
> ==>
> So in the next time of allocation, we NEED to borrow again,
> another one of the highest order page is borrowed from CMA and it is split.
> and results some other new split free pages.
> 

Then special case __rmqueue_fallback() to move pages stolen from
MIGRATE_CMA to the MIGRATE_MOVABLE lists but do not change the pageblock
type.

> ==>
> So when __rmqueue_fallback() borrows (highest order)memory from MIGRATE_CMA,
> it introduces fragments at the same time and may waste tlb(only one page is used in
> a pageblock).
> 
> Conclusion:
> We should borrows the smallest order memory from MIGRATE_CMA in such case
> 

That's excessive and unnecessary special casing. Just move the
MIGRATE_CMA pages to the MIGRATE_MOVABLE lists.

> Result(good):
> 1) use __rmqueue_smallest when borrow memory from MIGRATE_CMA
> 2) __rmqueue_fallback() don't handle CMA, it becomes much simpler
> Result(bad):
> __rmqueue_smallest() can't not be inlined to avoid function call overhead.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  include/linux/mmzone.h |    1 +
>  mm/page_alloc.c        |   63 ++++++++++++++++--------------------------------
>  2 files changed, 22 insertions(+), 42 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index bf3404e..979c333 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -40,6 +40,7 @@ enum {
>  	MIGRATE_RECLAIMABLE,
>  	MIGRATE_MOVABLE,
>  	MIGRATE_PCPTYPES,	/* the number of types on the pcp lists */
> +	MIGRATE_PRIME_TYPES = MIGRATE_PCPTYPES,

No explanation why this new name is necessary.

>  	MIGRATE_RESERVE = MIGRATE_PCPTYPES,
>  #ifdef CONFIG_CMA
>  	/*
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 476ae3e..efc327f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -893,17 +893,10 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
>   * This array describes the order lists are fallen back to when
>   * the free lists for the desirable migrate type are depleted
>   */
> -static int fallbacks[MIGRATE_TYPES][4] = {
> -	[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE,     MIGRATE_RESERVE },
> -	[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE,     MIGRATE_RESERVE },
> -#ifdef CONFIG_CMA
> -	[MIGRATE_MOVABLE]     = { MIGRATE_CMA,         MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
> -	[MIGRATE_CMA]         = { MIGRATE_RESERVE }, /* Never used */
> -#else
> -	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
> -#endif
> -	[MIGRATE_RESERVE]     = { MIGRATE_RESERVE }, /* Never used */
> -	[MIGRATE_ISOLATE]     = { MIGRATE_RESERVE }, /* Never used */
> +static int fallbacks[MIGRATE_PRIME_TYPES][2] = {
> +	[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE   },
> +	[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE   },
> +	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE },
>  };
>  

And it's completely unclear why it was necessary to rip out the existing
fallback lists. It reworks how fallback lists work for no clear benefit.

>  /*
> @@ -995,16 +988,15 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>  	struct page *page;
>  	int migratetype, i;
>  
> +	if (WARN_ON_ONCE(start_migratetype >= MIGRATE_PRIME_TYPES))
> +		start_migratetype = MIGRATE_UNMOVABLE;
> +

This should be completely unnecessary. If this warning is hit, the
callers are severely broken.

>  	/* Find the largest possible block of pages in the other list */
>  	for (current_order = MAX_ORDER-1; current_order >= order;
>  						--current_order) {
> -		for (i = 0;; i++) {
> +		for (i = 0; i < ARRAY_SIZE(fallbacks[0]); i++) {
>  			migratetype = fallbacks[start_migratetype][i];
>  
> -			/* MIGRATE_RESERVE handled later if necessary */
> -			if (migratetype == MIGRATE_RESERVE)
> -				break;
> -
>  			area = &(zone->free_area[current_order]);
>  			if (list_empty(&area->free_list[migratetype]))
>  				continue;
> @@ -1018,17 +1010,10 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>  			 * pages to the preferred allocation list. If falling
>  			 * back for a reclaimable kernel allocation, be more
>  			 * aggressive about taking ownership of free pages
> -			 *
> -			 * On the other hand, never change migration
> -			 * type of MIGRATE_CMA pageblocks nor move CMA
> -			 * pages on different free lists. We don't
> -			 * want unmovable pages to be allocated from
> -			 * MIGRATE_CMA areas.
>  			 */
> -			if (!is_migrate_cma(migratetype) &&
> -			    (unlikely(current_order >= pageblock_order / 2) ||
> -			     start_migratetype == MIGRATE_RECLAIMABLE ||
> -			     page_group_by_mobility_disabled)) {
> +			if (unlikely(current_order >= pageblock_order / 2) ||
> +			    start_migratetype == MIGRATE_RECLAIMABLE ||
> +			    page_group_by_mobility_disabled) {
>  				int pages;
>  				pages = move_freepages_block(zone, page,
>  								start_migratetype);
> @@ -1047,14 +1032,12 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>  			rmv_page_order(page);
>  
>  			/* Take ownership for orders >= pageblock_order */
> -			if (current_order >= pageblock_order &&
> -			    !is_migrate_cma(migratetype))
> +			if (current_order >= pageblock_order)
>  				change_pageblock_range(page, current_order,
>  							start_migratetype);
>  
>  			expand(zone, page, order, current_order, area,
> -			       is_migrate_cma(migratetype)
> -			     ? migratetype : start_migratetype);
> +			       start_migratetype);
>  
>  			trace_mm_page_alloc_extfrag(page, order, current_order,
>  				start_migratetype, migratetype);
> @@ -1075,22 +1058,18 @@ static struct page *__rmqueue(struct zone *zone, unsigned int order,
>  {
>  	struct page *page;
>  
> -retry_reserve:
>  	page = __rmqueue_smallest(zone, order, migratetype);
>  
> -	if (unlikely(!page) && migratetype != MIGRATE_RESERVE) {
> +#ifdef CONFIG_CMA
> +	if (unlikely(!page) && migratetype == MIGRATE_MOVABLE)
> +		page = __rmqueue_smallest(zone, order, MIGRATE_CMA);
> +#endif
> +
> +	if (unlikely(!page))
>  		page = __rmqueue_fallback(zone, order, migratetype);
>  
> -		/*
> -		 * Use MIGRATE_RESERVE rather than fail an allocation. goto
> -		 * is used because __rmqueue_smallest is an inline function
> -		 * and we want just one call site
> -		 */
> -		if (!page) {
> -			migratetype = MIGRATE_RESERVE;
> -			goto retry_reserve;
> -		}
> -	}
> +	if (unlikely(!page))
> +		page = __rmqueue_smallest(zone, order, MIGRATE_RESERVE);
>  
>  	trace_mm_page_alloc_zone_locked(page, order, migratetype);
>  	return page;

I didn't read this closely. It seems way more complex than necessary to
solve the problem described in the changelog. All you should need is to
special case that a __GFP_MOVABLE allocation using MIGRATE_CMA should
place the unused buddies on the MIGRATE_MOVABLE lists.

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

Re: [RFC PATCH 1/3 V1] mm, page_alloc: use __rmqueue_smallest when borrow memory from MIGRATE_CMA

[Lai Jiangshan]

On 07/04/2012 06:17 PM, Mel Gorman wrote:
> On Wed, Jul 04, 2012 at 03:26:16PM +0800, Lai Jiangshan wrote:
>> The pages of MIGRATE_CMA can't not be changed to the other type,
>> nor be moved to the other free list. 
>>
>> ==>
>> So when we use __rmqueue_fallback() to borrow memory from MIGRATE_CMA,
>> one of the highest order page is borrowed and it is split.
>> But the free pages resulted by splitting can NOT
>> be moved to MIGRATE_MOVABLE.
>>
>> ==>
>> So in the next time of allocation, we NEED to borrow again,
>> another one of the highest order page is borrowed from CMA and it is split.
>> and results some other new split free pages.
>>
> 
> Then special case __rmqueue_fallback() to move pages stolen from
> MIGRATE_CMA to the MIGRATE_MOVABLE lists but do not change the pageblock
> type.

Because unmovable-page-requirement can allocate page from
MIGRATE_MOVABLE free list. So We can not move MIGRATE_CMA pages
to the MIGRATE_MOVABLE free list.

See here:

MOVABLE list is empty
UNMOVABLE list is empty
movable-page-requirement
	borrow from CMA list
	split it, others are put into UNMOVABLE list
unmovable-page-requiremnt
	borrow from UNMOVABLE list
	NOW, it is BUG, we use CMA pages for unmovable usage.



> 
>> ==>
>> So when __rmqueue_fallback() borrows (highest order)memory from MIGRATE_CMA,
>> it introduces fragments at the same time and may waste tlb(only one page is used in
>> a pageblock).
>>
>> Conclusion:
>> We should borrows the smallest order memory from MIGRATE_CMA in such case
>>
> 
> That's excessive and unnecessary special casing. Just move the
> MIGRATE_CMA pages to the MIGRATE_MOVABLE lists.

...

> 
>> Result(good):
>> 1) use __rmqueue_smallest when borrow memory from MIGRATE_CMA
>> 2) __rmqueue_fallback() don't handle CMA, it becomes much simpler
>> Result(bad):
>> __rmqueue_smallest() can't not be inlined to avoid function call overhead.
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>>  include/linux/mmzone.h |    1 +
>>  mm/page_alloc.c        |   63 ++++++++++++++++--------------------------------
>>  2 files changed, 22 insertions(+), 42 deletions(-)
>>
>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>> index bf3404e..979c333 100644
>> --- a/include/linux/mmzone.h
>> +++ b/include/linux/mmzone.h
>> @@ -40,6 +40,7 @@ enum {
>>  	MIGRATE_RECLAIMABLE,
>>  	MIGRATE_MOVABLE,
>>  	MIGRATE_PCPTYPES,	/* the number of types on the pcp lists */
>> +	MIGRATE_PRIME_TYPES = MIGRATE_PCPTYPES,
> 
> No explanation why this new name is necessary.

These three types are the basic types.
reusing the name MIGRATE_PCPTYPES in fallback array makes confusing.


> 
>>  	MIGRATE_RESERVE = MIGRATE_PCPTYPES,
>>  #ifdef CONFIG_CMA
>>  	/*
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 476ae3e..efc327f 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -893,17 +893,10 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
>>   * This array describes the order lists are fallen back to when
>>   * the free lists for the desirable migrate type are depleted
>>   */
>> -static int fallbacks[MIGRATE_TYPES][4] = {
>> -	[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE,     MIGRATE_RESERVE },
>> -	[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE,     MIGRATE_RESERVE },
>> -#ifdef CONFIG_CMA
>> -	[MIGRATE_MOVABLE]     = { MIGRATE_CMA,         MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
>> -	[MIGRATE_CMA]         = { MIGRATE_RESERVE }, /* Never used */
>> -#else
>> -	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
>> -#endif
>> -	[MIGRATE_RESERVE]     = { MIGRATE_RESERVE }, /* Never used */
>> -	[MIGRATE_ISOLATE]     = { MIGRATE_RESERVE }, /* Never used */
>> +static int fallbacks[MIGRATE_PRIME_TYPES][2] = {
>> +	[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE   },
>> +	[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE   },
>> +	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE },
>>  };
>>  
> 
> And it's completely unclear why it was necessary to rip out the existing
> fallback lists. It reworks how fallback lists work for no clear benefit.
> 
>>  /*
>> @@ -995,16 +988,15 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>>  	struct page *page;
>>  	int migratetype, i;
>>  
>> +	if (WARN_ON_ONCE(start_migratetype >= MIGRATE_PRIME_TYPES))
>> +		start_migratetype = MIGRATE_UNMOVABLE;
>> +
> 
> This should be completely unnecessary. If this warning is hit, the
> callers are severely broken.

will be removed.

> 
>>  	/* Find the largest possible block of pages in the other list */
>>  	for (current_order = MAX_ORDER-1; current_order >= order;
>>  						--current_order) {
>> -		for (i = 0;; i++) {
>> +		for (i = 0; i < ARRAY_SIZE(fallbacks[0]); i++) {
>>  			migratetype = fallbacks[start_migratetype][i];
>>  
>> -			/* MIGRATE_RESERVE handled later if necessary */
>> -			if (migratetype == MIGRATE_RESERVE)
>> -				break;
>> -
>>  			area = &(zone->free_area[current_order]);
>>  			if (list_empty(&area->free_list[migratetype]))
>>  				continue;
>> @@ -1018,17 +1010,10 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>>  			 * pages to the preferred allocation list. If falling
>>  			 * back for a reclaimable kernel allocation, be more
>>  			 * aggressive about taking ownership of free pages
>> -			 *
>> -			 * On the other hand, never change migration
>> -			 * type of MIGRATE_CMA pageblocks nor move CMA
>> -			 * pages on different free lists. We don't
>> -			 * want unmovable pages to be allocated from
>> -			 * MIGRATE_CMA areas.
>>  			 */
>> -			if (!is_migrate_cma(migratetype) &&
>> -			    (unlikely(current_order >= pageblock_order / 2) ||
>> -			     start_migratetype == MIGRATE_RECLAIMABLE ||
>> -			     page_group_by_mobility_disabled)) {
>> +			if (unlikely(current_order >= pageblock_order / 2) ||
>> +			    start_migratetype == MIGRATE_RECLAIMABLE ||
>> +			    page_group_by_mobility_disabled) {
>>  				int pages;
>>  				pages = move_freepages_block(zone, page,
>>  								start_migratetype);
>> @@ -1047,14 +1032,12 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>>  			rmv_page_order(page);
>>  
>>  			/* Take ownership for orders >= pageblock_order */
>> -			if (current_order >= pageblock_order &&
>> -			    !is_migrate_cma(migratetype))
>> +			if (current_order >= pageblock_order)
>>  				change_pageblock_range(page, current_order,
>>  							start_migratetype);
>>  
>>  			expand(zone, page, order, current_order, area,
>> -			       is_migrate_cma(migratetype)
>> -			     ? migratetype : start_migratetype);
>> +			       start_migratetype);
>>  
>>  			trace_mm_page_alloc_extfrag(page, order, current_order,
>>  				start_migratetype, migratetype);
>> @@ -1075,22 +1058,18 @@ static struct page *__rmqueue(struct zone *zone, unsigned int order,
>>  {
>>  	struct page *page;
>>  
>> -retry_reserve:
>>  	page = __rmqueue_smallest(zone, order, migratetype);
>>  
>> -	if (unlikely(!page) && migratetype != MIGRATE_RESERVE) {
>> +#ifdef CONFIG_CMA
>> +	if (unlikely(!page) && migratetype == MIGRATE_MOVABLE)
>> +		page = __rmqueue_smallest(zone, order, MIGRATE_CMA);
>> +#endif
>> +
>> +	if (unlikely(!page))
>>  		page = __rmqueue_fallback(zone, order, migratetype);
>>  
>> -		/*
>> -		 * Use MIGRATE_RESERVE rather than fail an allocation. goto
>> -		 * is used because __rmqueue_smallest is an inline function
>> -		 * and we want just one call site
>> -		 */
>> -		if (!page) {
>> -			migratetype = MIGRATE_RESERVE;
>> -			goto retry_reserve;
>> -		}
>> -	}
>> +	if (unlikely(!page))
>> +		page = __rmqueue_smallest(zone, order, MIGRATE_RESERVE);
>>  
>>  	trace_mm_page_alloc_zone_locked(page, order, migratetype);
>>  	return page;
> 
> I didn't read this closely. It seems way more complex than necessary to
> solve the problem described in the changelog. All you should need is to
> special case that a __GFP_MOVABLE allocation using MIGRATE_CMA should
> place the unused buddies on the MIGRATE_MOVABLE lists.
> 

Re: [RFC PATCH 1/3 V1] mm, page_alloc: use __rmqueue_smallest when borrow memory from MIGRATE_CMA

[Mel Gorman]

On Wed, Jul 04, 2012 at 06:43:47PM +0800, Lai Jiangshan wrote:
> On 07/04/2012 06:17 PM, Mel Gorman wrote:
> > On Wed, Jul 04, 2012 at 03:26:16PM +0800, Lai Jiangshan wrote:
> >> The pages of MIGRATE_CMA can't not be changed to the other type,
> >> nor be moved to the other free list. 
> >>
> >> ==>
> >> So when we use __rmqueue_fallback() to borrow memory from MIGRATE_CMA,
> >> one of the highest order page is borrowed and it is split.
> >> But the free pages resulted by splitting can NOT
> >> be moved to MIGRATE_MOVABLE.
> >>
> >> ==>
> >> So in the next time of allocation, we NEED to borrow again,
> >> another one of the highest order page is borrowed from CMA and it is split.
> >> and results some other new split free pages.
> >>
> > 
> > Then special case __rmqueue_fallback() to move pages stolen from
> > MIGRATE_CMA to the MIGRATE_MOVABLE lists but do not change the pageblock
> > type.
> 
> Because unmovable-page-requirement can allocate page from
> MIGRATE_MOVABLE free list. So We can not move MIGRATE_CMA pages
> to the MIGRATE_MOVABLE free list.
> 

Ok, good point.

> See here:
> 
> MOVABLE list is empty
> UNMOVABLE list is empty
> movable-page-requirement
> 	borrow from CMA list
> 	split it, others are put into UNMOVABLE list
> unmovable-page-requiremnt
> 	borrow from UNMOVABLE list
> 	NOW, it is BUG, we use CMA pages for unmovable usage.
> 

The patch still looks unnecessarily complex for what you are trying to
achieve and as a result I'm not reviewing it as carefully as I should.
It looks like the entire patch boiled down to this hunk here

+#ifdef CONFIG_CMA
+	if (unlikely(!page) && migratetype == MIGRATE_MOVABLE)
+		page = __rmqueue_smallest(zone, order, MIGRATE_CMA);
+#endif
+

With that in place, this would would need to change from

[MIGRATE_MOVABLE]     = { MIGRATE_CMA, MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },

to

[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },

because the fallback is already being handled as a special case. Leave
the other fallback logic as it is.

This is not tested at all and is only meant to illustrate why I think
your patch looks excessively complex for what you are trying to
achieve.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4beb7ae..0063e93 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -895,11 +895,9 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
 static int fallbacks[MIGRATE_TYPES][4] = {
 	[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE,     MIGRATE_RESERVE },
 	[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE,     MIGRATE_RESERVE },
+	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
 #ifdef CONFIG_CMA
-	[MIGRATE_MOVABLE]     = { MIGRATE_CMA,         MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
 	[MIGRATE_CMA]         = { MIGRATE_RESERVE }, /* Never used */
-#else
-	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
 #endif
 	[MIGRATE_RESERVE]     = { MIGRATE_RESERVE }, /* Never used */
 	[MIGRATE_ISOLATE]     = { MIGRATE_RESERVE }, /* Never used */
@@ -1076,6 +1074,20 @@ static struct page *__rmqueue(struct zone *zone, unsigned int order,
 
 retry_reserve:
 	page = __rmqueue_smallest(zone, order, migratetype);
+#ifdef CONFIG_CMA
+	if (!unlikely(!page) && migratetype == MIGRATE_MOVABLE) {
+
+		/*
+		 * CMA is a special case where we want to use
+		 * the smallest available page instead of splitting
+		 * the largest chunks. We still must avoid the pages
+		 * moving to MIGRATE_MOVABLE where they might be
+		 * used for UNRECLAIMABLE or UNMOVABLE allocations
+		 */
+		migratetype = MIGRATE_CMA;
+		goto retry_reserve;
+	}
+#endif /* CONFIG_CMA */
 
 	if (unlikely(!page) && migratetype != MIGRATE_RESERVE) {
 		page = __rmqueue_fallback(zone, order, migratetype);

Re: [RFC PATCH 1/3 V1] mm, page_alloc: use __rmqueue_smallest when borrow memory from MIGRATE_CMA

[Lai Jiangshan]

On 07/04/2012 07:19 PM, Mel Gorman wrote:
> On Wed, Jul 04, 2012 at 06:43:47PM +0800, Lai Jiangshan wrote:
>> On 07/04/2012 06:17 PM, Mel Gorman wrote:
>>> On Wed, Jul 04, 2012 at 03:26:16PM +0800, Lai Jiangshan wrote:
>>>> The pages of MIGRATE_CMA can't not be changed to the other type,
>>>> nor be moved to the other free list. 
>>>>
>>>> ==>
>>>> So when we use __rmqueue_fallback() to borrow memory from MIGRATE_CMA,
>>>> one of the highest order page is borrowed and it is split.
>>>> But the free pages resulted by splitting can NOT
>>>> be moved to MIGRATE_MOVABLE.
>>>>
>>>> ==>
>>>> So in the next time of allocation, we NEED to borrow again,
>>>> another one of the highest order page is borrowed from CMA and it is split.
>>>> and results some other new split free pages.
>>>>
>>>
>>> Then special case __rmqueue_fallback() to move pages stolen from
>>> MIGRATE_CMA to the MIGRATE_MOVABLE lists but do not change the pageblock
>>> type.
>>
>> Because unmovable-page-requirement can allocate page from
>> MIGRATE_MOVABLE free list. So We can not move MIGRATE_CMA pages
>> to the MIGRATE_MOVABLE free list.
>>
> 
> Ok, good point.
> 
>> See here:
>>
>> MOVABLE list is empty
>> UNMOVABLE list is empty
>> movable-page-requirement
>> 	borrow from CMA list
>> 	split it, others are put into UNMOVABLE list
>> unmovable-page-requiremnt
>> 	borrow from UNMOVABLE list
>> 	NOW, it is BUG, we use CMA pages for unmovable usage.
>>
> 
> The patch still looks unnecessarily complex for what you are trying to

Which is complex in my code? __rmqueue_smallest()? __rmqueue_fallback()?

__rmqueue_smallest() ? I think it is required.

__rmqueue_fallback()?
It is just a cleanup for __rmqueue_fallback(), CMA is removed out from
__rmqueue_fallback(), so we can cleanup fallback(). I will remove/split
the cleanup part of the patch in next round.

> achieve and as a result I'm not reviewing it as carefully as I should.
> It looks like the entire patch boiled down to this hunk here
> 
> +#ifdef CONFIG_CMA
> +	if (unlikely(!page) && migratetype == MIGRATE_MOVABLE)
> +		page = __rmqueue_smallest(zone, order, MIGRATE_CMA);
> +#endif
> +
> 
> With that in place, this would would need to change from
> 
> [MIGRATE_MOVABLE]     = { MIGRATE_CMA, MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
> 
> to
> 
> [MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
> 
> because the fallback is already being handled as a special case. Leave
> the other fallback logic as it is.
> 
> This is not tested at all and is only meant to illustrate why I think
> your patch looks excessively complex for what you are trying to
> achieve.
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4beb7ae..0063e93 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -895,11 +895,9 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
>  static int fallbacks[MIGRATE_TYPES][4] = {
>  	[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE,     MIGRATE_RESERVE },
>  	[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE,     MIGRATE_RESERVE },
> +	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
>  #ifdef CONFIG_CMA
> -	[MIGRATE_MOVABLE]     = { MIGRATE_CMA,         MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
>  	[MIGRATE_CMA]         = { MIGRATE_RESERVE }, /* Never used */
> -#else
> -	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
>  #endif
>  	[MIGRATE_RESERVE]     = { MIGRATE_RESERVE }, /* Never used */
>  	[MIGRATE_ISOLATE]     = { MIGRATE_RESERVE }, /* Never used */
> @@ -1076,6 +1074,20 @@ static struct page *__rmqueue(struct zone *zone, unsigned int order,
>  
>  retry_reserve:
>  	page = __rmqueue_smallest(zone, order, migratetype);
> +#ifdef CONFIG_CMA
> +	if (!unlikely(!page) && migratetype == MIGRATE_MOVABLE) {
> +
> +		/*
> +		 * CMA is a special case where we want to use
> +		 * the smallest available page instead of splitting
> +		 * the largest chunks. We still must avoid the pages
> +		 * moving to MIGRATE_MOVABLE where they might be
> +		 * used for UNRECLAIMABLE or UNMOVABLE allocations
> +		 */
> +		migratetype = MIGRATE_CMA;
> +		goto retry_reserve;
> +	}
> +#endif /* CONFIG_CMA */
>  
>  	if (unlikely(!page) && migratetype != MIGRATE_RESERVE) {


need to add

+		if (migratetype == MIGRATE_CMA)
+			migratetype = MIGRATE_MOVABLE;

to restore the migratetype for fallback.

And your code are the same as mine in the view of CPU:
__rmqueue_smallest(MIGRATE_MOVABLE)
if failed: __rmqueue_smallest(MIGRATE_CMA)
if failed: __rmqueue_fallback()
if failed: __rmqueue_smallest(MIGRATE_RESERVE)

The differences are:
you just use "goto" instead "if" for instruction control.
your code are longer.
the number of branch in your code = mine + 1

My code have better readability:
Mine:

========================================================================
	page = __rmqueue_smallest(zone, order, migratetype);

#ifdef CONFIG_CMA
	if (unlikely(!page) && migratetype == MIGRATE_MOVABLE)
		page = __rmqueue_smallest(zone, order, MIGRATE_CMA);
#endif

	if (unlikely(!page))
		page = __rmqueue_fallback(zone, order, migratetype);

	if (unlikely(!page))
		page = __rmqueue_smallest(zone, order, MIGRATE_RESERVE);

	trace_mm_page_alloc_zone_locked(page, order, migratetype);
	return page;
=====================================================================

Yours:

==================================================================
retry_reserve:
	page = __rmqueue_smallest(zone, order, migratetype);

#ifdef CONFIG_CMA
	if (!unlikely(!page) && migratetype == MIGRATE_MOVABLE) {

		/*
		 * CMA is a special case where we want to use
		 * the smallest available page instead of splitting
		 * the largest chunks. We still must avoid the pages
		 * moving to MIGRATE_MOVABLE where they might be
		 * used for UNRECLAIMABLE or UNMOVABLE allocations
		 */
		migratetype = MIGRATE_CMA;
		goto retry_reserve;
	}
#endif /* CONFIG_CMA */


	if (unlikely(!page) && migratetype != MIGRATE_RESERVE) {
		if (migratetype == MIGRATE_CMA)
			migratetype = MIGRATE_MOVABLE;

		page = __rmqueue_fallback(zone, order, migratetype);

		/*
		 * Use MIGRATE_RESERVE rather than fail an allocation. goto
		 * is used because __rmqueue_smallest is an inline function
		 * and we want just one call site
		 */
		if (!page) {
			migratetype = MIGRATE_RESERVE;
			goto retry_reserve;
		}
	}

	trace_mm_page_alloc_zone_locked(page, order, migratetype);
	return page;
==========================================================================

How about this one? (just type it in the email client)

#define RMQUEUE_FALLBACK 1024
int rmqueue_list[3][4] = {
	[MIGRATE_UNMOVABLE] = { MIGRATE_UNMOVABLE, RMQUEUE_FALLBACK, MIGRATE_RESERVE},
	[MIGRATE_RECLAIMABLE] = { MIGRATE_RECLAIMABLE, RMQUEUE_FALLBACK, MIGRATE_RESERVE},
	[MIGRATE_MOVABLE] = {MIGRATE_MOVABLE, MIGRATE_CMA, RMQUEUE_FALLBACK, MIGRATE_RESERVE},
}

static struct page *__rmqueue(struct zone *zone, unsigned int order,
						int migratetype)
{
	struct page *page;
	int i, mt;

	for (i = 0; ; i++) {
		mt = rmqueue_list[migratetype][i];
		if (likely(mt != RMQUEUE_FALLBACK)
			page = __rmqueue_smallest(zone, order, mt);
		else
			page = __rmqueue_fallback(zone, order, migratetype);

		/* MIGRATE_RESERVE is always the last one */
		if (likely(page) || (mt == MIGRATE_RESERVE))
			break;
	}

	trace_mm_page_alloc_zone_locked(page, order, migratetype);
	return page;
}

Thanks,
Lai


>  		page = __rmqueue_fallback(zone, order, migratetype);
> --
> 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/
> 

--
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 PATCH 1/3 V1] mm, page_alloc: use __rmqueue_smallest when borrow memory from MIGRATE_CMA

[Mel Gorman]

On Thu, Jul 05, 2012 at 09:36:14AM +0800, Lai Jiangshan wrote:
> On 07/04/2012 07:19 PM, Mel Gorman wrote:
> > On Wed, Jul 04, 2012 at 06:43:47PM +0800, Lai Jiangshan wrote:
> >> On 07/04/2012 06:17 PM, Mel Gorman wrote:
> >>> On Wed, Jul 04, 2012 at 03:26:16PM +0800, Lai Jiangshan wrote:
> >>>> The pages of MIGRATE_CMA can't not be changed to the other type,
> >>>> nor be moved to the other free list. 
> >>>>
> >>>> ==>
> >>>> So when we use __rmqueue_fallback() to borrow memory from MIGRATE_CMA,
> >>>> one of the highest order page is borrowed and it is split.
> >>>> But the free pages resulted by splitting can NOT
> >>>> be moved to MIGRATE_MOVABLE.
> >>>>
> >>>> ==>
> >>>> So in the next time of allocation, we NEED to borrow again,
> >>>> another one of the highest order page is borrowed from CMA and it is split.
> >>>> and results some other new split free pages.
> >>>>
> >>>
> >>> Then special case __rmqueue_fallback() to move pages stolen from
> >>> MIGRATE_CMA to the MIGRATE_MOVABLE lists but do not change the pageblock
> >>> type.
> >>
> >> Because unmovable-page-requirement can allocate page from
> >> MIGRATE_MOVABLE free list. So We can not move MIGRATE_CMA pages
> >> to the MIGRATE_MOVABLE free list.
> >>
> > 
> > Ok, good point.
> > 
> >> See here:
> >>
> >> MOVABLE list is empty
> >> UNMOVABLE list is empty
> >> movable-page-requirement
> >> 	borrow from CMA list
> >> 	split it, others are put into UNMOVABLE list
> >> unmovable-page-requiremnt
> >> 	borrow from UNMOVABLE list
> >> 	NOW, it is BUG, we use CMA pages for unmovable usage.
> >>
> > 
> > The patch still looks unnecessarily complex for what you are trying to
> 
> Which is complex in my code? __rmqueue_smallest()? __rmqueue_fallback()?
> 

It's the review that was confusing. It churned a lot of code, altered the
fallback lists, added a misleading name (MIGRATE_PRIME_TYPES because PRIME
in this context does not mean anything useful), added a warning that made
no sense and stopped __rmqueue_smallest from being inlined. In cases
like this it is better to split your patch into the part you want
followed by a cleanup patch if that is necessary.

> __rmqueue_smallest() ? I think it is required.
> 
> __rmqueue_fallback()?
> It is just a cleanup for __rmqueue_fallback(), CMA is removed out from
> __rmqueue_fallback(), so we can cleanup fallback(). I will remove/split
> the cleanup part of the patch in next round.
> 
> > achieve and as a result I'm not reviewing it as carefully as I should.
> > It looks like the entire patch boiled down to this hunk here
> > 
> > +#ifdef CONFIG_CMA
> > +	if (unlikely(!page) && migratetype == MIGRATE_MOVABLE)
> > +		page = __rmqueue_smallest(zone, order, MIGRATE_CMA);
> > +#endif
> > +
> > 
> > With that in place, this would would need to change from
> > 
> > [MIGRATE_MOVABLE]     = { MIGRATE_CMA, MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
> > 
> > to
> > 
> > [MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
> > 
> > because the fallback is already being handled as a special case. Leave
> > the other fallback logic as it is.
> > 
> > This is not tested at all and is only meant to illustrate why I think
> > your patch looks excessively complex for what you are trying to
> > achieve.
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 4beb7ae..0063e93 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -895,11 +895,9 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
> >  static int fallbacks[MIGRATE_TYPES][4] = {
> >  	[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE,     MIGRATE_RESERVE },
> >  	[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE,     MIGRATE_RESERVE },
> > +	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
> >  #ifdef CONFIG_CMA
> > -	[MIGRATE_MOVABLE]     = { MIGRATE_CMA,         MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
> >  	[MIGRATE_CMA]         = { MIGRATE_RESERVE }, /* Never used */
> > -#else
> > -	[MIGRATE_MOVABLE]     = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   MIGRATE_RESERVE },
> >  #endif
> >  	[MIGRATE_RESERVE]     = { MIGRATE_RESERVE }, /* Never used */
> >  	[MIGRATE_ISOLATE]     = { MIGRATE_RESERVE }, /* Never used */
> > @@ -1076,6 +1074,20 @@ static struct page *__rmqueue(struct zone *zone, unsigned int order,
> >  
> >  retry_reserve:
> >  	page = __rmqueue_smallest(zone, order, migratetype);
> > +#ifdef CONFIG_CMA
> > +	if (!unlikely(!page) && migratetype == MIGRATE_MOVABLE) {
> > +
> > +		/*
> > +		 * CMA is a special case where we want to use
> > +		 * the smallest available page instead of splitting
> > +		 * the largest chunks. We still must avoid the pages
> > +		 * moving to MIGRATE_MOVABLE where they might be
> > +		 * used for UNRECLAIMABLE or UNMOVABLE allocations
> > +		 */
> > +		migratetype = MIGRATE_CMA;
> > +		goto retry_reserve;
> > +	}
> > +#endif /* CONFIG_CMA */
> >  
> >  	if (unlikely(!page) && migratetype != MIGRATE_RESERVE) {
> 
> 
> need to add
> 
> +		if (migratetype == MIGRATE_CMA)
> +			migratetype = MIGRATE_MOVABLE;
> 
> to restore the migratetype for fallback.
> 

True, but it'd still be easier to review and __rmqueue_smallest would
still be inlined.

> And your code are the same as mine in the view of CPU:
> __rmqueue_smallest(MIGRATE_MOVABLE)
> if failed: __rmqueue_smallest(MIGRATE_CMA)
> if failed: __rmqueue_fallback()
> if failed: __rmqueue_smallest(MIGRATE_RESERVE)
> 
> The differences are:
> you just use "goto" instead "if" for instruction control.
> your code are longer.
> the number of branch in your code = mine + 1
> 
> My code have better readability:
> Mine:
> 
> ========================================================================
> 	page = __rmqueue_smallest(zone, order, migratetype);
> 
> #ifdef CONFIG_CMA
> 	if (unlikely(!page) && migratetype == MIGRATE_MOVABLE)
> 		page = __rmqueue_smallest(zone, order, MIGRATE_CMA);
> #endif
> 
> 	if (unlikely(!page))
> 		page = __rmqueue_fallback(zone, order, migratetype);
> 
> 	if (unlikely(!page))
> 		page = __rmqueue_smallest(zone, order, MIGRATE_RESERVE);
> 
> 	trace_mm_page_alloc_zone_locked(page, order, migratetype);
> 	return page;
> =====================================================================
> 
> Yours:
> 
> ==================================================================
> retry_reserve:
> 	page = __rmqueue_smallest(zone, order, migratetype);
> 
> #ifdef CONFIG_CMA
> 	if (!unlikely(!page) && migratetype == MIGRATE_MOVABLE) {
> 
> 		/*
> 		 * CMA is a special case where we want to use
> 		 * the smallest available page instead of splitting
> 		 * the largest chunks. We still must avoid the pages
> 		 * moving to MIGRATE_MOVABLE where they might be
> 		 * used for UNRECLAIMABLE or UNMOVABLE allocations
> 		 */
> 		migratetype = MIGRATE_CMA;
> 		goto retry_reserve;
> 	}
> #endif /* CONFIG_CMA */
> 
> 
> 	if (unlikely(!page) && migratetype != MIGRATE_RESERVE) {
> 		if (migratetype == MIGRATE_CMA)
> 			migratetype = MIGRATE_MOVABLE;
> 
> 		page = __rmqueue_fallback(zone, order, migratetype);
> 
> 		/*
> 		 * Use MIGRATE_RESERVE rather than fail an allocation. goto
> 		 * is used because __rmqueue_smallest is an inline function
> 		 * and we want just one call site
> 		 */
> 		if (!page) {
> 			migratetype = MIGRATE_RESERVE;
> 			goto retry_reserve;
> 		}
> 	}
> 
> 	trace_mm_page_alloc_zone_locked(page, order, migratetype);
> 	return page;
> ==========================================================================
> 
> How about this one? (just type it in the email client)
> 
> #define RMQUEUE_FALLBACK 1024

Use -1 to be clear this is an impossible index. Add a comment explaining
that

> int rmqueue_list[3][4] = {

Use MIGRATE_PCPTYPES and MIGRATE_PCPTYPES+1 to size the array.

> 	[MIGRATE_UNMOVABLE] = { MIGRATE_UNMOVABLE, RMQUEUE_FALLBACK, MIGRATE_RESERVE},
> 	[MIGRATE_RECLAIMABLE] = { MIGRATE_RECLAIMABLE, RMQUEUE_FALLBACK, MIGRATE_RESERVE},
> 	[MIGRATE_MOVABLE] = {MIGRATE_MOVABLE, MIGRATE_CMA, RMQUEUE_FALLBACK, MIGRATE_RESERVE},
> }
> 
> static struct page *__rmqueue(struct zone *zone, unsigned int order,
> 						int migratetype)
> {
> 	struct page *page;
> 	int i, mt;
> 
> 	for (i = 0; ; i++) {
> 		mt = rmqueue_list[migratetype][i];
> 		if (likely(mt != RMQUEUE_FALLBACK)
> 			page = __rmqueue_smallest(zone, order, mt);
> 		else
> 			page = __rmqueue_fallback(zone, order, migratetype);
> 
> 		/* MIGRATE_RESERVE is always the last one */
> 		if (likely(page) || (mt == MIGRATE_RESERVE))
> 			break;
> 	}
> 
> 	trace_mm_page_alloc_zone_locked(page, order, migratetype);
> 	return page;
> }

That would indeed churn the code a lot less and preserve the inlining of
__rmqueue_smallest. It comes at the cost of an additional static array
but the flow is clear at least.

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

Re: [RFC PATCH 2/3 V1] mm, page migrate: add MIGRATE_HOTREMOVE type

[Mel Gorman]

On Wed, Jul 04, 2012 at 03:26:17PM +0800, Lai Jiangshan wrote:
> MIGRATE_HOTREMOVE is a special kind of MIGRATE_MOVABLE, but it is stable:
> any page of the type can NOT be changed to the other type nor be moved to
> the other free list.
> 
> So the pages of MIGRATE_HOTREMOVE are always movable, this ability is
> useful for hugepages and hotremove ...etc.
> 
> MIGRATE_HOTREMOVE pages is the used as the first candidate when
> we allocate movable pages.
> 
> 1) add small routine is_migrate_movable() for movable-like types
> 2) add small routine is_migrate_stable() for stable types
> 3) fix some comments
> 4) fix get_any_page(). The get_any_page() may change
>    MIGRATE_CMA/HOTREMOVE types page to MOVABLE which may cause this page
>    to be changed to UNMOVABLE.
> 

Reuse MIGRATE_CMA. Even if the pages are on the movable lists it should
not be a problem for memory hot-remove.

-- 
Mel Gorman
SUSE Labs

Re: [RFC PATCH 2/3 V1] mm, page migrate: add MIGRATE_HOTREMOVE type

[Lai Jiangshan]

On 07/04/2012 06:19 PM, Mel Gorman wrote:
> On Wed, Jul 04, 2012 at 03:26:17PM +0800, Lai Jiangshan wrote:
>> MIGRATE_HOTREMOVE is a special kind of MIGRATE_MOVABLE, but it is stable:
>> any page of the type can NOT be changed to the other type nor be moved to
>> the other free list.
>>
>> So the pages of MIGRATE_HOTREMOVE are always movable, this ability is
>> useful for hugepages and hotremove ...etc.
>>
>> MIGRATE_HOTREMOVE pages is the used as the first candidate when
>> we allocate movable pages.
>>
>> 1) add small routine is_migrate_movable() for movable-like types
>> 2) add small routine is_migrate_stable() for stable types
>> 3) fix some comments
>> 4) fix get_any_page(). The get_any_page() may change
>>    MIGRATE_CMA/HOTREMOVE types page to MOVABLE which may cause this page
>>    to be changed to UNMOVABLE.
>>
> 
> Reuse MIGRATE_CMA. 

Will do it.

> Even if the pages are on the movable lists it should
> not be a problem for memory hot-remove.

It does have problem, unmovable pages may be allocated on it.

The movable lists can be used for other type when ohter type is empty.
Or we can rename current movable-lists to movable-preference-lists.

Thanks,
Lai

--
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 PATCH 0/3 V1] mm: add new migrate type and online_movable for hotplug

[Mel Gorman]

On Wed, Jul 04, 2012 at 03:26:15PM +0800, Lai Jiangshan wrote:
> > <SNIP>
> 
> Different from ZONE_MOVABLE: it can be used for any given memroyblock.
> 
> Lai Jiangshan (3):
>   use __rmqueue_smallest when borrow memory from MIGRATE_CMA
>   add MIGRATE_HOTREMOVE type
>   add online_movable
> 
>  arch/tile/mm/init.c            |    2 +-
>  drivers/acpi/acpi_memhotplug.c |    3 +-
>  drivers/base/memory.c          |   24 +++++++----
>  include/linux/memory.h         |    1 +
>  include/linux/memory_hotplug.h |    4 +-
>  include/linux/mmzone.h         |   37 +++++++++++++++++
>  include/linux/page-isolation.h |    2 +-
>  mm/compaction.c                |    6 +-
>  mm/memory-failure.c            |    8 +++-
>  mm/memory_hotplug.c            |   36 +++++++++++++---
>  mm/page_alloc.c                |   86 ++++++++++++++++-----------------------
>  mm/vmstat.c                    |    3 +
>  12 files changed, 136 insertions(+), 76 deletions(-)
> 

I apologise for my crap review of the first patch to date. It was atrociously
bad form and one of the reasons my review was so superficial was because I
was aware of the problem below. It's pretty severe, we've encountered it on
other occasions and it led me to dismiss the series quickly without adequate
explanation or close review when I should have taken the time to explain it.

The reason ZONE_MOVABLE exists is because of page reclaim. MIGRATE_CMA
or any migrate type that is MIGRATE_CMA-like is not understood by reclaim
currently and is not addressed by this series just from looking the diffstat
(no changes to vmscan.c). In low memory situations, it's actually fine
and the system appears to work well. The problem is either when the
MIGRATE_CMA-like area is large and is either completely free or is the
only source of pages that can be reclaimed.

In these cases, MIGRATE_UNMOVABLE and MIGRATE_RECLAIMABLE allocations fail
because their lists and fallback lists are empty. However, if it enters
direct reclaim or wakes kswapd the watermarks are fine and reclaim does
nothing. Depending on implementation details this causes either a loop
or OOM.

Minimally the watermark checks need to take the MIGRATE_CMA area into account
but even then it is still fragile. If kswapd and direct reclaim are forced
to reclaim pages, there is no guarantee they will reclaim pages that are
usable by MIGRATE_UNMOVABLE or MIGRATE_RECLAIMABLE. To handle this you must
either keep reclaiming pages until it works (easy to implement but disruptive
to the system) or scan the LRU lists searching for suitable pages (higher
CPU usage, LRU age disruption, will require the entire zone to be scanned
in the OOM case which will be slow and subject to races and possible false
OOMs). When these situations occur, it is very difficult to debug because it
just looks like a hang and the exact triggering situations will be different.

If the allocation then fails due to insufficient usable memory, the
resulting OOM message will be harder to read because it'll show OOM when
there are plenty of pages free. This can be addressed by clear accounting and
informative messages of course but to be very clear it might be necessary
to walk all the buddy lists to identify how many of the free pages were
MIGRATE_CMA. You could use separate accounting of course but then you have
accounting and memory overhead instead.

In the case of CMA, this issue is less of a problem but it was discussed
before CMA was merged. CMAs use case means that it is not likely to suffer
severely because of the expected size of the region, how its used and how
many slab allocations are expected on the systems it targets. It's far worse
for memory hotplug because if the bulk of your memory is memory hotplugged,
you may not be able to use it for metadata-intensive workloads for example
which will result in bug reports. You could have 90% free memory and
be unable to use any of it because you cannot increase the size of slab
leading to odd corner cases.

ZONE_MOVABLE is not great, but it handles the reclaim issues in a
straight-forward manner, OOM is handled quickly because the whole system
does not have to be scanned to detect the situation and the OOM messages
are easy to read. If you want to replace it with MIGRATE_CMA or
something MIGRATE_CMA-like, you need to take these issues into account
or at the very least explain in detail why it is not an issue.

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