[PATCH v3] mm: ALLOC_HIGHATOMIC flag allocation issue

[PATCH v3] mm: ALLOC_HIGHATOMIC flag allocation issue

[Zhiguo Jiang]

Update base on the latest Linus' tree or Andrew's tree.

Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
---

Changelog:
v1:
In case that alloc_flags contains ALLOC_HIGHATOMIC and alloc order
is order1/2/3/10 in rmqueue(), if pages are alloced successfully
from pcplist, a free pageblock will be also moved from the alloced
migratetype freelist to MIGRATE_HIGHATOMIC freelist, rather than
alloc from MIGRATE_HIGHATOMIC freelist firstly, so this will result
in an increasing number of pages on the MIGRATE_HIGHATOMIC freelist,
pages in other migratetype freelist are reduced and more likely to
allocation failure.

Currently the sequence of ALLOC_HIGHATOMIC allocation is:
pcplist --> rmqueue_bulk() --> rmqueue_buddy() MIGRATE_HIGHATOMIC
--> rmqueue_buddy() allocation migratetype.

Due to the fact that requesting pages from the pcplist is faster than
buddy, the sequence of modifying the ALLOC_HIGHATOMIC allocation is:
pcplist --> rmqueue_buddy() MIGRATE_HIGHATOMIC --> rmqueue_buddy()
allocation migratetype.

This patch can solve the failure problem of allocating other types of
pages due to excessive MIGRATE_HIGHATOMIC freelist reservations.

In comparative testing, cat /proc/pagetypeinfo and the HighAtomic
freelist size is:
Test without this patch:
Node 0, zone Normal, type HighAtomic 2369 771 138 15 0 0 0 0 0 0 0
Test with this patch:
Node 0, zone Normal, type HighAtomic 206 82 4 2 1 0 0 0 0 0 0

