Skip to contentSkip to conversation

mm, memory_hotplug: check zone_movable in has_unmovable_pages

PATCH In review Michal Hocko 4 participants 12 messages Nov 6, 2018 – Nov 15, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mm, memory_hotplug: check zone_movable in has_unmovable_pages

From: Michal Hocko <[email protected]>

Page state checks are racy. Under a heavy memory workload (e.g. stress -m 200 -t 2h) it is quite easy to hit a race window when the page is allocated but its state is not fully populated yet. A debugging patch to dump the struct page state shows

: [  476.575516] has_unmovable_pages: pfn:0x10dfec00, found:0x1, count:0x0
: [  476.582103] page:ffffea0437fb0000 count:1 mapcount:1 mapping:ffff880e05239841 index:0x7f26e5000 compound_mapcount: 1
: [  476.592645] flags: 0x5fffffc0090034(uptodate|lru|active|head|swapbacked)

Note that the state has been checked for both PageLRU and PageSwapBacked already. Closing this race completely would require some sort of retry logic. This can be tricky and error prone (think of potential endless or long taking loops).

Workaround this problem for movable zones at least. Such a zone should only contain movable pages. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust") has told us that this is not strictly true though. Bootmem pages should be marked reserved though so we can move the original check after the PageReserved check. Pages from other zones are still prone to races but we even do not pretend that memory hotremove works for those so pre-mature failure doesn't hurt that much.

Reported-and-tested-by Baoquan He <[email protected]>
Fixes "mm, memory_hotplug: make has_unmovable_pages more robust")

---

Hi,
this has been reported [1] and we have tried multiple things to address the issue. The only reliable way was to reintroduce the movable zone check into has_unmovable_pages. This time it should be safe also for the bug originally fixed by 15c30bc09085.

[1] http://lkml.kernel.org/r/20181101091055.GA15166@MiWiFi-R3L-srv

 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/page_alloc.c +8 -0
index 863d46da6586..c6d900ee4982 100644
@@ -7788,6 +7788,14 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 		if (PageReserved(page))
 			goto unmovable;
 
