mm, page_isolation: remove drain_all_pages() in set_migratetype_isolate()

Below is a brief call flow for __offline_pages()

and
alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

Quoted 1 line · On Fri, 14 Dec 2018 10:39:12 +0800 Wei Yang <[email protected]> wrote:

Below is a brief call flow for __offline_pages()

Offtopic...

set_migratetype_isolate() has the comment

	/*
	 * immobile means "not-on-lru" pages. If immobile is larger than
	 * removable-by-driver pages reported by notifier, we'll fail.
	 */

what the heck does that mean? It used to talk about unmovable pages, but this was mysteriously changed to use the unique term "immobile" by Minchan's ee6f509c32 ("mm: factor out memory isolate functions"). Could someone please take a look?

Quoted 16 lines

and
alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

Quoted 3 lines

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

Well. drain_all_pages() moves pages from pcp to buddy so I'm not sure that argument holds water.

Can we step back a bit and ask ourselves what all these draining operations are actually for? What is the intent behind each callsite? Figuring that out (and perhaps even documenting it!) would help us decide the most appropriate places from which to perform the drain.

Quoted 1 line · On Fri, 14 Dec 2018 10:39:12 +0800 Wei Yang <[email protected]> wrote:

Below is a brief call flow for __offline_pages()

Offtopic...

set_migratetype_isolate() has the comment

	/*
	 * immobile means "not-on-lru" pages. If immobile is larger than
	 * removable-by-driver pages reported by notifier, we'll fail.
	 */

what the heck does that mean? It used to talk about unmovable pages, but this was mysteriously changed to use the unique term "immobile" by Minchan's ee6f509c32 ("mm: factor out memory isolate functions"). Could someone please take a look?

Quoted 15 lines

and
alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

Quoted 3 lines

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

Well. drain_all_pages() moves pages from pcp to buddy so I'm not sure that argument holds water.

Can we step back a bit and ask ourselves what all these draining operations are actually for? What is the intent behind each callsite? Figuring that out (and perhaps even documenting it!) would help us decide the most appropriate places from which to perform the drain.

Quoted 38 lines · Andrew Morton

Quoted 1 line · On Fri, 14 Dec 2018 10:39:12 +0800 Wei Yang <[email protected]> wrote:

Below is a brief call flow for __offline_pages()

Offtopic...

set_migratetype_isolate() has the comment

	/*
	 * immobile means "not-on-lru" pages. If immobile is larger than
	 * removable-by-driver pages reported by notifier, we'll fail.
	 */

what the heck does that mean? It used to talk about unmovable pages, but this was mysteriously changed to use the unique term "immobile" by Minchan's ee6f509c32 ("mm: factor out memory isolate functions"). Could someone please take a look?

Quoted 15 lines

and
alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

Quoted 3 lines

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

Quoted 7 lines

Well. drain_all_pages() moves pages from pcp to buddy so I'm not sure that argument holds water.

Can we step back a bit and ask ourselves what all these draining operations are actually for? What is the intent behind each callsite? Figuring that out (and perhaps even documenting it!) would help us decide the most appropriate places from which to perform the drain.

With some rethinking we even could take drain_all_pages() out of the repeat loop. Because after isolation, the page in this range will not be put to pcp pageset. So we just need to drain pages once.

The change may look like this.

mm/memory_hotplug.c +1 -1

index 6910e0eea074..120e9fdfd055 100644
@@ -1590,6 +1590,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
        if (ret)
                goto failed_removal;
 
+       drain_all_pages(zone);
        pfn = start_pfn;
 repeat:
        /* start memory hot removal */

@@ -1599,7 +1600,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
        cond_resched();
        lru_add_drain_all();
