mm: fix a BUG, the page is allocated 2 times

RFC In review yalin wang 4 participants 7 messages Oct 12, 2015 – Oct 13, 2015

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

yalin wang

author

2015-10-12 02:40 UTC #0 lore raw

RFC 1/1 mm: fix a BUG, the page is allocated 2 times

Remove unlikely(order), because we are sure order is not zero if code reach here, also add if (page == NULL), only allocate page again if __rmqueue_smallest() failed or alloc_flags & ALLOC_HARDER == 0

Signed-off-by yalin wang <[email protected]>

---
 mm/page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

mm/page_alloc.c +3 -3

index 0d6f540..de82e2c 100644
@@ -2241,13 +2241,13 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
 		spin_lock_irqsave(&zone->lock, flags);
 
 		page = NULL;
-		if (unlikely(order) && (alloc_flags & ALLOC_HARDER)) {
+		if (alloc_flags & ALLOC_HARDER) {
 			page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
 			if (page)
 				trace_mm_page_alloc_zone_locked(page, order, migratetype);
 		}
-
-		page = __rmqueue(zone, order, migratetype, gfp_flags);
+		if (page == NULL)
+			page = __rmqueue(zone, order, migratetype, gfp_flags);
 		spin_unlock(&zone->lock);
 		if (!page)
 			goto failed;

Signature · 7 lines

1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vlastimil Babka

2015-10-12 07:38 UTC #1 lore raw

Quoted 3 lines · yalin wang

Remove unlikely(order), because we are sure order is not zero if code reach here, also add if (page == NULL), only allocate page again if __rmqueue_smallest() failed or alloc_flags & ALLOC_HARDER == 0

The second mentioned change is actually more important as it removes a memory leak! Thanks for catching this. The problem is in patch mm-page_alloc-reserve-pageblocks-for-high-order-atomic-allocations-on-demand.patch and seems to have been due to a change in the last submitted version to make sure the tracepoint is called.

Quoted 12 lines

Signed-off-by yalin wang <[email protected]>

---
  mm/page_alloc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

mm/page_alloc.c +0 -0

index 0d6f540..de82e2c 100644
@@ -2241,13 +2241,13 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
  		spin_lock_irqsave(&zone->lock, flags);

mm/page_alloc.c struct page *buffered_rmqueue(struct zone *preferred_zone, · line 2249

Quoted 10 lines

mm/page_alloc.c +2 -3

  		page = NULL;
-		if (unlikely(order) && (alloc_flags & ALLOC_HARDER)) {
+		if (alloc_flags & ALLOC_HARDER) {
  			page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
  			if (page)
  				trace_mm_page_alloc_zone_locked(page, order, migratetype);
  		}
-
-		page = __rmqueue(zone, order, migratetype, gfp_flags);
+		if (page == NULL)

"if (!page)" is more common and already used below.
We could skip the check for !page in case we don't go through the ALLOC_HARDER branch, but I guess it's not worth the goto, and hopefully the compiler is smart enough anyway...

mm/page_alloc.c struct page *buffered_rmqueue(struct zone *preferred_zone, · line 2253

Quoted 5 lines

mm/page_alloc.c +1 -0

+			page = __rmqueue(zone, order, migratetype, gfp_flags);
  		spin_unlock(&zone->lock);
  		if (!page)
  			goto failed;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

yalin wang

author

2015-10-12 07:58 UTC #2 lore raw

Quoted 22 lines · Vlastimil Babka

Quoted 3 lines · yalin wang
Remove unlikely(order), because we are sure order is not zero if code reach here, also add if (page == NULL), only allocate page again if __rmqueue_smallest() failed or alloc_flags & ALLOC_HARDER == 0
The second mentioned change is actually more important as it removes a memory leak! Thanks for catching this. The problem is in patch mm-page_alloc-reserve-pageblocks-for-high-order-atomic-allocations-on-demand.patch and seems to have been due to a change in the last submitted version to make sure the tracepoint is called.
Quoted 12 lines
Signed-off-by yalin wang <[email protected]>
---
 mm/page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
mm/page_alloc.c +0 -0
index 0d6f540..de82e2c 100644
@@ -2241,13 +2241,13 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
 		spin_lock_irqsave(&zone->lock, flags);
 

Quoted 13 lines

mm/page_alloc.c struct page *buffered_rmqueue(struct zone *preferred_zone, · line 2249
Quoted 10 lines
mm/page_alloc.c +2 -3
 		page = NULL;
-		if (unlikely(order) && (alloc_flags & ALLOC_HARDER)) {
+		if (alloc_flags & ALLOC_HARDER) {
 			page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
 			if (page)
 				trace_mm_page_alloc_zone_locked(page, order, migratetype);
 		}
-
-		page = __rmqueue(zone, order, migratetype, gfp_flags);
+		if (page == NULL)
"if (!page)" is more common and already used below.
We could skip the check for !page in case we don't go through the ALLOC_HARDER branch, but I guess it's not worth the goto, and hopefully the compiler is smart enough anyway…

agree with your comments,
do i need send a new patch for this ?

Thanks

Kirill A. Shutemov

2015-10-12 10:05 UTC #3 lore raw

Quoted 35 lines · yalin wang

Quoted 22 lines · Vlastimil Babka
Quoted 3 lines · yalin wang
Remove unlikely(order), because we are sure order is not zero if code reach here, also add if (page == NULL), only allocate page again if __rmqueue_smallest() failed or alloc_flags & ALLOC_HARDER == 0
The second mentioned change is actually more important as it removes a memory leak! Thanks for catching this. The problem is in patch mm-page_alloc-reserve-pageblocks-for-high-order-atomic-allocations-on-demand.patch and seems to have been due to a change in the last submitted version to make sure the tracepoint is called.
Quoted 12 lines
Signed-off-by yalin wang <[email protected]>
---
 mm/page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
mm/page_alloc.c +0 -0
index 0d6f540..de82e2c 100644
@@ -2241,13 +2241,13 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
 		spin_lock_irqsave(&zone->lock, flags);
 
Quoted 13 lines
mm/page_alloc.c struct page *buffered_rmqueue(struct zone *preferred_zone, · line 2249
Quoted 10 lines
mm/page_alloc.c +2 -3
 		page = NULL;
-		if (unlikely(order) && (alloc_flags & ALLOC_HARDER)) {
+		if (alloc_flags & ALLOC_HARDER) {
 			page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
 			if (page)
 				trace_mm_page_alloc_zone_locked(page, order, migratetype);
 		}
-
-		page = __rmqueue(zone, order, migratetype, gfp_flags);
+		if (page == NULL)
"if (!page)" is more common and already used below.
We could skip the check for !page in case we don't go through the ALLOC_HARDER branch, but I guess it's not worth the goto, and hopefully the compiler is smart enough anywaya?|

Quoted 2 lines

agree with your comments,
do i need send a new patch for this ?

Looks like a two patches to me: memory leak and removing always-true part of condifition.

Signature · 7 lines

 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vlastimil Babka

2015-10-12 11:22 UTC #4 lore raw

Quoted 36 lines · Kirill A. Shutemov

Quoted 35 lines · yalin wang
Quoted 35 lines · Vlastimil Babka
Quoted 3 lines · yalin wang
Remove unlikely(order), because we are sure order is not zero if code reach here, also add if (page == NULL), only allocate page again if __rmqueue_smallest() failed or alloc_flags & ALLOC_HARDER == 0
The second mentioned change is actually more important as it removes a memory leak! Thanks for catching this. The problem is in patch mm-page_alloc-reserve-pageblocks-for-high-order-atomic-allocations-on-demand.patch and seems to have been due to a change in the last submitted version to make sure the tracepoint is called.
Quoted 12 lines
Signed-off-by yalin wang <[email protected]>
---
  mm/page_alloc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
mm/page_alloc.c +0 -0
index 0d6f540..de82e2c 100644
@@ -2241,13 +2241,13 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
  		spin_lock_irqsave(&zone->lock, flags);
 
mm/page_alloc.c struct page *buffered_rmqueue(struct zone *preferred_zone, · line 2249
Quoted 10 lines
mm/page_alloc.c +2 -3
  		page = NULL;
-		if (unlikely(order) && (alloc_flags & ALLOC_HARDER)) {
+		if (alloc_flags & ALLOC_HARDER) {
  			page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
  			if (page)
  				trace_mm_page_alloc_zone_locked(page, order, migratetype);
  		}
-
-		page = __rmqueue(zone, order, migratetype, gfp_flags);
+		if (page == NULL)
"if (!page)" is more common and already used below.
We could skip the check for !page in case we don't go through the ALLOC_HARDER branch, but I guess it's not worth the goto, and hopefully the compiler is smart enough anywaya?|

Quoted 2 lines

Quoted 2 lines
agree with your comments,
do i need send a new patch for this ?

I'd guess no need to, Andrew can edit the patch?

Quoted 3 lines

Looks like a two patches to me: memory leak and removing always-true part of condifition.

Yeah but I'd expect both would be in the end folded into the buggy patch in -mm?

Mel Gorman

Acked-by Acked-by on v1 1/1

2015-10-12 13:52 UTC #5 lore raw

In reply to yalin wang

Quoted 5 lines · yalin wang

Remove unlikely(order), because we are sure order is not zero if code reach here, also add if (page == NULL), only allocate page again if __rmqueue_smallest() failed or alloc_flags & ALLOC_HARDER == 0
Signed-off-by yalin wang <[email protected]>

Thanks very much for catching this!

Acked-by Mel Gorman <[email protected]>

With your current subject and changelog, there is a small risk that Andrew will miss this or not see it for some time. Would you mind resending the patch with a changelog similar to this please? It spells out that it is a fix to an mmotm patch so it'll be obvious where it should be inserted before merging to mainline.

From: yalin wang <[email protected]>
Subject: [PATCH] mm, page_alloc: reserve pageblocks for high-order atomic allocations on demand -fix

There is a redundant check and a memory leak introduced by a patch in mmotm. This patch removes an unlikely(order) check as we are sure order is not zero at the time. It also checks if a page is already allocated to avoid a memory leak.

This is a fix to the mmotm patch mm-page_alloc-reserve-pageblocks-for-high-order-atomic-allocations-on-demand.patch

Signed-off-by yalin wang <[email protected]>

Acked-by Mel Gorman <[email protected]>

yalin wang

author

2015-10-13 01:43 UTC #6 lore raw

Quoted 8 lines · Mel Gorman

There is a redundant check and a memory leak introduced by a patch in mmotm. This patch removes an unlikely(order) check as we are sure order is not zero at the time. It also checks if a page is already allocated to avoid a memory leak.

Quoted 5 lines

This is a fix to the mmotm patch mm-page_alloc-reserve-pageblocks-for-high-order-atomic-allocations-on-demand.patch
Signed-off-by yalin wang <[email protected]>
Acked-by Mel Gorman <[email protected]>

no problem,
i have send the patch again using your change log .

Thanks
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>