mm, page_alloc: fix core hung in free_pcppages_bulk()

PATCH Applied Charan Teja Reddy 4 participants 12 messages Aug 11, 2020 – Aug 14, 2020

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Charan Teja Reddy

author

2020-08-11 12:58 UTC #0 lore raw

PATCH v2 1/1 mm, page_alloc: fix core hung in free_pcppages_bulk()

The following race is observed with the repeated online, offline and a delay between two successive online of memory blocks of movable zone.

P1 P2

Online the first memory block in
the movable zone. The pcp struct
values are initialized to default
values,i.e., pcp->high = 0 &
pcp->batch = 1.

Allocate the pages from the movable zone.

Try to Online the second memory
block in the movable zone thus it
entered the online_pages() but yet
to call zone_pcp_update().
This process is entered into the exit path thus it tries to release the order-0 pages to pcp lists through free_unref_page_commit(). As pcp->high = 0, pcp->count = 1 proceed to call the function free_pcppages_bulk().
Update the pcp values thus the
new pcp values are like, say,
pcp->high = 378, pcp->batch = 63.
Read the pcp's batch value using READ_ONCE() and pass the same to free_pcppages_bulk(), pcp values passed here are, batch = 63, count = 1.

Since num of pages in the pcp lists are less than ->batch, then it will stuck in while(list_empty(list)) loop with interrupts disabled thus a core hung.

Avoid this by ensuring free_pcppages_bulk() is called with proper count of pcp list pages.

The mentioned race is some what easily reproducible without [1] because pcp's are not updated for the first memory block online and thus there is a enough race window for P2 between alloc+free and pcp struct values update through onlining of second memory block.

With [1], the race is still exists but it is very much narrow as we update the pcp struct values for the first memory block online itself.

[1]: https://patchwork.kernel.org/patch/11696389/

Signed-off-by Charan Teja Reddy <[email protected]>

---

v1: https://patchwork.kernel.org/patch/11707637/

 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)

mm/page_alloc.c +5 -0

index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;

Signature · 2 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

David Hildenbrand

2020-08-11 21:05 UTC #1 lore raw

Quoted 50 lines · Charan Teja Reddy

The following race is observed with the repeated online, offline and a delay between two successive online of memory blocks of movable zone.
P1 P2
Online the first memory block in
the movable zone. The pcp struct
values are initialized to default
values,i.e., pcp->high = 0 &
pcp->batch = 1.
Allocate the pages from the movable zone.
Try to Online the second memory
block in the movable zone thus it
entered the online_pages() but yet
to call zone_pcp_update().
This process is entered into the exit path thus it tries to release the order-0 pages to pcp lists through free_unref_page_commit(). As pcp->high = 0, pcp->count = 1 proceed to call the function free_pcppages_bulk().
Update the pcp values thus the
new pcp values are like, say,
pcp->high = 378, pcp->batch = 63.
Read the pcp's batch value using READ_ONCE() and pass the same to free_pcppages_bulk(), pcp values passed here are, batch = 63, count = 1.
Since num of pages in the pcp lists are less than ->batch, then it will stuck in while(list_empty(list)) loop with interrupts disabled thus a core hung.
Avoid this by ensuring free_pcppages_bulk() is called with proper count of pcp list pages.
The mentioned race is some what easily reproducible without [1] because pcp's are not updated for the first memory block online and thus there is a enough race window for P2 between alloc+free and pcp struct values update through onlining of second memory block.

Quoted 5 lines

With [1], the race is still exists but it is very much narrow as we update the pcp struct values for the first memory block online itself.
[1]: https://patchwork.kernel.org/patch/11696389/

IIUC, this is not limited to the movable zone, it could also happen in corner cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet).

Quoted 15 lines

Signed-off-by Charan Teja Reddy <[email protected]>
---
v1: https://patchwork.kernel.org/patch/11707637/
 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;

mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1314

Quoted 10 lines

mm/page_alloc.c +5 -0

 	LIST_HEAD(head);
 
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;

Fixes and Cc: stable... tags?

Signature · 3 lines

Thanks,

David / dhildenb

Charan Teja Kalla

author

2020-08-12 09:46 UTC #2 lore raw

Thanks David for the inputs.

Quoted 54 lines · David Hildenbrand

