Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1578672 > unrolled thread

[PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock Vlastimil Babka <vbabka@suse.cz> - 2017-02-10 19:20 +0100
    Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when  stealing from pageblock Mel Gorman <mgorman@techsingularity.net> - 2017-02-13 12:00 +0100
    Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing  from pageblock Xishi Qiu <qiuxishi@huawei.com> - 2017-02-14 11:10 +0100
      Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when  stealing from pageblock Vlastimil Babka <vbabka@suse.cz> - 2017-02-15 11:50 +0100
        Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing  from pageblock Xishi Qiu <qiuxishi@huawei.com> - 2017-02-15 13:00 +0100
          Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when  stealing from pageblock Vlastimil Babka <vbabka@suse.cz> - 2017-02-17 17:30 +0100
    Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when  stealing from pageblock Johannes Weiner <hannes@cmpxchg.org> - 2017-02-14 19:20 +0100
      Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when  stealing from pageblock Vlastimil Babka <vbabka@suse.cz> - 2017-02-17 17:20 +0100

#1578672 — [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

When stealing pages from pageblock of a different migratetype, we count how
many free pages were stolen, and change the pageblock's migratetype if more
than half of the pageblock was free. This might be too conservative, as there
might be other pages that are not free, but were allocated with the same
migratetype as our allocation requested.

While we cannot determine the migratetype of allocated pages precisely (at
least without the page_owner functionality enabled), we can count pages that
compaction would try to isolate for migration - those are either on LRU or
__PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
done as part of free page stealing with little additional overhead.

The page stealing code is changed so that it considers free pages plus pages
of the "good" migratetype for the decision whether to change pageblock's
migratetype.

The result should be more accurate migratetype of pageblocks wrt the actual
pages in the pageblocks, when stealing from semi-occupied pageblocks. This
should help the efficiency of page grouping by mobility.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/page-isolation.h |  5 +---
 mm/page_alloc.c                | 54 +++++++++++++++++++++++++++++++++---------
 mm/page_isolation.c            |  5 ++--
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
index 047d64706f2a..d4cd2014fa6f 100644
--- a/include/linux/page-isolation.h
+++ b/include/linux/page-isolation.h
@@ -33,10 +33,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 			 bool skip_hwpoisoned_pages);
 void set_pageblock_migratetype(struct page *page, int migratetype);
 int move_freepages_block(struct zone *zone, struct page *page,
-				int migratetype);
-int move_freepages(struct zone *zone,
-			  struct page *start_page, struct page *end_page,
-			  int migratetype);
+				int migratetype, int *num_movable);
 
 /*
  * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 314e6b9ddbc4..a7d33818610f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1844,9 +1844,9 @@ static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
  * Note that start_page and end_pages are not aligned on a pageblock
  * boundary. If alignment is required, use move_freepages_block()
  */
