Re: [PATCH v1 1/2] mm/memory_hotplug: PG_offline_skippable for offlining memory blocks with PageOffline pages

[David Hildenbrand <david@redhat.com>]

Thanks a bucnh for the review!

>> +
>> +		if (PageOffline(page) && PageOfflineSkippable(page))
>> +			continue;
>> +
> 
> Some comment like "Skippable PageOffline() pages are not migratable but are
> skipped during memory offline" might help understand the change.

I can add a comment like for the other cases.

> 
> Some thoughts after reading the related code:
> 
> offline_pages() is a little convoluted, since it has two steps to make
> sure a range of memory can be offlined:
> 1. start_isolate_page_range() checks unmovable (maybe not migratable
> is more precise) pages using has_unmovable_pages(), but leaves unmovable
> PageOffline() page handling to the driver;

Right, it's best-effort. For ZONE_MOVABLE we're skipping the checks 
completely.

I was wondering for a longer time that -- with the isolate flag being a 
separate bit soon :) -- we could simply isolate the whole range, and 
then fail if we stumble over an unmovable page during migration. That 
is, get rid of has_unmovable_pages() entirely. Un-doing the isolation 
would then properly preserve the migratetype -- no harm done?

Certainly worth a look. What do you think about that?

> 2. scan_movable_pages() and do_migrate_range() migrate pages and handle
> different types of PageOffline() pages.

Right, migrate what we can, skip these special once, and bail out on any 
others (at least in this patch, patch #2 restores the pre-virtio-mem 
behavior).

> 
> It might make the logic cleaner if start_isolate_page_range() can
> have a callback to allow driver to handle PageOffline() pages.

We have to identify them repeadetly, so start_isolate_page_range() would 
not be sufficient. Also, callbacks are rather tricky for the case where 
we cannot really stabilize the page.

> 
> Hmm, Skippable PageOffline() pages are virtio-mem specific, I wonder
> why offline_pages() takes care of them. Shouldn't virtio-mem driver
> handle them?
 > I also realize that the two steps in offline_pages()> are very 
similar to alloc_contig_range() and virtio-mem is using
> alloc_contig_range(). I wonder if the two steps in offline_pages()
> can be abstracted out and shared with virtio-mem.Yes, offline_pages()> operates at memory section granularity, different from the granularity,
> pageblock size, of alloc_contig_range() used in virtio-mem, but
> both are trying to check unmovable pages and migrate movable pages.

Not sure I get completely what you mean. virtio-mem really wants to use 
alloc_contig_range() to allocate ranges it wants to unplug, because it 
will fail fast and allows for smaller ranges.

offline_pages() is primarily also about offlining the memory section, 
which virtio-mem must do in order to remove the Linux memory block.

> 
> 
>>   		folio = page_folio(page);
>>
>>   		if (!folio_try_get(folio))
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a6fe1e9b95941..325b51c905076 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -7023,12 +7023,12 @@ unsigned long __offline_isolated_pages(unsigned long start_pfn,
>>   			continue;
>>   		}
>>   		/*
>> -		 * At this point all remaining PageOffline() pages have a
>> -		 * reference count of 0 and can simply be skipped.
>> +		 * At this point all remaining PageOffline() pages must be
>> +		 * "skippable" and have exactly one reference.
>>   		 */
>>   		if (PageOffline(page)) {
>> -			BUG_ON(page_count(page));
>> -			BUG_ON(PageBuddy(page));
>> +			WARN_ON_ONCE(!PageOfflineSkippable(page));
>> +			WARN_ON_ONCE(page_count(page) != 1);
>>   			already_offline++;
>>   			pfn++;
>>   			continue;
>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>> index b2fc5266e3d26..debd898b794ea 100644
>> --- a/mm/page_isolation.c
>> +++ b/mm/page_isolation.c
>> @@ -121,16 +121,11 @@ static struct page *has_unmovable_pages(unsigned long start_pfn, unsigned long e
>>   			continue;
>>
>>   		/*
>> -		 * We treat all PageOffline() pages as movable when offlining
>> -		 * to give drivers a chance to decrement their reference count
>> -		 * in MEM_GOING_OFFLINE in order to indicate that these pages
>> -		 * can be offlined as there are no direct references anymore.
>> -		 * For actually unmovable PageOffline() where the driver does
>> -		 * not support this, we will fail later when trying to actually
>> -		 * move these pages that still have a reference count > 0.
>> -		 * (false negatives in this function only)
>> +		 * PageOffline() pages that are marked as "skippable"
>> +		 * are treated like movable pages for memory offlining.
>>   		 */
>> -		if ((flags & MEMORY_OFFLINE) && PageOffline(page))
>> +		if ((flags & MEMORY_OFFLINE) && PageOffline(page) &&
>> +		    PageOfflineSkippable(page))
>>   			continue;
> 
> With this change, we are no longer give non-virtio-mem driver a chance
> to decrease PageOffline(page) refcount? Or virtio-mem is the only driver
> doing this?

The only in-tree driver making use of this so far, yes.


There is one tweak I'll have to perform in the virtio-mem driver to 
cover one corner case: when force-unloading the virtio-mem driver, we 
have to make sure that memory blocks with fake-offline pages cannot get 
offlined (re-onlining would be bad). I'll fix that up.

-- 
Cheers,

David / dhildenb