-       drain_all_pages(zone);
 
        pfn = scan_movable_pages(start_pfn, end_pfn);
        if (pfn) { /* We have movable pages */

Signature · 3 lines

-- 
Wei Yang
Help you, Help me

Quoted 47 lines · Wei Yang

Quoted 38 lines · Andrew Morton

Quoted 1 line · On Fri, 14 Dec 2018 10:39:12 +0800 Wei Yang <[email protected]> wrote:

Below is a brief call flow for __offline_pages()

Offtopic...

set_migratetype_isolate() has the comment

	/*
	 * immobile means "not-on-lru" pages. If immobile is larger than
	 * removable-by-driver pages reported by notifier, we'll fail.
	 */

what the heck does that mean? It used to talk about unmovable pages, but this was mysteriously changed to use the unique term "immobile" by Minchan's ee6f509c32 ("mm: factor out memory isolate functions"). Could someone please take a look?

Quoted 15 lines

and
alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

Quoted 3 lines

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

Quoted 7 lines

Well. drain_all_pages() moves pages from pcp to buddy so I'm not sure that argument holds water.

Can we step back a bit and ask ourselves what all these draining operations are actually for? What is the intent behind each callsite? Figuring that out (and perhaps even documenting it!) would help us decide the most appropriate places from which to perform the drain.

Quoted 5 lines

With some rethinking we even could take drain_all_pages() out of the repeat loop. Because after isolation, the page in this range will not be put to pcp pageset. So we just need to drain pages once.

The change may look like this.

No, this is incorrect. Draining pcp lists before scan_movable_pages is most likely sub-optimal, because scan_movable_pages will simply ignore pages being on the pcp lists. But we definitely want to drain before we terminate the offlining phase because we do not want to have isolated pages on those lists before we allow the final hotremove.

The way how we retry the migration loop until there is no page in use just guarantees that drain_all_pages is called. If you put it out of the loop then you just break that assumption. Moving drain_all_pages down after the migration is done should work well AFAICS but I didn't really think through all potential side effects nor have time to do so now.

Quoted 24 lines

mm/memory_hotplug.c +1 -1

index 6910e0eea074..120e9fdfd055 100644
@@ -1590,6 +1590,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
        if (ret)
                goto failed_removal;
 
+       drain_all_pages(zone);
        pfn = start_pfn;
 repeat:
        /* start memory hot removal */

@@ -1599,7 +1600,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
        cond_resched();
        lru_add_drain_all();
-       drain_all_pages(zone);
 
        pfn = scan_movable_pages(start_pfn, end_pfn);
        if (pfn) { /* We have movable pages */

Signature · 3 lines

-- 
Wei Yang
Help you, Help me

Signature · 2 lines

Michal Hocko
SUSE Labs

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

Signed-off-by Wei Yang <[email protected]>

---
 mm/page_isolation.c | 2 --
 1 file changed, 2 deletions(-)

mm/page_isolation.c +0 -2

index 43e085608846..f44c0e333bed 100644
@@ -83,8 +83,6 @@ static int set_migratetype_isolate(struct page *page, int migratetype,
 	}
 
 	spin_unlock_irqrestore(&zone->lock, flags);
-	if (!ret)
-		drain_all_pages(zone);
 	return ret;
 }
 

Signature · 1 line

2.15.1

Quoted 15 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

Quoted 3 lines

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

I have to confess I am not sure about the purpose of the draining here. I suspect it is to make sure that pages in the pcp lists really get isolated and if that is the case then it makes sense.

In any case I strongly suggest not touching this code without a very good explanation on why this is not needed. Callers do XYZ is not a proper explanation because assumes that all callers will know that this has to be done. So either we really need to drain and then it is better to make it here or we don't but that requires some explanation.

Quoted 24 lines · Michal Hocko

Quoted 15 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

Quoted 3 lines

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

I have to confess I am not sure about the purpose of the draining here. I suspect it is to make sure that pages in the pcp lists really get isolated and if that is the case then it makes sense.

Quoted 6 lines

In any case I strongly suggest not touching this code without a very good explanation on why this is not needed. Callers do XYZ is not a proper explanation because assumes that all callers will know that this has to be done. So either we really need to drain and then it is better to make it here or we don't but that requires some explanation.

Yep, let me try to explain what is trying to do.

Based on my understanding, online_pages do two things

* adjust zone/pgdat status
* put pages into Buddy

Generally, offline_pages do the reverse

* take pages out of Buddy
* adjust zone/pgdat status

While it is not that easy to take pages out of Buddy, since pages are

* pcp list
* slub
* other usage

This means before taking a page out of Buddy, we need to return it first to Buddy.

Current implementation is interesting by introducing migrate type. By setting migrate type to MIGRATE_ISOLATE, this range of pages will never be allocated from Buddy. And every page returned in this range will never be touched by Buddy.

Function start_isolate_page_range() just do this.

Then let's focus on the pcp list. This is a little bit different than other allocated pages. These are actually "partially" allocated pages. They are not counted in Buddy Free pages, either no real use. So we have two choice to get back those pages:

* wait until it is allocated to a real user and wait for return
* or drain them directly

Current implementation take 2nd approach.

Then we can see there are also two way to drain them:

* drain them range by range
* drain them in a whole range

Both looks good, but not necessary to do them both. Because after we set a pageblock migrate type to MIGRATE_ISOLATE, pages in this range will never be allocated nor be put on pcp list. So after we drain one particular range, it is not necessary to drain this range again.

The reason why I choose to drain them in a whole range is current drain_all_pages() just carry zone information. For example, a zone may have 1G while a pageblock is 128M. The pageblock is 1/8 of this zone. This means in case there are 8 pages on pcp list, only 1 page drained by drain_all_pages belongs to this pageblock. But we drain other 7 healthy pages.

CPU1 pcp list CPU2 pcp list

 +---------------+                        +---------------+ 
 |A1  B3  C8  F6 |                        |E1  G3  D8  B6 |
 +---------------+                        +---------------+

A B C D E F G

  +---------+---------+---------+---------+---------+---------+---------+
  |012345678|         |         |         |         |         |         |
  +---------+---------+---------+---------+---------+---------+---------+
                                |<-pgblk->|
  |<-                              Zone                               ->|

This is a chart for illustration. In case we want to isolate pgblk D, while zone pcp list has 8 pages and only one belongs to this pgblk D. This means the drain on pgblk base has much side effect. And with one drain on each pgblk, this may increase the contention on this zone.

Well, another approach is to enable drain_all_pages() with exact range information. But neither approach needs to do them both.

Quoted 72 lines · Wei Yang

Quoted 24 lines · Michal Hocko

Quoted 15 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Since set_migratetype_isolate() is only used in start_isolate_page_range(), which is just used in __offline_pages() and alloc_contig_range(). And both of them call drain_all_pages() if every check looks good. This means it is not necessary call drain_all_pages() in each iteration of set_migratetype_isolate().

Quoted 3 lines

By doing so, the logic seems a little bit clearer. set_migratetype_isolate() handles pages in Buddy, while drain_all_pages() takes care of pages in pcp.

I have to confess I am not sure about the purpose of the draining here. I suspect it is to make sure that pages in the pcp lists really get isolated and if that is the case then it makes sense.

Quoted 6 lines

In any case I strongly suggest not touching this code without a very good explanation on why this is not needed. Callers do XYZ is not a proper explanation because assumes that all callers will know that this has to be done. So either we really need to drain and then it is better to make it here or we don't but that requires some explanation.

Yep, let me try to explain what is trying to do.

Based on my understanding, online_pages do two things

* adjust zone/pgdat status
* put pages into Buddy

Generally, offline_pages do the reverse

* take pages out of Buddy
* adjust zone/pgdat status

While it is not that easy to take pages out of Buddy, since pages are

* pcp list
* slub
* other usage

This means before taking a page out of Buddy, we need to return it first to Buddy.

Current implementation is interesting by introducing migrate type. By setting migrate type to MIGRATE_ISOLATE, this range of pages will never be allocated from Buddy. And every page returned in this range will never be touched by Buddy.

Function start_isolate_page_range() just do this.

Then let's focus on the pcp list. This is a little bit different than other allocated pages. These are actually "partially" allocated pages. They are not counted in Buddy Free pages, either no real use. So we have two choice to get back those pages:

* wait until it is allocated to a real user and wait for return
* or drain them directly

Current implementation take 2nd approach.

Then we can see there are also two way to drain them:

Quoted 7 lines

* drain them range by range
* drain them in a whole range

Both looks good, but not necessary to do them both. Because after we set a pageblock migrate type to MIGRATE_ISOLATE, pages in this range will never be allocated nor be put on pcp list. So after we drain one particular range, it is not necessary to drain this range again.

OK, this is an important point and actually the argument that i am wrong. I have missed that free_unref_page_commit skips pcp lists for MIGRATE_ISOLATE (resp. all migrate types above MIGRATE_PCPTYPES). Then you are right that we are OK to drain the zone only once _after_ we have isolated the full range.

So please send a new patch with this clarification in the changelog and I will ack it.

Quoted 27 lines

The reason why I choose to drain them in a whole range is current drain_all_pages() just carry zone information. For example, a zone may have 1G while a pageblock is 128M. The pageblock is 1/8 of this zone. This means in case there are 8 pages on pcp list, only 1 page drained by drain_all_pages belongs to this pageblock. But we drain other 7 healthy pages.

CPU1 pcp list CPU2 pcp list

 +---------------+                        +---------------+ 
 |A1  B3  C8  F6 |                        |E1  G3  D8  B6 |
 +---------------+                        +---------------+

A B C D E F G

  +---------+---------+---------+---------+---------+---------+---------+
  |012345678|         |         |         |         |         |         |
  +---------+---------+---------+---------+---------+---------+---------+
                                |<-pgblk->|
  |<-                              Zone                               ->|

This is a chart for illustration. In case we want to isolate pgblk D, while zone pcp list has 8 pages and only one belongs to this pgblk D. This means the drain on pgblk base has much side effect. And with one drain on each pgblk, this may increase the contention on this zone.

Quoted 2 lines

Well, another approach is to enable drain_all_pages() with exact range information. But neither approach needs to do them both.

Is this actually worth the additional complexity? Have you seen an actual workload that would benefit from that?

Signature · 2 lines

Michal Hocko
SUSE Labs

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

Since drain_all_pages() is zone based, by reduce times of drain_all_pages() also reduce some contention on this particular zone.

Signed-off-by Wei Yang <[email protected]>

---
v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range

---
 mm/page_isolation.c | 2 --
 1 file changed, 2 deletions(-)

mm/page_isolation.c +0 -2

index 43e085608846..f44c0e333bed 100644
@@ -83,8 +83,6 @@ static int set_migratetype_isolate(struct page *page, int migratetype,
 	}
 
 	spin_unlock_irqrestore(&zone->lock, flags);
-	if (!ret)
-		drain_all_pages(zone);
 	return ret;
 }
 

