Re: [PATCH 1/6] mm: page_alloc: remove pcppage migratetype caching

[Vlastimil Babka]

On 9/12/23 17:03, Johannes Weiner wrote:
> On Tue, Sep 12, 2023 at 03:47:45PM +0200, Vlastimil Babka wrote:
>> I think after this change we should [...]
> 
> Speaking of follow-ups, AFAICS we no longer need those either:

Seems so, but the comments do talk about races, so once those are sorted out :)

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9cad31de1bf5..bea499fbca58 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1751,13 +1751,6 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
>  
>  	old_block_type = get_pageblock_migratetype(page);
>  
> -	/*
> -	 * This can happen due to races and we want to prevent broken
> -	 * highatomic accounting.
> -	 */
> -	if (is_migrate_highatomic(old_block_type))
> -		goto single_page;
> -
>  	/* Take ownership for orders >= pageblock_order */
>  	if (current_order >= pageblock_order) {
>  		change_pageblock_range(page, current_order, start_type);
> @@ -1926,24 +1919,15 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
>  				continue;
>  
>  			/*
> -			 * In page freeing path, migratetype change is racy so
> -			 * we can counter several free pages in a pageblock
> -			 * in this loop although we changed the pageblock type
> -			 * from highatomic to ac->migratetype. So we should
> -			 * adjust the count once.
> +			 * It should never happen but changes to
> +			 * locking could inadvertently allow a per-cpu
> +			 * drain to add pages to MIGRATE_HIGHATOMIC
> +			 * while unreserving so be safe and watch for
> +			 * underflows.
>  			 */
> -			if (is_migrate_highatomic_page(page)) {
> -				/*
> -				 * It should never happen but changes to
> -				 * locking could inadvertently allow a per-cpu
> -				 * drain to add pages to MIGRATE_HIGHATOMIC
> -				 * while unreserving so be safe and watch for
> -				 * underflows.
> -				 */
> -				zone->nr_reserved_highatomic -= min(
> -						pageblock_nr_pages,
> -						zone->nr_reserved_highatomic);
> -			}
> +			zone->nr_reserved_highatomic -= min(
> +				pageblock_nr_pages,
> +				zone->nr_reserved_highatomic);
>  
>  			/*
>  			 * Convert to ac->migratetype and avoid the normal
> 
> I think they were only in place because we could change the highatomic
> status of pages on the pcplist, and those pages would then end up on
> some other freelist due to the stale pcppage cache.
> 
> I replaced them locally with WARNs and ran an hour or so of kernel
> builds under pressure. It didn't trigger. So I would send a follow up
> to remove them.
> 
> Unless you point me to a good reason why they're definitely still
> needed - in which case this is a moot proposal - but then we should
> make the comments more specific.