Re: [RFC 5/5] mm, page_alloc: disable pcplists during page isolation

[David Hildenbrand <david@redhat.com>]

On 10.09.20 13:05, Vlastimil Babka wrote:
> On 9/10/20 12:29 PM, David Hildenbrand wrote:
>> On 09.09.20 13:55, Vlastimil Babka wrote:
>>> On 9/9/20 1:36 PM, Michal Hocko wrote:
>>>> On Wed 09-09-20 12:48:54, Vlastimil Babka wrote:
>>>>> Here's a version that will apply on top of next-20200908. The first 4 patches need no change.
>>>>> For callers that pair start_isolate_page_range() with
>>>>> undo_isolated_page_range() properly, this is transparent. Currently that's
>>>>> alloc_contig_range(). __offline_pages() doesn't call undo_isolated_page_range()
>>>>> in the succes case, so it has to be carful to handle restoring pcp->high and batch
>>>>> and unlocking pcp_batch_high_lock.
>>>>
>>>> I was hoping that it would be possible to have this completely hidden
>>>> inside start_isolate_page_range code path.
>>>
>>> I hoped so too, but we can't know the moment when all processes that were in the
>>> critical part of freeing pages to pcplists have moved on (they might have been
>>> rescheduled).
>>> We could change free_unref_page() to disable IRQs sooner, before
>>> free_unref_page_prepare(), or at least the get_pfnblock_migratetype() part. Then
>>> after the single drain, we should be safe, AFAICS?
>>
>> At least moving it before getting the migratetype should not be that severe?
>>
>>> RT guys might not be happy though, but it's much simpler than this patch. I
>>> still like some of the cleanups in 1-4 though tbh :)
>>
>> It would certainly make this patch much simpler. Do you have a prototype
>> lying around?
> 
> Below is the critical part, while writing it I realized that there's also
> free_unref_page_list() where it's more ugly.
> 
> So free_unref_page() simply moves the "determine migratetype" part under
> disabled interrupts. For free_unref_page_list() we optimistically determine them
> without disabling interrupts, and with disabled interrupts we check if zone has
> isolated pageblocks and thus we should not trust that value anymore. That's same
> pattern as free_pcppages_bulk uses.
> 
> But unfortunately it means an extra page_zone() plus a test for each page on the
> list :/

That function is mostly called from mm/vmscan.c AFAIKS.

The fist thing free_unref_page_commit() does is doing page_zone().
That's easy to optimize if needed.

> 
> ----8<----
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index defefed79cfb..57e2a341c95c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3103,18 +3103,21 @@ void mark_free_pages(struct zone *zone)
>  }
>  #endif /* CONFIG_PM */
>  
> -static bool free_unref_page_prepare(struct page *page, unsigned long pfn)
> +static bool free_unref_page_prepare(struct page *page)
>  {
> -	int migratetype;
> -
>  	if (!free_pcp_prepare(page))
>  		return false;
>  
> -	migratetype = get_pfnblock_migratetype(page, pfn);
> -	set_pcppage_migratetype(page, migratetype);
>  	return true;
>  }
>  
> +static inline void free_unref_page_set_migratetype(struct page *page, unsigned long pfn)
> +{
> +	int migratetype	= get_pfnblock_migratetype(page, pfn);
> +
> +	set_pcppage_migratetype(page, migratetype);
> +}
> +
>  static void free_unref_page_commit(struct page *page, unsigned long pfn)
>  {
>  	struct zone *zone = page_zone(page);
> @@ -3156,10 +3159,17 @@ void free_unref_page(struct page *page)
>  	unsigned long flags;
>  	unsigned long pfn = page_to_pfn(page);
>  
> -	if (!free_unref_page_prepare(page, pfn))
> +	if (!free_unref_page_prepare(page))
>  		return;
>  
> +	/*
> +	 * by disabling interrupts before reading pageblock's migratetype,
> +	 * we can guarantee that after changing a pageblock to MIGRATE_ISOLATE
> +	 * and drain_all_pages(), there's nobody who would read the old
> +	 * migratetype and put a page from isoalted pageblock to pcplists
> +	 */
>  	local_irq_save(flags);
> +	free_unref_page_set_migratetype(page, pfn);
>  	free_unref_page_commit(page, pfn);
>  	local_irq_restore(flags);
>  }
> @@ -3176,9 +3186,10 @@ void free_unref_page_list(struct list_head *list)
>  	/* Prepare pages for freeing */
>  	list_for_each_entry_safe(page, next, list, lru) {
>  		pfn = page_to_pfn(page);
> -		if (!free_unref_page_prepare(page, pfn))
> +		if (!free_unref_page_prepare(page))
>  			list_del(&page->lru);
>  		set_page_private(page, pfn);
> +		free_unref_page_set_migratetype(page, pfn);
>  	}
>  
>  	local_irq_save(flags);
> @@ -3187,6 +3198,8 @@ void free_unref_page_list(struct list_head *list)
>  
>  		set_page_private(page, 0);
>  		trace_mm_page_free_batched(page);
> +		if (has_isolate_pageblock(page_zone(page)))
> +			free_unref_page_set_migratetype(page, pfn);
>  		free_unref_page_commit(page, pfn);
>  

FWIW: You can safely add unlikely() for the has_isolate_pageblock(), as
for most other users of that function I was able to spot.

-- 
Thanks,

David / dhildenb