Quoted 53 lines · Charan Teja Reddy
The following race is observed with the repeated online, offline and a delay between two successive online of memory blocks of movable zone.
P1 P2
Online the first memory block in
the movable zone. The pcp struct
values are initialized to default
values,i.e., pcp->high = 0 &
pcp->batch = 1.
Allocate the pages from the movable zone.
Try to Online the second memory
block in the movable zone thus it
entered the online_pages() but yet
to call zone_pcp_update().
This process is entered into the exit path thus it tries to release the order-0 pages to pcp lists through free_unref_page_commit(). As pcp->high = 0, pcp->count = 1 proceed to call the function free_pcppages_bulk().
Update the pcp values thus the
new pcp values are like, say,
pcp->high = 378, pcp->batch = 63.
Read the pcp's batch value using READ_ONCE() and pass the same to free_pcppages_bulk(), pcp values passed here are, batch = 63, count = 1.
Since num of pages in the pcp lists are less than ->batch, then it will stuck in while(list_empty(list)) loop with interrupts disabled thus a core hung.
Avoid this by ensuring free_pcppages_bulk() is called with proper count of pcp list pages.
The mentioned race is some what easily reproducible without [1] because pcp's are not updated for the first memory block online and thus there is a enough race window for P2 between alloc+free and pcp struct values update through onlining of second memory block.
With [1], the race is still exists but it is very much narrow as we update the pcp struct values for the first memory block online itself.

Quoted 6 lines

Quoted 2 lines
[1]: https://patchwork.kernel.org/patch/11696389/
IIUC, this is not limited to the movable zone, it could also happen in corner cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet).

Yes, this is my understanding too. I explained the above race in terms of just movable zone for which it is observed. We can add the below line in the end in patch commit message:
"This is not limited to the movable zone, it could also happen in cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet)."

Just curious, there exists such systems where just a dma zone present and we hot add the normal zone? I am not aware such thing in the embedded world.

Quoted 17 lines

Quoted 17 lines
Signed-off-by Charan Teja Reddy <[email protected]>
---
v1: https://patchwork.kernel.org/patch/11707637/
 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 

Quoted 11 lines

mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1314
Quoted 9 lines
mm/page_alloc.c +5 -0
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
 
Fixes and Cc: stable... tags?

Fixes 5f8dcc21211a ("page-allocator: split per-cpu list into

one-list-per-migrate-type")
Cc: <[email protected]> [2.6+]

I am not sure If I should have to raise V3 including these?

Quoted 1 line

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

David Hildenbrand

2020-08-12 10:00 UTC #3 lore raw

Quoted 65 lines · Charan Teja Kalla

Thanks David for the inputs.
Quoted 54 lines · David Hildenbrand
Quoted 53 lines · Charan Teja Reddy
The following race is observed with the repeated online, offline and a delay between two successive online of memory blocks of movable zone.
P1 P2
Online the first memory block in
the movable zone. The pcp struct
values are initialized to default
values,i.e., pcp->high = 0 &
pcp->batch = 1.
Allocate the pages from the movable zone.
Try to Online the second memory
block in the movable zone thus it
entered the online_pages() but yet
to call zone_pcp_update().
This process is entered into the exit path thus it tries to release the order-0 pages to pcp lists through free_unref_page_commit(). As pcp->high = 0, pcp->count = 1 proceed to call the function free_pcppages_bulk().
Update the pcp values thus the
new pcp values are like, say,
pcp->high = 378, pcp->batch = 63.
Read the pcp's batch value using READ_ONCE() and pass the same to free_pcppages_bulk(), pcp values passed here are, batch = 63, count = 1.
Since num of pages in the pcp lists are less than ->batch, then it will stuck in while(list_empty(list)) loop with interrupts disabled thus a core hung.
Avoid this by ensuring free_pcppages_bulk() is called with proper count of pcp list pages.
The mentioned race is some what easily reproducible without [1] because pcp's are not updated for the first memory block online and thus there is a enough race window for P2 between alloc+free and pcp struct values update through onlining of second memory block.
With [1], the race is still exists but it is very much narrow as we update the pcp struct values for the first memory block online itself.
Quoted 6 lines
Quoted 2 lines
[1]: https://patchwork.kernel.org/patch/11696389/
IIUC, this is not limited to the movable zone, it could also happen in corner cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet).

Quoted 6 lines

Yes, this is my understanding too. I explained the above race in terms of just movable zone for which it is observed. We can add the below line in the end in patch commit message:
"This is not limited to the movable zone, it could also happen in cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet)."

Yeah, that makes sense!

Quoted 4 lines