Quoted 20 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

Since drain_all_pages() is zone based, by reduce times of drain_all_pages() also reduce some contention on this particular zone.

Quoted 1 line

Signed-off-by Wei Yang <[email protected]>

Yes, as far as I can see, when a MIGRATE_ISOLATE page gets freed, it will not go onto the pcp list again.

However, start_isolate_page_range() is also called via alloc_contig_range(). Are you sure we can effectively drop the drain_all_pages() for that call path?

Quoted 21 lines

---
v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range

---
 mm/page_isolation.c | 2 --
 1 file changed, 2 deletions(-)

mm/page_isolation.c +0 -2

index 43e085608846..f44c0e333bed 100644
@@ -83,8 +83,6 @@ static int set_migratetype_isolate(struct page *page, int migratetype,
 	}
 
 	spin_unlock_irqrestore(&zone->lock, flags);
-	if (!ret)
-		drain_all_pages(zone);
 	return ret;
 }
 

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 21 lines · David Hildenbrand

Quoted 20 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

Since drain_all_pages() is zone based, by reduce times of drain_all_pages() also reduce some contention on this particular zone.

Quoted 9 lines

Quoted 1 line

Signed-off-by Wei Yang <[email protected]>

Yes, as far as I can see, when a MIGRATE_ISOLATE page gets freed, it will not go onto the pcp list again.

