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

[Vlastimil Babka]

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 :/

----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);
 
 		/*