-int move_freepages(struct zone *zone,
+static int move_freepages(struct zone *zone,
 			  struct page *start_page, struct page *end_page,
-			  int migratetype)
+			  int migratetype, int *num_movable)
 {
 	struct page *page;
 	unsigned int order;
@@ -1863,6 +1863,9 @@ int move_freepages(struct zone *zone,
 	VM_BUG_ON(page_zone(start_page) != page_zone(end_page));
 #endif
 
+	if (num_movable)
+		*num_movable = 0;
+
 	for (page = start_page; page <= end_page;) {
 		/* Make sure we are not inadvertently changing nodes */
 		VM_BUG_ON_PAGE(page_to_nid(page) != zone_to_nid(zone), page);
@@ -1873,6 +1876,14 @@ int move_freepages(struct zone *zone,
 		}
 
 		if (!PageBuddy(page)) {
+			/*
+			 * We assume that pages that could be isolated for
+			 * migration are movable. But we don't actually try
+			 * isolating, as that would be expensive.
+			 */
+			if (num_movable && (PageLRU(page) || __PageMovable(page)))
+				(*num_movable)++;
+
 			page++;
 			continue;
 		}
@@ -1888,7 +1899,7 @@ int move_freepages(struct zone *zone,
 }
 
 int move_freepages_block(struct zone *zone, struct page *page,
-				int migratetype)
+				int migratetype, int *num_movable)
 {
 	unsigned long start_pfn, end_pfn;
 	struct page *start_page, *end_page;
@@ -1905,7 +1916,8 @@ int move_freepages_block(struct zone *zone, struct page *page,
 	if (!zone_spans_pfn(zone, end_pfn))
 		return 0;
 
-	return move_freepages(zone, start_page, end_page, migratetype);
+	return move_freepages(zone, start_page, end_page, migratetype,
+								num_movable);
 }
 
 static void change_pageblock_range(struct page *pageblock_page,
@@ -1960,11 +1972,12 @@ static bool can_steal_fallback(unsigned int order, int start_mt)
  * use it's pages as requested migratetype in the future.
  */
 static void steal_suitable_fallback(struct zone *zone, struct page *page,
-					 int start_type, bool whole_block)
+					int start_type, bool whole_block)
 {
 	unsigned int current_order = page_order(page);
 	struct free_area *area;
-	int pages;
+	int free_pages, good_pages;
+	int old_block_type;
 
 	/* Take ownership for orders >= pageblock_order */
 	if (current_order >= pageblock_order) {
@@ -1981,10 +1994,29 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
 		return;
 	}
 
-	pages = move_freepages_block(zone, page, start_type);
+	free_pages = move_freepages_block(zone, page, start_type,
+						&good_pages);
+	/*
+	 * good_pages is now the number of movable pages, but if we
+	 * want UNMOVABLE or RECLAIMABLE allocation, it's more tricky
+	 */
+	if (start_type != MIGRATE_MOVABLE) {
+		/*
+		 * If we are falling back to MIGRATE_MOVABLE pageblock,
+		 * treat all non-movable pages as good. If it's UNMOVABLE
+		 * falling back to RECLAIMABLE or vice versa, be conservative
+		 * as we can't distinguish the exact migratetype.
+		 */
+		old_block_type = get_pageblock_migratetype(page);
+		if (old_block_type == MIGRATE_MOVABLE)
+			good_pages = pageblock_nr_pages
+						- free_pages - good_pages;
+		else
+			good_pages = 0;
+	}
 
-	/* Claim the whole block if over half of it is free */
-	if (pages >= (1 << (pageblock_order-1)) ||
+	/* Claim the whole block if over half of it is free or good type */
+	if (free_pages + good_pages >= (1 << (pageblock_order-1)) ||
 			page_group_by_mobility_disabled)
 		set_pageblock_migratetype(page, start_type);
 }
@@ -2056,7 +2088,7 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
 			!is_migrate_isolate(mt) && !is_migrate_cma(mt)) {
 		zone->nr_reserved_highatomic += pageblock_nr_pages;
 		set_pageblock_migratetype(page, MIGRATE_HIGHATOMIC);
-		move_freepages_block(zone, page, MIGRATE_HIGHATOMIC);
+		move_freepages_block(zone, page, MIGRATE_HIGHATOMIC, NULL);
 	}
 
 out_unlock:
@@ -2113,7 +2145,7 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 			 * may increase.
 			 */
 			set_pageblock_migratetype(page, ac->migratetype);
-			move_freepages_block(zone, page, ac->migratetype);
+			move_freepages_block(zone, page, ac->migratetype, NULL);
 			spin_unlock_irqrestore(&zone->lock, flags);
 			return;
 		}
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index a5594bfcc5ed..29c2f9b9aba7 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -66,7 +66,8 @@ static int set_migratetype_isolate(struct page *page,
 
 		set_pageblock_migratetype(page, MIGRATE_ISOLATE);
 		zone->nr_isolate_pageblock++;
-		nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE);
+		nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE,
+									NULL);
 
 		__mod_zone_freepage_state(zone, -nr_pages, migratetype);
 	}