However, start_isolate_page_range() is also called via alloc_contig_range(). Are you sure we can effectively drop the drain_all_pages() for that call path?

alloc_contig_range() does following now:

- isolate page range
- do reclaim and migration
- drain lru
- drain pcp list

If step 2 fails, it will not drain lru and pcp list.

I don't see we have to drain pcp list before step 2. And after this change, it will save some effort if step 2 fails.

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

Since drain_all_pages() is zone based, by reduce times of drain_all_pages() also reduce some contention on this particular zone.

Signed-off-by Wei Yang <[email protected]>

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

Quoted 13 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

Quoted 3 lines

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages().

Quoted 14 lines · Michal Hocko

Quoted 13 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

Quoted 6 lines

Quoted 3 lines

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages().

I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves the pages to a new list:

<--
list_move(&page->lru,

			  &zone->free_area[order].free_list[migratetype]);

-->

But looking at it again, I see that this is only for BuddyPages, so I guess that pcp-pages do not really get unlinked, so we could still allocate them.

Uhmf, I missed that.

Signature · 2 lines

Oscar Salvador
SUSE L3

Quoted 25 lines · Oscar Salvador

Quoted 14 lines · Michal Hocko

Quoted 13 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

Quoted 6 lines

Quoted 3 lines

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages().