Just curious, there exists such systems where just a dma zone present and we hot add the normal zone? I am not aware such thing in the embedded world.

You can easily create such setups using QEMU.

IIRC, just specify a QEMU guest with 2G initial memory and a single NUMA node, or 4G initial memory and two NUMA nodes. Then hotplug memory.

(IIRC kata containers always start a VM with 2G and then hotplug memory)

Quoted 29 lines

Quoted 17 lines
Quoted 17 lines
Signed-off-by Charan Teja Reddy <[email protected]>
---
v1: https://patchwork.kernel.org/patch/11707637/
 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 
Quoted 11 lines
mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1314
Quoted 9 lines
mm/page_alloc.c +5 -0
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
 
Fixes and Cc: stable... tags?

Quoted 3 lines

Fixes 5f8dcc21211a ("page-allocator: split per-cpu list into
one-list-per-migrate-type")
Cc: <[email protected]> [2.6+]

Did we have memory hotplug support then already?

Quoted 2 lines

I am not sure If I should have to raise V3 including these?

Maybe Andrew can fixup when applying.

Signature · 3 lines

Thanks,

David / dhildenb

Charan Teja Kalla

author

2020-08-12 10:11 UTC #4 lore raw

Quoted 80 lines · David Hildenbrand

Quoted 65 lines · Charan Teja Kalla
Thanks David for the inputs.
Quoted 54 lines · David Hildenbrand
Quoted 53 lines · Charan Teja Reddy
The following race is observed with the repeated online, offline and a delay between two successive online of memory blocks of movable zone.
P1 P2
Online the first memory block in
the movable zone. The pcp struct
values are initialized to default
values,i.e., pcp->high = 0 &
pcp->batch = 1.
Allocate the pages from the movable zone.
Try to Online the second memory
block in the movable zone thus it
entered the online_pages() but yet
to call zone_pcp_update().
This process is entered into the exit path thus it tries to release the order-0 pages to pcp lists through free_unref_page_commit(). As pcp->high = 0, pcp->count = 1 proceed to call the function free_pcppages_bulk().
Update the pcp values thus the
new pcp values are like, say,
pcp->high = 378, pcp->batch = 63.
Read the pcp's batch value using READ_ONCE() and pass the same to free_pcppages_bulk(), pcp values passed here are, batch = 63, count = 1.
Since num of pages in the pcp lists are less than ->batch, then it will stuck in while(list_empty(list)) loop with interrupts disabled thus a core hung.
Avoid this by ensuring free_pcppages_bulk() is called with proper count of pcp list pages.
The mentioned race is some what easily reproducible without [1] because pcp's are not updated for the first memory block online and thus there is a enough race window for P2 between alloc+free and pcp struct values update through onlining of second memory block.
With [1], the race is still exists but it is very much narrow as we update the pcp struct values for the first memory block online itself.
Quoted 6 lines
Quoted 2 lines
[1]: https://patchwork.kernel.org/patch/11696389/
IIUC, this is not limited to the movable zone, it could also happen in corner cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet).
Quoted 6 lines
Yes, this is my understanding too. I explained the above race in terms of just movable zone for which it is observed. We can add the below line in the end in patch commit message:
"This is not limited to the movable zone, it could also happen in cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet)."
Yeah, that makes sense!
Quoted 4 lines
Just curious, there exists such systems where just a dma zone present and we hot add the normal zone? I am not aware such thing in the embedded world.

Quoted 7 lines

You can easily create such setups using QEMU.
IIRC, just specify a QEMU guest with 2G initial memory and a single NUMA node, or 4G initial memory and two NUMA nodes. Then hotplug memory.
(IIRC kata containers always start a VM with 2G and then hotplug memory)

I see. Thanks for letting me know this.

Quoted 29 lines

Quoted 29 lines
Quoted 28 lines
Quoted 16 lines
Signed-off-by Charan Teja Reddy <[email protected]>
---
v1: https://patchwork.kernel.org/patch/11707637/
 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1314
Quoted 10 lines
mm/page_alloc.c +5 -0
 	LIST_HEAD(head);
 
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
 
Fixes and Cc: stable... tags?

Quoted 5 lines