@@ -120,7 +121,7 @@ static void unset_migratetype_isolate(struct page *page, unsigned migratetype)
 	 * pageblock scanning for freepage moving.
 	 */
 	if (!isolated_page) {
-		nr_pages = move_freepages_block(zone, page, migratetype);
+		nr_pages = move_freepages_block(zone, page, migratetype, NULL);
 		__mod_zone_freepage_state(zone, nr_pages, migratetype);
 	}
 	set_pageblock_migratetype(page, migratetype);
-- 
2.11.0

[toc] | [next] | [standalone]

#1579614 — Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

On Fri, Feb 10, 2017 at 06:23:37PM +0100, Vlastimil Babka wrote:
> When stealing pages from pageblock of a different migratetype, we count how
> many free pages were stolen, and change the pageblock's migratetype if more
> than half of the pageblock was free. This might be too conservative, as there
> might be other pages that are not free, but were allocated with the same
> migratetype as our allocation requested.
> 
> While we cannot determine the migratetype of allocated pages precisely (at
> least without the page_owner functionality enabled), we can count pages that
> compaction would try to isolate for migration - those are either on LRU or
> __PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
> MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
> done as part of free page stealing with little additional overhead.
> 
> The page stealing code is changed so that it considers free pages plus pages
> of the "good" migratetype for the decision whether to change pageblock's
> migratetype.
> 
> The result should be more accurate migratetype of pageblocks wrt the actual
> pages in the pageblocks, when stealing from semi-occupied pageblocks. This
> should help the efficiency of page grouping by mobility.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

While it's fine now, it may be necessary to unify the checks that
compaction and the page allocator use for determining if the page can
move. In general, this is still a better idea for a modest amount of
overhead in a path that is considered slow anyway so

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1580445 — Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

On 2017/2/11 1:23, Vlastimil Babka wrote:

> When stealing pages from pageblock of a different migratetype, we count how
> many free pages were stolen, and change the pageblock's migratetype if more
> than half of the pageblock was free. This might be too conservative, as there
> might be other pages that are not free, but were allocated with the same
> migratetype as our allocation requested.
> 
> While we cannot determine the migratetype of allocated pages precisely (at
> least without the page_owner functionality enabled), we can count pages that
> compaction would try to isolate for migration - those are either on LRU or
> __PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
> MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
> done as part of free page stealing with little additional overhead.
> 
> The page stealing code is changed so that it considers free pages plus pages
> of the "good" migratetype for the decision whether to change pageblock's
> migratetype.
> 
> The result should be more accurate migratetype of pageblocks wrt the actual
> pages in the pageblocks, when stealing from semi-occupied pageblocks. This
> should help the efficiency of page grouping by mobility.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Hi Vlastimil,

How about these two changes?

1. If we steal some free pages, we will add these page at the head of start_migratetype
list, it will cause more fixed, because these pages will be allocated more easily.
So how about use list_move_tail instead of list_move?

__rmqueue_fallback
	steal_suitable_fallback
		move_freepages_block
			move_freepages
				list_move

2. When doing expand() - list_add(), usually the list is empty, but in the
following case, the list is not empty, because we did move_freepages_block()
before.

__rmqueue_fallback
	steal_suitable_fallback
		move_freepages_block  // move to the list of start_migratetype
	expand  // split the largest order
		list_add  // add to the list of start_migratetype

So how about use list_add_tail instead of list_add? Then we can merge the large
block again as soon as the page freed.

Thanks,
Xishi Qiu

[toc] | [prev] | [next] | [standalone]

#1581207 — Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

On 02/14/2017 11:07 AM, Xishi Qiu wrote:
> On 2017/2/11 1:23, Vlastimil Babka wrote:
> 
>> When stealing pages from pageblock of a different migratetype, we count how
>> many free pages were stolen, and change the pageblock's migratetype if more
>> than half of the pageblock was free. This might be too conservative, as there
>> might be other pages that are not free, but were allocated with the same
>> migratetype as our allocation requested.
>>
>> While we cannot determine the migratetype of allocated pages precisely (at
>> least without the page_owner functionality enabled), we can count pages that
>> compaction would try to isolate for migration - those are either on LRU or
>> __PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
>> MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
>> done as part of free page stealing with little additional overhead.
>>
>> The page stealing code is changed so that it considers free pages plus pages
>> of the "good" migratetype for the decision whether to change pageblock's
>> migratetype.
>>
>> The result should be more accurate migratetype of pageblocks wrt the actual
>> pages in the pageblocks, when stealing from semi-occupied pageblocks. This
>> should help the efficiency of page grouping by mobility.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Hi Vlastimil,
> 
> How about these two changes?
> 
> 1. If we steal some free pages, we will add these page at the head of start_migratetype
> list, it will cause more fixed, because these pages will be allocated more easily.