I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves the pages to a new list:

Quoted 8 lines

<--
list_move(&page->lru,

			  &zone->free_area[order].free_list[migratetype]);

-->

But looking at it again, I see that this is only for BuddyPages, so I guess that pcp-pages do not really get unlinked, so we could still allocate them.

Well, I think you are right. But with this in mind, current code looks buggy.

Between has_unmovable_pages() and drain_all_pages(), others still could allocate pages on pcp list, right? This means we thought we have isolated the range, but not.

Quoted 38 lines · Wei Yang

Quoted 25 lines · Oscar Salvador

Quoted 14 lines · Michal Hocko

Quoted 13 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

Quoted 6 lines

Quoted 3 lines

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages().

I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves the pages to a new list:

Quoted 8 lines

<--
list_move(&page->lru,

			  &zone->free_area[order].free_list[migratetype]);

-->

But looking at it again, I see that this is only for BuddyPages, so I guess that pcp-pages do not really get unlinked, so we could still allocate them.

Well, I think you are right. But with this in mind, current code looks buggy.

Quoted 3 lines

Between has_unmovable_pages() and drain_all_pages(), others still could allocate pages on pcp list, right? This means we thought we have isolated the range, but not.

THere is no guarantee in that regards and I believe there is also no demand for such a strong semantic. Or I do not see it at least.

Signature · 2 lines

Michal Hocko
SUSE Labs

Then I am confused about the objection to this patch. Finally, we drain all the pages in pcp list and the range is isolated.

On Wed 19-12-18 14:33:27, Wei Yang wrote:
[...]

Quoted 2 lines

Then I am confused about the objection to this patch. Finally, we drain all the pages in pcp list and the range is isolated.

Please read my emails more carefully. As I've said, the only reason to do care about draining is to remove it from where it doesn't belong.

Signature · 3 lines

-- 
Michal Hocko
SUSE Labs

Quoted 7 lines · Michal Hocko

On Wed 19-12-18 14:33:27, Wei Yang wrote:
[...]

Quoted 2 lines

Then I am confused about the objection to this patch. Finally, we drain all the pages in pcp list and the range is isolated.

Please read my emails more carefully. As I've said, the only reason to do care about draining is to remove it from where it doesn't belong.

I go through the thread again and classify two main opinions from you and Oscar.