Quoted 3 lines
Fixes 5f8dcc21211a ("page-allocator: split per-cpu list into
one-list-per-migrate-type")
Cc: <[email protected]> [2.6+]
Did we have memory hotplug support then already?

Yes, it exist.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/mm/memory_hotplug.c?h=v2.6.39

Quoted 6 lines

Quoted 3 lines
I am not sure If I should have to raise V3 including these?
Maybe Andrew can fixup when applying.

Okay, let Andrew decide on this. Meanwhile If you find that this patch looks correct, ACK from you helps here.

Quoted 2 lines

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

David Hildenbrand

Acked-by

2020-08-13 09:32 UTC #5 lore raw

Quoted 121 lines · Charan Teja Kalla

Quoted 80 lines · David Hildenbrand
Quoted 65 lines · Charan Teja Kalla
Thanks David for the inputs.
Quoted 54 lines · David Hildenbrand
Quoted 53 lines · Charan Teja Reddy
The following race is observed with the repeated online, offline and a delay between two successive online of memory blocks of movable zone.
P1 P2
Online the first memory block in
the movable zone. The pcp struct
values are initialized to default
values,i.e., pcp->high = 0 &
pcp->batch = 1.
Allocate the pages from the movable zone.
Try to Online the second memory
block in the movable zone thus it
entered the online_pages() but yet
to call zone_pcp_update().
This process is entered into the exit path thus it tries to release the order-0 pages to pcp lists through free_unref_page_commit(). As pcp->high = 0, pcp->count = 1 proceed to call the function free_pcppages_bulk().
Update the pcp values thus the
new pcp values are like, say,
pcp->high = 378, pcp->batch = 63.
Read the pcp's batch value using READ_ONCE() and pass the same to free_pcppages_bulk(), pcp values passed here are, batch = 63, count = 1.
Since num of pages in the pcp lists are less than ->batch, then it will stuck in while(list_empty(list)) loop with interrupts disabled thus a core hung.
Avoid this by ensuring free_pcppages_bulk() is called with proper count of pcp list pages.
The mentioned race is some what easily reproducible without [1] because pcp's are not updated for the first memory block online and thus there is a enough race window for P2 between alloc+free and pcp struct values update through onlining of second memory block.
With [1], the race is still exists but it is very much narrow as we update the pcp struct values for the first memory block online itself.
Quoted 6 lines
Quoted 2 lines
[1]: https://patchwork.kernel.org/patch/11696389/
IIUC, this is not limited to the movable zone, it could also happen in corner cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet).
Quoted 6 lines
Yes, this is my understanding too. I explained the above race in terms of just movable zone for which it is observed. We can add the below line in the end in patch commit message:
"This is not limited to the movable zone, it could also happen in cases with the normal zone (e.g., hotplug to a node that only has DMA memory, or no other memory yet)."
Yeah, that makes sense!
Quoted 4 lines
Just curious, there exists such systems where just a dma zone present and we hot add the normal zone? I am not aware such thing in the embedded world.
Quoted 7 lines
You can easily create such setups using QEMU.
IIRC, just specify a QEMU guest with 2G initial memory and a single NUMA node, or 4G initial memory and two NUMA nodes. Then hotplug memory.
(IIRC kata containers always start a VM with 2G and then hotplug memory)
I see. Thanks for letting me know this.
Quoted 29 lines
Quoted 29 lines
Quoted 28 lines
Quoted 16 lines
Signed-off-by Charan Teja Reddy <[email protected]>
---
v1: https://patchwork.kernel.org/patch/11707637/
 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1314
Quoted 10 lines
mm/page_alloc.c +5 -0
 	LIST_HEAD(head);
 
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
 