What do you mean by "more fixed" here?

> So how about use list_move_tail instead of list_move?

Hmm, not sure if it can make any difference. We steal because the lists
are currently empty (at least for the order we want), so it shouldn't
matter if we add to head or tail.

> __rmqueue_fallback
> 	steal_suitable_fallback
> 		move_freepages_block
> 			move_freepages
> 				list_move
> 
> 2. When doing expand() - list_add(), usually the list is empty, but in the
> following case, the list is not empty, because we did move_freepages_block()
> before.
> 
> __rmqueue_fallback
> 	steal_suitable_fallback
> 		move_freepages_block  // move to the list of start_migratetype
> 	expand  // split the largest order
> 		list_add  // add to the list of start_migratetype
> 
> So how about use list_add_tail instead of list_add? Then we can merge the large
> block again as soon as the page freed.

Same here. The lists are not empty, but contain probably just the pages
from our stolen pageblock. It shouldn't matter how we order them within
the same block.

So maybe it could make some difference for higher-order allocations, but
it's unclear to me. Making e.g. expand() more complex with a flag to
tell it the head vs tail add could mean extra overhead in allocator fast
path that would offset any gains.

> Thanks,
> Xishi Qiu
> 

[toc] | [prev] | [next] | [standalone]

#1581253 — Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

On 2017/2/15 18:47, Vlastimil Babka wrote:

> On 02/14/2017 11:07 AM, Xishi Qiu wrote:
>> On 2017/2/11 1:23, Vlastimil Babka wrote:
>>
>>> When stealing pages from pageblock of a different migratetype, we count how
>>> many free pages were stolen, and change the pageblock's migratetype if more
>>> than half of the pageblock was free. This might be too conservative, as there
>>> might be other pages that are not free, but were allocated with the same
>>> migratetype as our allocation requested.
>>>
>>> While we cannot determine the migratetype of allocated pages precisely (at
>>> least without the page_owner functionality enabled), we can count pages that
>>> compaction would try to isolate for migration - those are either on LRU or
>>> __PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
>>> MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
>>> done as part of free page stealing with little additional overhead.
>>>
>>> The page stealing code is changed so that it considers free pages plus pages
>>> of the "good" migratetype for the decision whether to change pageblock's
>>> migratetype.
>>>
>>> The result should be more accurate migratetype of pageblocks wrt the actual
>>> pages in the pageblocks, when stealing from semi-occupied pageblocks. This
>>> should help the efficiency of page grouping by mobility.
>>>
>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>>
>> Hi Vlastimil,
>>
>> How about these two changes?
>>
>> 1. If we steal some free pages, we will add these page at the head of start_migratetype
>> list, it will cause more fixed, because these pages will be allocated more easily.
> 
> What do you mean by "more fixed" here?
> 
>> So how about use list_move_tail instead of list_move?
> 
> Hmm, not sure if it can make any difference. We steal because the lists
> are currently empty (at least for the order we want), so it shouldn't
> matter if we add to head or tail.
> 

Hi Vlastimil,

Please see the following case, I am not sure if it is right.

MIGRATE_MOVABLE
order:    0 1 2 3 4 5 6 7 8 9 10
free num: 1 1 1 1 1 1 1 1 1 1 0  // one page(e.g. page A) was allocated before

MIGRATE_UNMOVABLE
order:    0 1 2 3 4 5 6 7 8 9 10
free num: x x x x 0 0 0 0 0 0 0 // we want order=4, so steal from MIGRATE_MOVABLE