1) We can still allocate pages in a specific range from pcp list even we have already isolate this range.
2) We shouldn't rely on caller to drain pages and set_migratetype_isolate() may handle a range cross zones.

I understand the second one and agree it is not proper to rely on caller and make the assumption on range for set_migratetype_isolate().

My confusion comes from the first one. As you and Oscar both mentioned this and Oscar said "I had the same fear", this makes me think current implementation is buggy. But your following reply said this is not. This means current approach works fine.

If the above understanding is correct, and combining with previous discussion, the improvement we can do is to remove the drain_all_pages() in __offline_pages()/alloc_contig_range(). By doing so, the pcp list drain doesn't rely on caller and the isolation/drain on each pageblock ensures pcp list will not contain any page in this range now and future. This imply the drain_all_pages() in __offline_pages()/alloc_contig_range() is not necessary.

Is my understanding correct?

Quoted 33 lines · Wei Yang

Quoted 7 lines · Michal Hocko

On Wed 19-12-18 14:33:27, Wei Yang wrote:
[...]

Quoted 2 lines

Then I am confused about the objection to this patch. Finally, we drain all the pages in pcp list and the range is isolated.

Please read my emails more carefully. As I've said, the only reason to do care about draining is to remove it from where it doesn't belong.

I go through the thread again and classify two main opinions from you and Oscar.

1) We can still allocate pages in a specific range from pcp list even we have already isolate this range.
2) We shouldn't rely on caller to drain pages and set_migratetype_isolate() may handle a range cross zones.

I understand the second one and agree it is not proper to rely on caller and make the assumption on range for set_migratetype_isolate().

My confusion comes from the first one. As you and Oscar both mentioned this and Oscar said "I had the same fear", this makes me think current implementation is buggy. But your following reply said this is not. This means current approach works fine.

If the above understanding is correct, and combining with previous discussion, the improvement we can do is to remove the drain_all_pages() in __offline_pages()/alloc_contig_range(). By doing so, the pcp list drain doesn't rely on caller and the isolation/drain on each pageblock ensures pcp list will not contain any page in this range now and future. This imply the drain_all_pages() in __offline_pages()/alloc_contig_range() is not necessary.

Quoted 1 line

Is my understanding correct?

Yes

Signature · 3 lines

-- 
Michal Hocko
SUSE Labs

Quoted 9 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Quoted 7 lines

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages().

One thing that I really do not like about this patch (and I believe I have mentioned that previously) that you rely on callers to do the right thing. The proper fix would be to do the draining in start_isolate_page_range and remove them from callers. Also what does

prevent start_isolate_page_range to work on multiple zones? At least contiguous allocator can do that in principle.

So no I do not like this patch, it is not an improvement.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 18 lines · Michal Hocko

Quoted 9 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Quoted 7 lines

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

Quoted 7 lines

But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages().

One thing that I really do not like about this patch (and I believe I have mentioned that previously) that you rely on callers to do the right thing. The proper fix would be to do the draining in start_isolate_page_range and remove them from callers. Also what does

Well, I don't really understand this meaning previously.

So you prefer set_migratetype_isolate() do the drain instead of the caller (__offline_pages) do the drain. Is my understanding correct?

Quoted 2 lines

prevent start_isolate_page_range to work on multiple zones? At least contiguous allocator can do that in principle.

As the comment mentioned, in current implementation the range must be in one zone.

Quoted 27 lines · Wei Yang

Quoted 18 lines · Michal Hocko

Quoted 9 lines · Wei Yang

Below is a brief call flow for __offline_pages() and alloc_contig_range():

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages()

Quoted 7 lines

Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range.

The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list.

Quoted 7 lines

But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages().

One thing that I really do not like about this patch (and I believe I have mentioned that previously) that you rely on callers to do the right thing. The proper fix would be to do the draining in start_isolate_page_range and remove them from callers. Also what does

Quoted 4 lines

Well, I don't really understand this meaning previously.

So you prefer set_migratetype_isolate() do the drain instead of the caller (__offline_pages) do the drain. Is my understanding correct?