+		/*
+		 * If the zone is movable and we have ruled out all reserved
+		 * pages then it should be reasonably safe to assume the rest
+		 * is movable.
+		 */
+		if (zone_idx(zone) == ZONE_MOVABLE)
+			continue;
+
 		/*
 		 * Hugepages are not in LRU lists, but they're movable.
 		 * We need not scan over tail pages bacause we don't
Signature · 1 line
2.19.1
#2 lore raw
In reply to Michal Hocko
Quoted 46 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Page state checks are racy. Under a heavy memory workload (e.g. stress -m 200 -t 2h) it is quite easy to hit a race window when the page is allocated but its state is not fully populated yet. A debugging patch to dump the struct page state shows

: [  476.575516] has_unmovable_pages: pfn:0x10dfec00, found:0x1, count:0x0
: [  476.582103] page:ffffea0437fb0000 count:1 mapcount:1 mapping:ffff880e05239841 index:0x7f26e5000 compound_mapcount: 1
: [  476.592645] flags: 0x5fffffc0090034(uptodate|lru|active|head|swapbacked)

Note that the state has been checked for both PageLRU and PageSwapBacked already. Closing this race completely would require some sort of retry logic. This can be tricky and error prone (think of potential endless or long taking loops).

Workaround this problem for movable zones at least. Such a zone should only contain movable pages. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust") has told us that this is not strictly true though. Bootmem pages should be marked reserved though so we can move the original check after the PageReserved check. Pages from other zones are still prone to races but we even do not pretend that memory hotremove works for those so pre-mature failure doesn't hurt that much.

Reported-and-tested-by Baoquan He <[email protected]>
Fixes "mm, memory_hotplug: make has_unmovable_pages more robust")

---

Hi,
this has been reported [1] and we have tried multiple things to address the issue. The only reliable way was to reintroduce the movable zone check into has_unmovable_pages. This time it should be safe also for the bug originally fixed by 15c30bc09085.

[1] http://lkml.kernel.org/r/20181101091055.GA15166@MiWiFi-R3L-srv

 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/page_alloc.c +0 -0
index 863d46da6586..c6d900ee4982 100644
@@ -7788,6 +7788,14 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 		if (PageReserved(page))
 			goto unmovable;

mm/page_alloc.c bool has_unmovable_pages(struct zone *zone, struct page *page, int count, · line 7799

Quoted 10 lines
mm/page_alloc.c +8 -0
 
+		/*
+		 * If the zone is movable and we have ruled out all reserved
+		 * pages then it should be reasonably safe to assume the rest
+		 * is movable.
+		 */
+		if (zone_idx(zone) == ZONE_MOVABLE)
+			continue;
+
 		/*

There is a WARN_ON() in case of failure at the end of the routine, is that triggered when we hit the bug? If we're adding this patch, the WARN_ON needs to go as well.

The check seems to be quite aggressive and in a loop that iterates pages, but has nothing to do with the page, did you mean to make the check

zone_idx(page_zone(page)) == ZONE_MOVABLE

it also skips all checks for pinned pages and other checks

Balbir Singh.

#3 lore raw
Quoted 48 lines · Balbir Singh
Quoted 47 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Page state checks are racy. Under a heavy memory workload (e.g. stress -m 200 -t 2h) it is quite easy to hit a race window when the page is allocated but its state is not fully populated yet. A debugging patch to dump the struct page state shows

: [  476.575516] has_unmovable_pages: pfn:0x10dfec00, found:0x1, count:0x0
: [  476.582103] page:ffffea0437fb0000 count:1 mapcount:1 mapping:ffff880e05239841 index:0x7f26e5000 compound_mapcount: 1
: [  476.592645] flags: 0x5fffffc0090034(uptodate|lru|active|head|swapbacked)

Note that the state has been checked for both PageLRU and PageSwapBacked already. Closing this race completely would require some sort of retry logic. This can be tricky and error prone (think of potential endless or long taking loops).

Workaround this problem for movable zones at least. Such a zone should only contain movable pages. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust") has told us that this is not strictly true though. Bootmem pages should be marked reserved though so we can move the original check after the PageReserved check. Pages from other zones are still prone to races but we even do not pretend that memory hotremove works for those so pre-mature failure doesn't hurt that much.

Reported-and-tested-by Baoquan He <[email protected]>
Fixes "mm, memory_hotplug: make has_unmovable_pages more robust")

---

Hi,
this has been reported [1] and we have tried multiple things to address the issue. The only reliable way was to reintroduce the movable zone check into has_unmovable_pages. This time it should be safe also for the bug originally fixed by 15c30bc09085.

[1] http://lkml.kernel.org/r/20181101091055.GA15166@MiWiFi-R3L-srv

 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/page_alloc.c +0 -0
index 863d46da6586..c6d900ee4982 100644
@@ -7788,6 +7788,14 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 		if (PageReserved(page))
 			goto unmovable;
 
Quoted 14 lines

mm/page_alloc.c bool has_unmovable_pages(struct zone *zone, struct page *page, int count, · line 7799

Quoted 9 lines
mm/page_alloc.c +8 -0
+		/*
+		 * If the zone is movable and we have ruled out all reserved
+		 * pages then it should be reasonably safe to assume the rest
+		 * is movable.
+		 */
+		if (zone_idx(zone) == ZONE_MOVABLE)
+			continue;
+
 		/*

There is a WARN_ON() in case of failure at the end of the routine, is that triggered when we hit the bug? If we're adding this patch, the WARN_ON needs to go as well.

No the warning should stay in case we encounter reserved pages in zone movable.

Quoted 5 lines

The check seems to be quite aggressive and in a loop that iterates pages, but has nothing to do with the page, did you mean to make the check

zone_idx(page_zone(page)) == ZONE_MOVABLE

Does it make any difference? Can we actually encounter a page from a different zone here?

Quoted 1 line

it also skips all checks for pinned pages and other checks

Yes, this is intentional and the comment tries to explain why. I wish we could be add a more specific checks for movable pages - e.g. detect long term pins that would prevent migration - but we do not have any facility for that. Please note that the worst case of a false positive is a repeated migration failure and user has a way to break out of migration by a signal.

Signature · 2 lines
Michal Hocko
SUSE Labs
#4 lore raw
Quoted 64 lines · Michal Hocko
Quoted 48 lines · Balbir Singh
Quoted 47 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Page state checks are racy. Under a heavy memory workload (e.g. stress -m 200 -t 2h) it is quite easy to hit a race window when the page is allocated but its state is not fully populated yet. A debugging patch to dump the struct page state shows

: [  476.575516] has_unmovable_pages: pfn:0x10dfec00, found:0x1, count:0x0
: [  476.582103] page:ffffea0437fb0000 count:1 mapcount:1 mapping:ffff880e05239841 index:0x7f26e5000 compound_mapcount: 1
: [  476.592645] flags: 0x5fffffc0090034(uptodate|lru|active|head|swapbacked)

Note that the state has been checked for both PageLRU and PageSwapBacked already. Closing this race completely would require some sort of retry logic. This can be tricky and error prone (think of potential endless or long taking loops).

Workaround this problem for movable zones at least. Such a zone should only contain movable pages. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust") has told us that this is not strictly true though. Bootmem pages should be marked reserved though so we can move the original check after the PageReserved check. Pages from other zones are still prone to races but we even do not pretend that memory hotremove works for those so pre-mature failure doesn't hurt that much.

Reported-and-tested-by Baoquan He <[email protected]>
Fixes "mm, memory_hotplug: make has_unmovable_pages more robust")

---

Hi,
this has been reported [1] and we have tried multiple things to address the issue. The only reliable way was to reintroduce the movable zone check into has_unmovable_pages. This time it should be safe also for the bug originally fixed by 15c30bc09085.

[1] http://lkml.kernel.org/r/20181101091055.GA15166@MiWiFi-R3L-srv

 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/page_alloc.c +0 -0
index 863d46da6586..c6d900ee4982 100644
@@ -7788,6 +7788,14 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 		if (PageReserved(page))
 			goto unmovable;
 
Quoted 14 lines

mm/page_alloc.c bool has_unmovable_pages(struct zone *zone, struct page *page, int count, · line 7799

Quoted 9 lines
mm/page_alloc.c +8 -0
+		/*
+		 * If the zone is movable and we have ruled out all reserved
+		 * pages then it should be reasonably safe to assume the rest
+		 * is movable.
+		 */
+		if (zone_idx(zone) == ZONE_MOVABLE)
+			continue;
+
 		/*

There is a WARN_ON() in case of failure at the end of the routine, is that triggered when we hit the bug? If we're adding this patch, the WARN_ON needs to go as well.

Quoted 2 lines

No the warning should stay in case we encounter reserved pages in zone movable.

And to clarify. I am OK with changing the WARN to pr_warn if the warning is considered harmful but we do want to note that something unexpected is going on here.

Signature · 2 lines
Michal Hocko
SUSE Labs
#5 lore raw
In reply to Michal Hocko
Quoted 9 lines · Michal Hocko
Quoted 5 lines · Balbir Singh

The check seems to be quite aggressive and in a loop that iterates pages, but has nothing to do with the page, did you mean to make the check

zone_idx(page_zone(page)) == ZONE_MOVABLE

Does it make any difference? Can we actually encounter a page from a different zone here?

AFAIK, test_pages_in_a_zone() called from offline_pages() should ensure that the range belongs to a unique zone, so we should not encounter pages from other zones there, right?

---
Oscar
Suse L3

#6 lore raw
Quoted 6 lines · osalvador
Quoted 5 lines · Michal Hocko
Quoted 4 lines · Balbir Singh

The check seems to be quite aggressive and in a loop that iterates pages, but has nothing to do with the page, did you mean to make the check

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

zone_idx(page_zone(page)) == ZONE_MOVABLE

Does it make any difference? Can we actually encounter a page from a different zone here?

AFAIK, test_pages_in_a_zone() called from offline_pages() should ensure that the range belongs to a unique zone, so we should not encounter pages from other zones there, right?

Yes that is the case for memory hotplug. We do assume a single zone at set_migratetype_isolate where we take the zone->lock. If the contig_alloc can span multiple zones then it should check for similar.

Signature · 2 lines
Michal Hocko
SUSE Labs
#7 lore raw
In reply to Michal Hocko
Quoted 64 lines · Michal Hocko
Quoted 48 lines · Balbir Singh
Quoted 47 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Page state checks are racy. Under a heavy memory workload (e.g. stress -m 200 -t 2h) it is quite easy to hit a race window when the page is allocated but its state is not fully populated yet. A debugging patch to dump the struct page state shows

: [  476.575516] has_unmovable_pages: pfn:0x10dfec00, found:0x1, count:0x0
: [  476.582103] page:ffffea0437fb0000 count:1 mapcount:1 mapping:ffff880e05239841 index:0x7f26e5000 compound_mapcount: 1
: [  476.592645] flags: 0x5fffffc0090034(uptodate|lru|active|head|swapbacked)

Note that the state has been checked for both PageLRU and PageSwapBacked already. Closing this race completely would require some sort of retry logic. This can be tricky and error prone (think of potential endless or long taking loops).

Workaround this problem for movable zones at least. Such a zone should only contain movable pages. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust") has told us that this is not strictly true though. Bootmem pages should be marked reserved though so we can move the original check after the PageReserved check. Pages from other zones are still prone to races but we even do not pretend that memory hotremove works for those so pre-mature failure doesn't hurt that much.

Reported-and-tested-by Baoquan He <[email protected]>
Fixes "mm, memory_hotplug: make has_unmovable_pages more robust")

---

Hi,
this has been reported [1] and we have tried multiple things to address the issue. The only reliable way was to reintroduce the movable zone check into has_unmovable_pages. This time it should be safe also for the bug originally fixed by 15c30bc09085.

[1] http://lkml.kernel.org/r/20181101091055.GA15166@MiWiFi-R3L-srv

 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/page_alloc.c +0 -0
index 863d46da6586..c6d900ee4982 100644
@@ -7788,6 +7788,14 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 		if (PageReserved(page))
 			goto unmovable;
 
Quoted 14 lines

mm/page_alloc.c bool has_unmovable_pages(struct zone *zone, struct page *page, int count, · line 7799

Quoted 9 lines
mm/page_alloc.c +8 -0
+		/*
+		 * If the zone is movable and we have ruled out all reserved
+		 * pages then it should be reasonably safe to assume the rest
+		 * is movable.
+		 */
+		if (zone_idx(zone) == ZONE_MOVABLE)
+			continue;
+
 		/*

There is a WARN_ON() in case of failure at the end of the routine, is that triggered when we hit the bug? If we're adding this patch, the WARN_ON needs to go as well.

Quoted 3 lines

No the warning should stay in case we encounter reserved pages in zone movable.

Fair enough!

Quoted 9 lines
Quoted 5 lines

The check seems to be quite aggressive and in a loop that iterates pages, but has nothing to do with the page, did you mean to make the check

zone_idx(page_zone(page)) == ZONE_MOVABLE

Does it make any difference? Can we actually encounter a page from a different zone here?

Just to avoid page state related issues, do we want to go ahead with the migration if zone_idx(page_zone(page)) != ZONE_MOVABLE.

Quoted 9 lines
Quoted 1 line

it also skips all checks for pinned pages and other checks

Yes, this is intentional and the comment tries to explain why. I wish we could be add a more specific checks for movable pages - e.g. detect long term pins that would prevent migration - but we do not have any facility for that. Please note that the worst case of a false positive is a repeated migration failure and user has a way to break out of migration by a signal.

Basically isolate_pages() will fail as opposed to hotplug failing upfront. The basic assertion this patch makes is that all ZONE_MOVABLE pages that are not reserved are hotpluggable.

Balbir Singh.

#8 lore raw
Quoted 2 lines · Balbir Singh
Quoted 1 line · Michal Hocko

On Wed 07-11-18 07:35:18, Balbir Singh wrote:

[...]

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

The check seems to be quite aggressive and in a loop that iterates pages, but has nothing to do with the page, did you mean to make the check

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

zone_idx(page_zone(page)) == ZONE_MOVABLE

Does it make any difference? Can we actually encounter a page from a different zone here?

Just to avoid page state related issues, do we want to go ahead with the migration if zone_idx(page_zone(page)) != ZONE_MOVABLE.

Could you be more specific what kind of state related issues you have in mind?

Quoted 10 lines
Quoted 9 lines
Quoted 1 line

it also skips all checks for pinned pages and other checks

Yes, this is intentional and the comment tries to explain why. I wish we could be add a more specific checks for movable pages - e.g. detect long term pins that would prevent migration - but we do not have any facility for that. Please note that the worst case of a false positive is a repeated migration failure and user has a way to break out of migration by a signal.

Quoted 3 lines

Basically isolate_pages() will fail as opposed to hotplug failing upfront. The basic assertion this patch makes is that all ZONE_MOVABLE pages that are not reserved are hotpluggable.

Yes, that is correct.

Signature · 2 lines
Michal Hocko
SUSE Labs
Acked-by
#9 lore raw
Quoted 14 lines · Michal Hocko
Quoted 2 lines · Balbir Singh
Quoted 1 line · Michal Hocko

On Wed 07-11-18 07:35:18, Balbir Singh wrote:

[...]

Quoted 10 lines
Quoted 9 lines
Quoted 5 lines

The check seems to be quite aggressive and in a loop that iterates pages, but has nothing to do with the page, did you mean to make the check

zone_idx(page_zone(page)) == ZONE_MOVABLE

Does it make any difference? Can we actually encounter a page from a different zone here?

Quoted 6 lines
Quoted 2 lines

Just to avoid page state related issues, do we want to go ahead with the migration if zone_idx(page_zone(page)) != ZONE_MOVABLE.

Could you be more specific what kind of state related issues you have in mind?

I was wondering if page_zone() is setup correctly, but it's setup upfront, so I don't think that is ever an issue.

Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 1 line

it also skips all checks for pinned pages and other checks

Yes, this is intentional and the comment tries to explain why. I wish we could be add a more specific checks for movable pages - e.g. detect long term pins that would prevent migration - but we do not have any facility for that. Please note that the worst case of a false positive is a repeated migration failure and user has a way to break out of migration by a signal.

Quoted 6 lines
Quoted 3 lines

Basically isolate_pages() will fail as opposed to hotplug failing upfront. The basic assertion this patch makes is that all ZONE_MOVABLE pages that are not reserved are hotpluggable.

Yes, that is correct.

I wonder if it is easier to catch a __SetPageReserved() on ZONE_MOVABLE memory at set time, the downside is that we never know if that memory will ever be hot(un)plugged. The patch itself, I think is OK

Balbir Singh.

#10 lore raw
In reply to Michal Hocko
Quoted 5 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Page state checks are racy. Under a heavy memory workload (e.g. stress -m 200 -t 2h) it is quite easy to hit a race window when the page is allocated but its state is not fully populated yet. A debugging patch to

The original phenomenon is the value of /sys/devices/system/memory/memoryxxx/removable is 0 on several memory blocks of hotpluggable node. And almost on each hotpluggable node, there are one or several blocks which has this zero value of removable attribute. It caused the hot removing failure always.

And only cat /sys/devices/system/memory/memoryxxx/removable will trigger the call trace.

With this fix, all 'removable' of memory block on those hotpluggable nodes are '1', and hotplug can succeed.

Quoted 18 lines

dump the struct page state shows

: [  476.575516] has_unmovable_pages: pfn:0x10dfec00, found:0x1, count:0x0
: [  476.582103] page:ffffea0437fb0000 count:1 mapcount:1 mapping:ffff880e05239841 index:0x7f26e5000 compound_mapcount: 1
: [  476.592645] flags: 0x5fffffc0090034(uptodate|lru|active|head|swapbacked)

Note that the state has been checked for both PageLRU and PageSwapBacked already. Closing this race completely would require some sort of retry logic. This can be tricky and error prone (think of potential endless or long taking loops).

Workaround this problem for movable zones at least. Such a zone should only contain movable pages. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust") has told us that this is not strictly true though. Bootmem pages should be marked reserved though so we can move the original check after the PageReserved check. Pages from other zones are still prone to races but we even do not pretend that memory hotremove works for those so pre-mature failure doesn't hurt that much.

Quoted 3 lines
Reported-and-tested-by Baoquan He <[email protected]>
Fixes "mm, memory_hotplug: make has_unmovable_pages more robust")
Fixes 15c30bc09085 "mm, memory_hotplug: make has_unmovable_pages more robust")
Quoted 35 lines

---

Hi,
this has been reported [1] and we have tried multiple things to address the issue. The only reliable way was to reintroduce the movable zone check into has_unmovable_pages. This time it should be safe also for the bug originally fixed by 15c30bc09085.

[1] http://lkml.kernel.org/r/20181101091055.GA15166@MiWiFi-R3L-srv

 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/page_alloc.c +8 -0
index 863d46da6586..c6d900ee4982 100644
@@ -7788,6 +7788,14 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 		if (PageReserved(page))
 			goto unmovable;
 
+		/*
+		 * If the zone is movable and we have ruled out all reserved
+		 * pages then it should be reasonably safe to assume the rest
+		 * is movable.
+		 */
+		if (zone_idx(zone) == ZONE_MOVABLE)
+			continue;
+
 		/*
 		 * Hugepages are not in LRU lists, but they're movable.
 		 * We need not scan over tail pages bacause we don't
Signature · 1 line
2.19.1
#11 lore raw
Quoted 12 lines · Baoquan He
Quoted 5 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Page state checks are racy. Under a heavy memory workload (e.g. stress -m 200 -t 2h) it is quite easy to hit a race window when the page is allocated but its state is not fully populated yet. A debugging patch to

The original phenomenon is the value of /sys/devices/system/memory/memoryxxx/removable is 0 on several memory blocks of hotpluggable node. And almost on each hotpluggable node, there are one or several blocks which has this zero value of removable attribute. It caused the hot removing failure always.

Quoted 5 lines

And only cat /sys/devices/system/memory/memoryxxx/removable will trigger the call trace.

With this fix, all 'removable' of memory block on those hotpluggable nodes are '1', and hotplug can succeed.

Oh, by the way, hot removing/adding can always succeed when no memory pressure is added.

The hot removing failure with high memory pressure has been raised in another thread.

Thanks
Baoquan

Quoted 60 lines
Quoted 19 lines

dump the struct page state shows

: [  476.575516] has_unmovable_pages: pfn:0x10dfec00, found:0x1, count:0x0
: [  476.582103] page:ffffea0437fb0000 count:1 mapcount:1 mapping:ffff880e05239841 index:0x7f26e5000 compound_mapcount: 1
: [  476.592645] flags: 0x5fffffc0090034(uptodate|lru|active|head|swapbacked)

Note that the state has been checked for both PageLRU and PageSwapBacked already. Closing this race completely would require some sort of retry logic. This can be tricky and error prone (think of potential endless or long taking loops).

Workaround this problem for movable zones at least. Such a zone should only contain movable pages. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust") has told us that this is not strictly true though. Bootmem pages should be marked reserved though so we can move the original check after the PageReserved check. Pages from other zones are still prone to races but we even do not pretend that memory hotremove works for those so pre-mature failure doesn't hurt that much.

Quoted 3 lines
Reported-and-tested-by Baoquan He <[email protected]>
Fixes "mm, memory_hotplug: make has_unmovable_pages more robust")
Fixes 15c30bc09085 "mm, memory_hotplug: make has_unmovable_pages more robust")
Quoted 35 lines

---

Hi,
this has been reported [1] and we have tried multiple things to address the issue. The only reliable way was to reintroduce the movable zone check into has_unmovable_pages. This time it should be safe also for the bug originally fixed by 15c30bc09085.

[1] http://lkml.kernel.org/r/20181101091055.GA15166@MiWiFi-R3L-srv

 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)
mm/page_alloc.c +8 -0
index 863d46da6586..c6d900ee4982 100644
@@ -7788,6 +7788,14 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 		if (PageReserved(page))
 			goto unmovable;
 
+		/*
+		 * If the zone is movable and we have ruled out all reserved
+		 * pages then it should be reasonably safe to assume the rest
+		 * is movable.
+		 */
+		if (zone_idx(zone) == ZONE_MOVABLE)
+			continue;
+
 		/*
 		 * Hugepages are not in LRU lists, but they're movable.
 		 * We need not scan over tail pages bacause we don't
Signature · 1 line
2.19.1