We alloc order=4 in MIGRATE_UNMOVABLE, then it will fallback to steal pages from
MIGRATE_MOVABLE, and we will move free pages form MIGRATE_MOVABLE list to 
MIGRATE_UNMOVABLE list.

List of order 4-9 in MIGRATE_UNMOVABLE is empty, so add head or tail is the same.
But order 0-3 is not empty, so if we add to the head, we will allocate pages which
stolen from MIGRATE_MOVABLE first later. So we will have less chance to make a large
block(order=10) when the one page(page A) free again.

Also we will split order=9 which from MIGRATE_MOVABLE to alloc order=4 in expand(),
so if we add to the head, we will allocate pages which split from order=9 first later.
So we will have less chance to make a large block(order=9) when the order=4 page
free again.

>> __rmqueue_fallback
>> 	steal_suitable_fallback
>> 		move_freepages_block
>> 			move_freepages
>> 				list_move
>>
>> 2. When doing expand() - list_add(), usually the list is empty, but in the
>> following case, the list is not empty, because we did move_freepages_block()
>> before.
>>
>> __rmqueue_fallback
>> 	steal_suitable_fallback
>> 		move_freepages_block  // move to the list of start_migratetype
>> 	expand  // split the largest order
>> 		list_add  // add to the list of start_migratetype
>>
>> So how about use list_add_tail instead of list_add? Then we can merge the large
>> block again as soon as the page freed.
> 
> Same here. The lists are not empty, but contain probably just the pages
> from our stolen pageblock. It shouldn't matter how we order them within
> the same block.
> 
> So maybe it could make some difference for higher-order allocations, but
> it's unclear to me. Making e.g. expand() more complex with a flag to
> tell it the head vs tail add could mean extra overhead in allocator fast
> path that would offset any gains.
> 
>> Thanks,
>> Xishi Qiu
>>
> 
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1583586 — Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

On 02/15/2017 12:56 PM, Xishi Qiu wrote:
> On 2017/2/15 18:47, Vlastimil Babka wrote:
> 
>> On 02/14/2017 11:07 AM, Xishi Qiu wrote:
>>> On 2017/2/11 1:23, Vlastimil Babka wrote:
>>>
>>>> When stealing pages from pageblock of a different migratetype, we count how
>>>> many free pages were stolen, and change the pageblock's migratetype if more
>>>> than half of the pageblock was free. This might be too conservative, as there
>>>> might be other pages that are not free, but were allocated with the same
>>>> migratetype as our allocation requested.
>>>>
>>>> While we cannot determine the migratetype of allocated pages precisely (at
>>>> least without the page_owner functionality enabled), we can count pages that
>>>> compaction would try to isolate for migration - those are either on LRU or
>>>> __PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
>>>> MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
>>>> done as part of free page stealing with little additional overhead.
>>>>
>>>> The page stealing code is changed so that it considers free pages plus pages
>>>> of the "good" migratetype for the decision whether to change pageblock's
>>>> migratetype.
>>>>
>>>> The result should be more accurate migratetype of pageblocks wrt the actual
>>>> pages in the pageblocks, when stealing from semi-occupied pageblocks. This
>>>> should help the efficiency of page grouping by mobility.
>>>>
>>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>>>
>>> Hi Vlastimil,
>>>
>>> How about these two changes?
>>>
>>> 1. If we steal some free pages, we will add these page at the head of start_migratetype
>>> list, it will cause more fixed, because these pages will be allocated more easily.
>> 
>> What do you mean by "more fixed" here?
>> 
>>> So how about use list_move_tail instead of list_move?
>> 
>> Hmm, not sure if it can make any difference. We steal because the lists
>> are currently empty (at least for the order we want), so it shouldn't
>> matter if we add to head or tail.
>> 
> 
> Hi Vlastimil,
> 
> Please see the following case, I am not sure if it is right.
> 
> MIGRATE_MOVABLE
> order:    0 1 2 3 4 5 6 7 8 9 10
> free num: 1 1 1 1 1 1 1 1 1 1 0  // one page(e.g. page A) was allocated before
>
> MIGRATE_UNMOVABLE
> order:    0 1 2 3 4 5 6 7 8 9 10
> free num: x x x x 0 0 0 0 0 0 0 // we want order=4, so steal from MIGRATE_MOVABLE
> 
> We alloc order=4 in MIGRATE_UNMOVABLE, then it will fallback to steal pages from
> MIGRATE_MOVABLE, and we will move free pages form MIGRATE_MOVABLE list to 
> MIGRATE_UNMOVABLE list.
> 
> List of order 4-9 in MIGRATE_UNMOVABLE is empty, so add head or tail is the same.
> But order 0-3 is not empty, so if we add to the head, we will allocate pages which
> stolen from MIGRATE_MOVABLE first later. So we will have less chance to make a large
> block(order=10) when the one page(page A) free again.