Either set_migratetype_isolate or start_isolate_page_range. The later only if this is guaranteed that we cannot intemix zones in the range.

Quoted 5 lines

Quoted 2 lines

prevent start_isolate_page_range to work on multiple zones? At least contiguous allocator can do that in principle.

As the comment mentioned, in current implementation the range must be in one zone.

I do not see anything like that documented for set_migratetype_isolate.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 1 line · Michal Hocko

On Wed 19-12-18 13:29:34, Wei Yang wrote:

Quoted 4 lines

Quoted 2 lines

As the comment mentioned, in current implementation the range must be in one zone.

I do not see anything like that documented for set_migratetype_isolate.

The comment is not on set_migratetype_isolate, but for its two (grandparent) callers:

__offline_pages
alloc_contig_range

Quoted 2 lines · Wei Yang

Quoted 1 line · Michal Hocko

On Wed 19-12-18 13:29:34, Wei Yang wrote:

[...]

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

As the comment mentioned, in current implementation the range must be in one zone.

I do not see anything like that documented for set_migratetype_isolate.

The comment is not on set_migratetype_isolate, but for its two (grandparent) callers:

Quoted 2 lines

__offline_pages
alloc_contig_range

But those are consumers while the main api here is start_isolate_page_range. What happens if we grow a new user? Go over the same problems? See the difference?

Please try to look at these things from a higher level. We really do not want micro optimise on behalf of a sane API. Unless there is a very good reason to do that - e.g. when the performance difference is really huge.

Signature · 2 lines

Michal Hocko
SUSE Labs

Since drain_all_pages() is zone based, by reduce times of drain_all_pages() also reduce some contention on this particular zone.

From this snippet we can see current logic is isolate and drain pcp list

In current implementation, there are two places to isolate a range of
page: __offline_pages() and alloc_contig_range(). During this procedure, it will drain pages on pcp list.

Below is a brief call flow:

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages() <--- A

Quoted 1 line

From this snippet we can see current logic is isolate and drain pcp list

for each pageblock and drain pcp list again for the whole range.

While the drain at A is not necessary. The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list. This means the procedure to drain pages on pcp list after start_isolate_page_range() will not drain any page in the target range.

Signed-off-by Wei Yang <[email protected]>

---
v3:
* it is not proper to rely on caller to drain pages, so keep to drain pages during iteration and remove the one in callers.
v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range

---
 mm/memory_hotplug.c | 1 -
 mm/page_alloc.c     | 1 -
 2 files changed, 2 deletions(-)

mm/memory_hotplug.c +0 -1

index 6910e0eea074..d2fa6cbbb2db 100644
@@ -1599,7 +1599,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
 	cond_resched();
 	lru_add_drain_all();