Fixes and Cc: stable... tags?

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Fixes 5f8dcc21211a ("page-allocator: split per-cpu list into
one-list-per-migrate-type")
Cc: <[email protected]> [2.6+]
Did we have memory hotplug support then already?
Yes, it exist.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/mm/memory_hotplug.c?h=v2.6.39

Okay, so I guess these tags make sense.

Quoted 10 lines

Quoted 7 lines
Quoted 4 lines
I am not sure If I should have to raise V3 including these?
Maybe Andrew can fixup when applying.
Okay, let Andrew decide on this. Meanwhile If you find that this patch looks correct, ACK from you helps here.

Sure, I think this is good enough as a simple fix.

Acked-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

David Rientjes

Acked-by

2020-08-12 18:53 UTC #6 lore raw

In reply to Charan Teja Kalla

Quoted 28 lines · Charan Teja Kalla

Quoted 16 lines
Quoted 16 lines
Signed-off-by Charan Teja Reddy <[email protected]>
---
v1: https://patchwork.kernel.org/patch/11707637/
 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 
Quoted 11 lines
mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1314
Quoted 9 lines
mm/page_alloc.c +5 -0
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
 
Fixes and Cc: stable... tags?

Quoted 4 lines

Fixes 5f8dcc21211a ("page-allocator: split per-cpu list into
one-list-per-migrate-type")
Cc: <[email protected]> [2.6+]

Acked-by David Rientjes <[email protected]>

Michal Hocko

reviewer

2020-08-13 11:41 UTC #7 lore raw

In reply to Charan Teja Reddy

On Tue 11-08-20 18:28:23, Charan Teja Reddy wrote:
[...]

Quoted 5 lines

mm/page_alloc.c +0 -0
index e4896e6..839039f 100644

mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1313

Quoted 10 lines

mm/page_alloc.c +5 -0

@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;

How does this prevent the race actually? Don't we need something like the following instead?

mm/page_alloc.c +9 -1

index e028b87ce294..45bcc7ba37c4 100644
@@ -1317,9 +1317,16 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 		 * lists
 		 */
 		do {
+			bool looped = false;
+
 			batch_free++;
-			if (++migratetype == MIGRATE_PCPTYPES)
+			if (++migratetype == MIGRATE_PCPTYPES) {
+				if (looped)
+					goto free;
+
 				migratetype = 0;
+				looped = true;
+			}
 			list = &pcp->lists[migratetype];
 		} while (list_empty(list));

@@ -1352,6 +1359,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 		} while (--count && --batch_free && !list_empty(list));
 	}
 
+free:
 	spin_lock(&zone->lock);
 	isolated_pageblocks = has_isolate_pageblock(zone);

Signature · 2 lines

Michal Hocko
SUSE Labs

Charan Teja Kalla

author

2020-08-13 16:21 UTC #8 lore raw

Thanks Michal for comments.

Quoted 10 lines · Michal Hocko

On Tue 11-08-20 18:28:23, Charan Teja Reddy wrote:
[...]
Quoted 8 lines
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 

Quoted 10 lines

mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1313
Quoted 7 lines
mm/page_alloc.c +5 -0
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
How does this prevent the race actually?

This doesn't prevent the race. This only fixes the core hung(as this is called with spin_lock_irq()) caused by the race condition. This core hung is because of incorrect count value is passed to the free_pcppages_bulk() function.

The actual race should be fixed by David's suggestion (isolate, online and undo isolation).

Something needs to be improved in commit message? May be like below:
s/The following race is observed with the repeated ... / Cpu core hung is observed as a result of race with the use case of repeated...

Don't we need something like

mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1320

Quoted 10 lines

the following instead?

mm/page_alloc.c +1 -0

index e028b87ce294..45bcc7ba37c4 100644
@@ -1317,9 +1317,16 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 		 * lists
 		 */
 		do {
+			bool looped = false;

IIUC, this looped will always be initialzed to false thus never jumped to free.
But I think I got your idea that looping of the pcp lists for any pages. If not found despite MIGRATE_PCPTYPES count lists are traversed, just break the loop. Does this checking really required? Shouldn't pcp->count tells the same whether any pages left in the pcp lists?

Quoted 22 lines

mm/page_alloc.c +8 -1

+
 			batch_free++;
-			if (++migratetype == MIGRATE_PCPTYPES)
+			if (++migratetype == MIGRATE_PCPTYPES) {
+				if (looped)
+					goto free;
+
 				migratetype = 0;
+				looped = true;
+			}
 			list = &pcp->lists[migratetype];
 		} while (list_empty(list));

@@ -1352,6 +1359,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 		} while (--count && --batch_free && !list_empty(list));
 	}
 
+free:
 	spin_lock(&zone->lock);
 	isolated_pageblocks = has_isolate_pageblock(zone);

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Michal Hocko

reviewer

2020-08-13 16:30 UTC #9 lore raw

Quoted 24 lines · Charan Teja Kalla

Thanks Michal for comments.
Quoted 10 lines · Michal Hocko
On Tue 11-08-20 18:28:23, Charan Teja Reddy wrote:
[...]
Quoted 8 lines
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 
Quoted 10 lines
mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1313
Quoted 7 lines
mm/page_alloc.c +5 -0
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
How does this prevent the race actually?

Quoted 4 lines

This doesn't prevent the race. This only fixes the core hung(as this is called with spin_lock_irq()) caused by the race condition. This core hung is because of incorrect count value is passed to the free_pcppages_bulk() function.

