Re: [PATCH] mm:ALLOC_HIGHATOMIC flag allocation issuse

[Andrew Morton]

On Wed,  8 Nov 2023 14:54:07 +0800 Zhiguo Jiang <justinjiang@xxxxxxxx> wrote:

> 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 

Hopefully hannes can check this for us, but I have a stylistic concern...

> +#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 b8544f08cc36..67cec88164b1
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2854,11 +2854,15 @@ struct page *__rmqueue_pcplist(struct zone *zone, unsigned int order,
>  			int batch = nr_pcp_alloc(pcp, zone, order);
>  			int alloced;
>  
> +			if (alloc_flags & ALLOC_HIGHATOMIC)
> +				goto out;
> +

A comment here explaining why we're doing this would help readers.

>  			alloced = rmqueue_bulk(zone, order,
>  					batch, list,
>  					migratetype, alloc_flags);
>  
>  			pcp->count += alloced << order;
> +out:
>  			if (unlikely(list_empty(list)))
>  				return NULL;
>  		}
> @@ -2921,7 +2925,7 @@ __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,
> +			gfp_t gfp_flags, unsigned int *alloc_flags,
>  			int migratetype)
>  {
>  	struct page *page;
> @@ -2934,17 +2938,19 @@ 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))
> +				       migratetype, *alloc_flags);
> +		if (likely(page)) {
> +			*alloc_flags |= ALLOC_PCPLIST;
>  			goto out;
> +		}
>  	}

So we're effectively returning a boolean to the caller via *alloc_flags.

This isn't a great way of doing it.  Wouldn't it be cleaner to pass a
new bool* argument to rmqueue() for this?  Make it explicit?

rmqueue() will be inlined into its sole caller, so this approach
shouldn't add overhead.

> -	page = rmqueue_buddy(preferred_zone, zone, order, alloc_flags,
> +	page = rmqueue_buddy(preferred_zone, zone, order, *alloc_flags,
>  							migratetype);
> 
>  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));
> @@ -3343,7 +3349,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);
>  		if (page) {
>  			prep_new_page(page, order, gfp_mask, alloc_flags);
>  
> @@ -3351,7 +3357,8 @@ 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))
> +			if (unlikely(alloc_flags & ALLOC_HIGHATOMIC) &&
> +				unlikely(!(alloc_flags & ALLOC_PCPLIST)))

Again, a comment explaining the reason for the test would be good.

>  				reserve_highatomic_pageblock(page, zone);
>  
>  			return page;
> -- 
> 2.39.0
>