-	drain_all_pages(zone);
 
 	pfn = scan_movable_pages(start_pfn, end_pfn);
 	if (pfn) { /* We have movable pages */

mm/page_alloc.c +0 -1

index f1edd36a1e2b..d9ee4bb3a1a7 100644
@@ -8041,7 +8041,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
 	 */
 
 	lru_add_drain_all();
-	drain_all_pages(cc.zone);
 
 	order = 0;
 	outer_start = start;

Signature · 1 line

2.15.1

Quoted 13 lines · Wei Yang

In current implementation, there are two places to isolate a range of
page: __offline_pages() and alloc_contig_range(). During this procedure, it will drain pages on pcp list.

Below is a brief call flow:

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages() <--- A

Quoted 1 line

From this snippet we can see current logic is isolate and drain pcp list

Quoted 8 lines

for each pageblock and drain pcp list again for the whole range.

While the drain at A is not necessary. The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list. This means the procedure to drain pages on pcp list after start_isolate_page_range() will not drain any page in the target range.

I am still not happy with the changelog. I would suggest the following instead

"
start_isolate_page_range is responsible for isolating the given pfn range. One part of that job is to make sure that also pages that are on the allocator pcp lists are properly isolated. Otherwise they could be reused and the range wouldn't be completely isolated until the memory is freed back. While there is no strict guarantee here because pages might get allocated at any time before drain_all_pages is called there doesn't seem to be any strong demand for such a guarantee.

In any case, draining is already done at the isolation level and there is no need to do it again later by start_isolate_page_range callers (memory hotplug and CMA allocator currently). Therefore remove pointless draining in existing callers to make the code more clear and functionally correct.
"

Quoted 1 line

Signed-off-by Wei Yang <[email protected]>

With something like that, you can add

Acked-by Michal Hocko <[email protected]>

From this snippet we can see current logic is isolate and drain pcp list

In current implementation, there are two places to isolate a range of
page: __offline_pages() and alloc_contig_range(). During this procedure, it will drain pages on pcp list.

Below is a brief call flow:

__offline_pages()/alloc_contig_range()
start_isolate_page_range()
set_migratetype_isolate()
drain_all_pages()
drain_all_pages() <--- A

From this snippet we can see current logic is isolate and drain pcp list for each pageblock and drain pcp list again for the whole range.

start_isolate_page_range is responsible for isolating the given pfn range. One part of that job is to make sure that also pages that are on the allocator pcp lists are properly isolated. Otherwise they could be reused and the range wouldn't be completely isolated until the memory is freed back. While there is no strict guarantee here because pages might get allocated at any time before drain_all_pages is called there doesn't seem to be any strong demand for such a guarantee.

In any case, draining is already done at the isolation level and there is no need to do it again later by start_isolate_page_range callers (memory hotplug and CMA allocator currently). Therefore remove pointless draining in existing callers to make the code more clear and functionally correct.

[[email protected]: provide a clearer changelog for the last two paragraph]

Signed-off-by Wei Yang <[email protected]>

Acked-by Michal Hocko <[email protected]>

---
v4:
* adjust last two paragraph changelog from Michal's comment
v3:
* it is not proper to rely on caller to drain pages, so keep to drain pages during iteration and remove the one in callers.
v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range

---
 mm/memory_hotplug.c | 1 -
 mm/page_alloc.c     | 1 -
 2 files changed, 2 deletions(-)

mm/memory_hotplug.c +0 -1

index 6910e0eea074..d2fa6cbbb2db 100644
@@ -1599,7 +1599,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
 	cond_resched();
 	lru_add_drain_all();
-	drain_all_pages(zone);
 
 	pfn = scan_movable_pages(start_pfn, end_pfn);
 	if (pfn) { /* We have movable pages */

mm/page_alloc.c +0 -1

index f1edd36a1e2b..d9ee4bb3a1a7 100644
@@ -8041,7 +8041,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
 	 */
 
 	lru_add_drain_all();
-	drain_all_pages(cc.zone);
 
 	order = 0;
 	outer_start = start;

Signed-off-by Wei Yang <[email protected]>

Acked-by Michal Hocko <[email protected]>

---
v4:
* adjust last two paragraph changelog from Michal's comment
v3:
* it is not proper to rely on caller to drain pages, so keep to drain pages during iteration and remove the one in callers.
v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range

---
 mm/memory_hotplug.c | 1 -
 mm/page_alloc.c     | 1 -
 2 files changed, 2 deletions(-)

mm/memory_hotplug.c +0 -1

index 6910e0eea074..d2fa6cbbb2db 100644
@@ -1599,7 +1599,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
 	cond_resched();
 	lru_add_drain_all();
-	drain_all_pages(zone);
 
 	pfn = scan_movable_pages(start_pfn, end_pfn);
 	if (pfn) { /* We have movable pages */

mm/page_alloc.c +0 -1

index f1edd36a1e2b..d9ee4bb3a1a7 100644
@@ -8041,7 +8041,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
 	 */
 
 	lru_add_drain_all();
-	drain_all_pages(cc.zone);
 
 	order = 0;
 	outer_start = start;

Signature · 1 line

2.15.1