Let me ask differently. What does enforce that the count and lists do not get out of sync in the loop. Your changelog says that the fix is to use the proper value without any specifics.

Signature · 2 lines

Michal Hocko
SUSE Labs

Charan Teja Kalla

author

2020-08-13 17:27 UTC #10 lore raw

Thanks Michal.

Quoted 25 lines · Michal Hocko

Quoted 24 lines · Charan Teja Kalla
Thanks Michal for comments.
Quoted 20 lines · Michal Hocko
On Tue 11-08-20 18:28:23, Charan Teja Reddy wrote:
[...]
Quoted 5 lines
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1313
Quoted 10 lines
mm/page_alloc.c +5 -0
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
How does this prevent the race actually?

Quoted 7 lines

Quoted 4 lines
This doesn't prevent the race. This only fixes the core hung(as this is called with spin_lock_irq()) caused by the race condition. This core hung is because of incorrect count value is passed to the free_pcppages_bulk() function.
Let me ask differently. What does enforce that the count and lists do not get out of sync in the loop.

count value is updated whenever an order-0 page is being added to the pcp lists through free_unref_page_commit(), which is being called with both interrupts, premption disabled.

static void free_unref_page_commit(struct page *page, {
	....
	list_add(&page->lru, &pcp->lists[migratetype]);
	pcp->count++
}

As these are pcp lists, they only gets touched by another process when this process is context switched, which happens only after enabling premption or interrupts. So, as long as process is operating on these pcp lists in free_unref_page_commit function, the count and lists are always synced.

However, the problem here is not that the count and lists are being out of sync. They do always in sync, as explained above. It is with the asking free_pcppages_bulk() to free the pages more than what is present in the pcp lists which is ending up in while(list_empty()).

Quoted 3 lines

Your changelog says that the fix is to
use the proper value without any specifics.

Will change this to: Ensure the count value passed is not greater than the pcp lists count. Any better you suggest?

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Michal Hocko

reviewer Acked-by

2020-08-14 06:39 UTC #11 lore raw

Quoted 51 lines · Charan Teja Kalla

Thanks Michal.
Quoted 25 lines · Michal Hocko
Quoted 24 lines · Charan Teja Kalla
Thanks Michal for comments.
Quoted 20 lines · Michal Hocko
On Tue 11-08-20 18:28:23, Charan Teja Reddy wrote:
[...]
Quoted 5 lines
mm/page_alloc.c +0 -0
index e4896e6..839039f 100644
mm/page_alloc.c static void free_pcppages_bulk(struct zone *zone, int count, · line 1313
Quoted 10 lines
mm/page_alloc.c +5 -0
@@ -1304,6 +1304,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	struct page *page, *tmp;
 	LIST_HEAD(head);
 
+	/*
+	 * Ensure proper count is passed which otherwise would stuck in the
+	 * below while (list_empty(list)) loop.
+	 */
+	count = min(pcp->count, count);
 	while (count) {
 		struct list_head *list;
How does this prevent the race actually?
Quoted 7 lines
Quoted 4 lines
This doesn't prevent the race. This only fixes the core hung(as this is called with spin_lock_irq()) caused by the race condition. This core hung is because of incorrect count value is passed to the free_pcppages_bulk() function.
Let me ask differently. What does enforce that the count and lists do not get out of sync in the loop.
count value is updated whenever an order-0 page is being added to the pcp lists through free_unref_page_commit(), which is being called with both interrupts, premption disabled.
static void free_unref_page_commit(struct page *page, {
	....
	list_add(&page->lru, &pcp->lists[migratetype]);
	pcp->count++
}
As these are pcp lists, they only gets touched by another process when this process is context switched, which happens only after enabling premption or interrupts. So, as long as process is operating on these pcp lists in free_unref_page_commit function, the count and lists are always synced.

Quoted 4 lines

However, the problem here is not that the count and lists are being out of sync. They do always in sync, as explained above. It is with the asking free_pcppages_bulk() to free the pages more than what is present in the pcp lists which is ending up in while(list_empty()).

You are right. I managed to confuse myself. The thing is that the batch count is out of sync.

Quoted 5 lines

Quoted 3 lines
Your changelog says that the fix is to
use the proper value without any specifics.
Will change this to: Ensure the count value passed is not greater than the pcp lists count. Any better you suggest?

Yes, this makes it more clear.

Feel free to add

Acked-by Michal Hocko <[email protected]>

Signature · 2 lines

Michal Hocko
SUSE Labs