I see. But do we know that page A, and the order-4 page we just allocated, are
both going to be freed soon? It's not a clear win to me, so maybe you can try
implementing it and see if it makes any difference?

> Also we will split order=9 which from MIGRATE_MOVABLE to alloc order=4 in expand(),

Yes, for pageblock order == 9.

> so if we add to the head, we will allocate pages which split from order=9 first later.
> So we will have less chance to make a large block(order=9) when the order=4 page
> free again.

Again that assumes our order-4 allocation is temporary. Is there a significant
chance of this?

>>> __rmqueue_fallback
>>> 	steal_suitable_fallback
>>> 		move_freepages_block
>>> 			move_freepages
>>> 				list_move
>>>
>>> 2. When doing expand() - list_add(), usually the list is empty, but in the
>>> following case, the list is not empty, because we did move_freepages_block()
>>> before.
>>>
>>> __rmqueue_fallback
>>> 	steal_suitable_fallback
>>> 		move_freepages_block  // move to the list of start_migratetype
>>> 	expand  // split the largest order
>>> 		list_add  // add to the list of start_migratetype
>>>
>>> So how about use list_add_tail instead of list_add? Then we can merge the large
>>> block again as soon as the page freed.
>> 
>> Same here. The lists are not empty, but contain probably just the pages
>> from our stolen pageblock. It shouldn't matter how we order them within
>> the same block.
>> 
>> So maybe it could make some difference for higher-order allocations, but
>> it's unclear to me. Making e.g. expand() more complex with a flag to
>> tell it the head vs tail add could mean extra overhead in allocator fast
>> path that would offset any gains.
>> 
>>> Thanks,
>>> Xishi Qiu
>>>
>> 
>> 
>> .
>> 
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1580736 — Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

On Fri, Feb 10, 2017 at 06:23:37PM +0100, Vlastimil Babka wrote:
> When stealing pages from pageblock of a different migratetype, we count how
> many free pages were stolen, and change the pageblock's migratetype if more
> than half of the pageblock was free. This might be too conservative, as there
> might be other pages that are not free, but were allocated with the same
> migratetype as our allocation requested.
> 
> While we cannot determine the migratetype of allocated pages precisely (at
> least without the page_owner functionality enabled), we can count pages that
> compaction would try to isolate for migration - those are either on LRU or
> __PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
> MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
> done as part of free page stealing with little additional overhead.
> 
> The page stealing code is changed so that it considers free pages plus pages
> of the "good" migratetype for the decision whether to change pageblock's
> migratetype.
> 
> The result should be more accurate migratetype of pageblocks wrt the actual
> pages in the pageblocks, when stealing from semi-occupied pageblocks. This
> should help the efficiency of page grouping by mobility.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

That makes sense to me. I have just one nit about the patch:

> @@ -1981,10 +1994,29 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
>  		return;
>  	}
>  
> -	pages = move_freepages_block(zone, page, start_type);
> +	free_pages = move_freepages_block(zone, page, start_type,
> +						&good_pages);
> +	/*
> +	 * good_pages is now the number of movable pages, but if we
> +	 * want UNMOVABLE or RECLAIMABLE allocation, it's more tricky
> +	 */
> +	if (start_type != MIGRATE_MOVABLE) {
> +		/*
> +		 * If we are falling back to MIGRATE_MOVABLE pageblock,
> +		 * treat all non-movable pages as good. If it's UNMOVABLE
> +		 * falling back to RECLAIMABLE or vice versa, be conservative
> +		 * as we can't distinguish the exact migratetype.
> +		 */
> +		old_block_type = get_pageblock_migratetype(page);
> +		if (old_block_type == MIGRATE_MOVABLE)
> +			good_pages = pageblock_nr_pages
> +						- free_pages - good_pages;

This line had me scratch my head for a while, and I think it's mostly
because of the variable naming and the way the comments are phrased.

Could you use a variable called movable_pages to pass to and be filled
in by move_freepages_block?

And instead of good_pages something like starttype_pages or
alike_pages or st_pages or mt_pages or something, to indicate the
number of pages that are comparable to the allocation's migratetype?

> -	/* Claim the whole block if over half of it is free */
> -	if (pages >= (1 << (pageblock_order-1)) ||
> +	/* Claim the whole block if over half of it is free or good type */
> +	if (free_pages + good_pages >= (1 << (pageblock_order-1)) ||
>  			page_group_by_mobility_disabled)
>  		set_pageblock_migratetype(page, start_type);

This would then read

	if (free_pages + alike_pages ...)

which I think would be more descriptive.

The comment leading the entire section following move_freepages_block
could then say something like "If a sufficient number of pages in the
block are either free or of comparable migratability as our
allocation, claim the whole block." Followed by the caveats of how we
determine this migratibility.

Or maybe even the function. The comment above the function seems out
of date after this patch.

[toc] | [prev] | [next] | [standalone]

#1583568 — Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

On 02/14/2017 07:10 PM, Johannes Weiner wrote:
> 
> That makes sense to me. I have just one nit about the patch:
> 
>> @@ -1981,10 +1994,29 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
>>  		return;
>>  	}
>>  
>> -	pages = move_freepages_block(zone, page, start_type);
>> +	free_pages = move_freepages_block(zone, page, start_type,
>> +						&good_pages);
>> +	/*
>> +	 * good_pages is now the number of movable pages, but if we
>> +	 * want UNMOVABLE or RECLAIMABLE allocation, it's more tricky
>> +	 */
>> +	if (start_type != MIGRATE_MOVABLE) {
>> +		/*
>> +		 * If we are falling back to MIGRATE_MOVABLE pageblock,
>> +		 * treat all non-movable pages as good. If it's UNMOVABLE
>> +		 * falling back to RECLAIMABLE or vice versa, be conservative
>> +		 * as we can't distinguish the exact migratetype.
>> +		 */
>> +		old_block_type = get_pageblock_migratetype(page);
>> +		if (old_block_type == MIGRATE_MOVABLE)
>> +			good_pages = pageblock_nr_pages
>> +						- free_pages - good_pages;
> 
> This line had me scratch my head for a while, and I think it's mostly
> because of the variable naming and the way the comments are phrased.
> 
> Could you use a variable called movable_pages to pass to and be filled
> in by move_freepages_block?
> 
> And instead of good_pages something like starttype_pages or
> alike_pages or st_pages or mt_pages or something, to indicate the
> number of pages that are comparable to the allocation's migratetype?
> 
>> -	/* Claim the whole block if over half of it is free */
>> -	if (pages >= (1 << (pageblock_order-1)) ||
>> +	/* Claim the whole block if over half of it is free or good type */
>> +	if (free_pages + good_pages >= (1 << (pageblock_order-1)) ||
>>  			page_group_by_mobility_disabled)
>>  		set_pageblock_migratetype(page, start_type);
> 
> This would then read
> 
> 	if (free_pages + alike_pages ...)
> 
> which I think would be more descriptive.
> 
> The comment leading the entire section following move_freepages_block
> could then say something like "If a sufficient number of pages in the
> block are either free or of comparable migratability as our
> allocation, claim the whole block." Followed by the caveats of how we
> determine this migratibility.
> 
> Or maybe even the function. The comment above the function seems out
> of date after this patch.

I'll incorporate this for the next posting, thanks for the feedback!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web