v2:
Update comments and modify variable highatomic_allocation to highatomic.

 mm/page_alloc.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 49890d00cc3c..693e86fc9850 100755
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2851,9 +2851,9 @@ struct page *__rmqueue_pcplist(struct zone *zone, unsigned int order,
 			int alloced;
 
 			/*
-			 * If pcplist is empty and alloc_flags is with ALLOC_HIGHATOMIC,
-			 * it should alloc from buddy highatomic migrate freelist firstly
-			 * to ensure quick and successful allocation.
+			 * If pcplist is empty and alloc_flags contains
+			 * ALLOC_HIGHATOMIC, alloc from buddy highatomic
+			 * freelist first.
 			 */
 			if (alloc_flags & ALLOC_HIGHATOMIC)
 				goto out;
@@ -2927,7 +2927,7 @@ static inline
 struct page *rmqueue(struct zone *preferred_zone,
 			struct zone *zone, unsigned int order,
 			gfp_t gfp_flags, unsigned int alloc_flags,
-			int migratetype, bool *highatomc_allocation)
+			int migratetype, bool *highatomic)
 {
 	struct page *page;
 
@@ -2948,21 +2948,22 @@ struct page *rmqueue(struct zone *preferred_zone,
 							migratetype);
 
 	/*
-	 * The high-order atomic allocation pageblock reserved conditions:
+	 * The high-order atomic allocation pageblock reserved:
 	 *
-	 * If the high-order atomic allocation page is alloced from pcplist,
-	 * the highatomic pageblock does not need to be reserved, which can
-	 * void to migrate an increasing number of pages into buddy
-	 * MIGRATE_HIGHATOMIC freelist and lead to an increasing risk of
-	 * allocation failure on other buddy migrate freelists.
+	 * If the high-order atomic page is allocated from pcplist,
+	 * the highatomic pageblock does not need to be reserved,
+	 * which can avoid migrating an increasing number of pages
+	 * into buddy highatomic freelist and leading to an increased
+	 * risk of allocation failure on other migrate freelists in
+	 * buddy.
 	 *
-	 * If the high-order atomic allocation page is alloced from buddy
-	 * highatomic migrate freelist, regardless of whether the allocation
-	 * is successful or not, the highatomic pageblock can try to be
-	 * reserved.
+	 * If the high-order atomic page is allocated from buddy
+	 * highatomic freelist, regardless of whether the allocation
+	 * is successful or not, the highatomic pageblock can try to
+	 * be reserved.
 	 */
 	if (unlikely(alloc_flags & ALLOC_HIGHATOMIC))
-		*highatomc_allocation = true;
+		*highatomic = true;
 
 out:
 	/* Separate test+clear to avoid unnecessary atomics */
@@ -3234,7 +3235,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 	struct pglist_data *last_pgdat = NULL;
 	bool last_pgdat_dirty_ok = false;
 	bool no_fallback;
-	bool highatomc_allocation = false;
+	bool highatomic = false;
 
 retry:
 	/*
@@ -3366,7 +3367,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 
 try_this_zone:
 		page = rmqueue(ac->preferred_zoneref->zone, zone, order,
-				gfp_mask, alloc_flags, ac->migratetype, &highatomc_allocation);
+				gfp_mask, alloc_flags, ac->migratetype, &highatomic);
 		if (page) {
 			prep_new_page(page, order, gfp_mask, alloc_flags);
 
@@ -3374,7 +3375,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 			 * If this is a high-order atomic allocation then check
 			 * if the pageblock should be reserved for the future
 			 */
-			if (unlikely(highatomc_allocation))
+			if (unlikely(highatomic))
 				reserve_highatomic_pageblock(page, zone);
 
 			return page;
-- 
2.39.0

Re: [PATCH v3] mm: ALLOC_HIGHATOMIC flag allocation issue

[Matthew Wilcox]

On Wed, Nov 22, 2023 at 09:39:25AM +0800, Zhiguo Jiang wrote:
> Update base on the latest Linus' tree or Andrew's tree.
> +++ b/mm/page_alloc.c
> @@ -2851,9 +2851,9 @@ struct page *__rmqueue_pcplist(struct zone *zone, unsigned int order,
>  			int alloced;
>  
>  			/*
> -			 * If pcplist is empty and alloc_flags is with ALLOC_HIGHATOMIC,
> -			 * it should alloc from buddy highatomic migrate freelist firstly
> -			 * to ensure quick and successful allocation.
> +			 * If pcplist is empty and alloc_flags contains
> +			 * ALLOC_HIGHATOMIC, alloc from buddy highatomic
> +			 * freelist first.

No, it's still based on your earlier patch,  is it really so hard for
you to read your patches before you send them?

Re: [PATCH v3] mm: ALLOC_HIGHATOMIC flag allocation issue

[zhiguojiang]

在 2023/11/22 14:20, Matthew Wilcox 写道:
> On Wed, Nov 22, 2023 at 09:39:25AM +0800, Zhiguo Jiang wrote:
>> Update base on the latest Linus' tree or Andrew's tree.
>> +++ b/mm/page_alloc.c
>> @@ -2851,9 +2851,9 @@ struct page *__rmqueue_pcplist(struct zone *zone, unsigned int order,
>>   			int alloced;
>>   
>>   			/*
>> -			 * If pcplist is empty and alloc_flags is with ALLOC_HIGHATOMIC,
>> -			 * it should alloc from buddy highatomic migrate freelist firstly
>> -			 * to ensure quick and successful allocation.
>> +			 * If pcplist is empty and alloc_flags contains
>> +			 * ALLOC_HIGHATOMIC, alloc from buddy highatomic
>> +			 * freelist first.
> No, it's still based on your earlier patch,  is it really so hard for
> you to read your patches before you send them?
Sorry, I didn't understand your meaning correctly before. I have updated 
base on the latest linux-next tree in patch v4.
Thank you for your patient guidance.

[PATCH v3] mm: ALLOC_HIGHATOMIC flag allocation issue

[Zhiguo Jiang]

Retain (and update) the changelog with this patch.

Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
---

Changelog:
v1:
In case that alloc_flags contains ALLOC_HIGHATOMIC and alloc order
is order1/2/3/10 in rmqueue(), if pages are alloced successfully
from pcplist, a free pageblock will be also moved from the alloced
migratetype freelist to MIGRATE_HIGHATOMIC freelist, rather than
alloc from MIGRATE_HIGHATOMIC freelist firstly, so this will result
in an increasing number of pages on the MIGRATE_HIGHATOMIC freelist,
pages in other migratetype freelist are reduced and more likely to
allocation failure.

Currently the sequence of ALLOC_HIGHATOMIC allocation is:
pcplist --> rmqueue_bulk() --> rmqueue_buddy() MIGRATE_HIGHATOMIC
--> rmqueue_buddy() allocation migratetype.

Due to the fact that requesting pages from the pcplist is faster than
buddy, the sequence of modifying the ALLOC_HIGHATOMIC allocation is:
pcplist --> rmqueue_buddy() MIGRATE_HIGHATOMIC --> rmqueue_buddy()
allocation migratetype.

This patch can solve the failure problem of allocating other types of
pages due to excessive MIGRATE_HIGHATOMIC freelist reservations.

In comparative testing, cat /proc/pagetypeinfo and the HighAtomic
freelist size is:
Test without this patch:
Node 0, zone Normal, type HighAtomic 2369 771 138 15 0 0 0 0 0 0 0
Test with this patch:
Node 0, zone Normal, type HighAtomic 206 82 4 2 1 0 0 0 0 0 0

v2:
Add a new bool* argument to pass return flag instead of *alloc_flags
and add the related comments.

 mm/internal.h   |  1 -
 mm/page_alloc.c | 41 ++++++++++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 98c14b16ce81..1d67c141902d 100755
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -905,7 +905,6 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 #endif
 #define ALLOC_HIGHATOMIC	0x200 /* Allows access to MIGRATE_HIGHATOMIC */
 #define ALLOC_KSWAPD		0x800 /* allow waking of kswapd, __GFP_KSWAPD_RECLAIM set */
-#define ALLOC_PCPLIST		0x1000 /* Allocations from pcplist */
 
 /* Flags that allow allocations below the min watermark. */
 #define ALLOC_RESERVES (ALLOC_NON_BLOCK|ALLOC_MIN_RESERVE|ALLOC_HIGHATOMIC|ALLOC_OOM)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 67cec88164b1..3c84c3e3eeb0 100755
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2854,6 +2854,11 @@ struct page *__rmqueue_pcplist(struct zone *zone, unsigned int order,
 			int batch = nr_pcp_alloc(pcp, zone, order);
 			int alloced;
 
+			/*
+			 * If pcplist is empty and alloc_flags is with ALLOC_HIGHATOMIC,
+			 * it should alloc from buddy highatomic migrate freelist firstly
+			 * to ensure quick and successful allocation.
+			 */
 			if (alloc_flags & ALLOC_HIGHATOMIC)
 				goto out;
 
@@ -2925,8 +2930,8 @@ __no_sanitize_memory
 static inline
 struct page *rmqueue(struct zone *preferred_zone,
 			struct zone *zone, unsigned int order,
-			gfp_t gfp_flags, unsigned int *alloc_flags,
-			int migratetype)
+			gfp_t gfp_flags, unsigned int alloc_flags,
+			int migratetype, bool *highatomc_allocation)
 {
 	struct page *page;
 
@@ -2938,19 +2943,33 @@ struct page *rmqueue(struct zone *preferred_zone,
 
 	if (likely(pcp_allowed_order(order))) {
 		page = rmqueue_pcplist(preferred_zone, zone, order,
-				       migratetype, *alloc_flags);
-		if (likely(page)) {
-			*alloc_flags |= ALLOC_PCPLIST;
+				       migratetype, alloc_flags);
+		if (likely(page))
 			goto out;
-		}
 	}
 
-	page = rmqueue_buddy(preferred_zone, zone, order, *alloc_flags,
+	page = rmqueue_buddy(preferred_zone, zone, order, alloc_flags,
 							migratetype);
 
+	/*
+	 * The high-order atomic allocation pageblock reserved conditions:
+	 *
+	 * If the high-order atomic allocation page is alloced from pcplist,
+	 * the highatomic pageblock does not need to be reserved, which can
+	 * void to migrate an increasing number of pages into buddy
+	 * MIGRATE_HIGHATOMIC freelist and lead to an increasing risk of
+	 * allocation failure on other buddy migrate freelists.
+	 *
+	 * If the high-order atomic allocation page is alloced from buddy
+	 * highatomic migrate freelist, regardless of whether the allocation
+	 * is successful or not, the highatomic pageblock can try to be
+	 * reserved.
+	 */
+	if (unlikely(alloc_flags & ALLOC_HIGHATOMIC))
+		*highatomc_allocation = true;
 out:
 	/* Separate test+clear to avoid unnecessary atomics */
-	if ((*alloc_flags & ALLOC_KSWAPD) &&
+	if ((alloc_flags & ALLOC_KSWAPD) &&
 	    unlikely(test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags))) {
 		clear_bit(ZONE_BOOSTED_WATERMARK, &zone->flags);
 		wakeup_kswapd(zone, 0, 0, zone_idx(zone));
@@ -3218,6 +3237,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 	struct pglist_data *last_pgdat = NULL;
 	bool last_pgdat_dirty_ok = false;
 	bool no_fallback;
+	bool highatomc_allocation = false;
 
 retry:
 	/*
@@ -3349,7 +3369,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 
 try_this_zone:
 		page = rmqueue(ac->preferred_zoneref->zone, zone, order,
-				gfp_mask, &alloc_flags, ac->migratetype);
+				gfp_mask, alloc_flags, ac->migratetype, &highatomc_allocation);
 		if (page) {
 			prep_new_page(page, order, gfp_mask, alloc_flags);
 
@@ -3357,8 +3377,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 			 * If this is a high-order atomic allocation then check
 			 * if the pageblock should be reserved for the future
 			 */
-			if (unlikely(alloc_flags & ALLOC_HIGHATOMIC) &&
-				unlikely(!(alloc_flags & ALLOC_PCPLIST)))
+			if (unlikely(highatomc_allocation))
 				reserve_highatomic_pageblock(page, zone);
 
 			return page;
-- 
2.39.0