Re: [PATCH v5 1/5] mm,memory_hotplug: Allocate memmap from the added memory range

[David Hildenbrand <david@redhat.com>]

On 26.03.21 09:35, Michal Hocko wrote:
> On Thu 25-03-21 23:06:50, Oscar Salvador wrote:
>> On Thu, Mar 25, 2021 at 05:47:30PM +0100, Michal Hocko wrote:
>>> On Thu 25-03-21 17:36:22, Michal Hocko wrote:
>>>> If all it takes is to make pfn_to_online_page work (and my
>>>> previous attempt is incorrect because it should consult block rather
>>>> than section pfn range)
>>>
>>> This should work.
>>
>> Sorry, but while this solves some of the issues with that approach, I really
>> think that overcomplicates things and buys us not so much in return.
>> To me it seems that we are just patching things to make it work that
>> way.
> 
> I do agree that special casing vmemmap areas is something I do not
> really like but we are in that schrödinger situation when this memory is
> not onlineable unless it shares memory section with an onlineable
> memory. There are two ways around that, either special case it on
> pfn_to_online_page or mark the vmemmap section online even though it is
> not really.
> 
>> To be honest, I dislike this, and I guess we can only agree to disagree
>> here.
> 
> No problem there. I will not insist on my approach unless I can convince
> you that it is a better solution. It seems I have failed and I can live
> with that.
> 
>> I find the following much easier, cleaner, and less risky to encounter
>> pitfalls in the future:
>>
>> (!!!It is untested and incomplete, and I would be surprised if it even
>> compiles, but it is enough as a PoC !!!)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 5ea2b3fbce02..799d14fc2f9b 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -169,6 +169,60 @@ int memory_notify(unsigned long val, void *v)
>>   	return blocking_notifier_call_chain(&memory_chain, val, v);
>>   }
>>
>> +static int memory_block_online(unsigned long start_pfn, unsigned long nr_pages,
>> +			       unsigned long nr_vmemmap_pages, int online_type,
>> +			       int nid)
>> +{
>> +	int ret;
>> +	/*
>> +	 * Despite vmemmap pages having a different lifecycle than the pages
>> +	 * they describe, initialiating and accounting vmemmap pages at the
>> +	 * online/offline stage eases things a lot.
> 
> This requires quite some explaining.
> 
>> +	 * We do it out of {online,offline}_pages, so those routines only have
>> +	 * to deal with pages that are actual usable memory.
>> +	 */
>> +	if (nr_vmemmap_pages) {
>> +		struct zone *z;
>> +
>> +		z = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages);
>> +		move_pfn_range_to_zone(z, start_pfn, nr_vmemmap_pages, NULL,
>> +				       MIGRATE_UNMOVABLE);
>> +		/*
>> +		 * The below could go to a helper to make it less bulky here,
>> +		 * so {online,offline}_pages could also use it.
>> +		 */
>> +		z->present_pages += nr_pages;
>> +		pgdat_resize_lock(z->zone_pgdat, &flags);
>> +		z->zone_pgdat->node_present_pages += nr_pages;
>> +		pgdat_resize_unlock(z->zone_pgdat, &flags);

Might have to set fully spanned section online. (vmemmap >= SECTION_SIZE)

>> +	}
>> +
>> +	ret = online_pages(start_pfn + nr_vmemmap_pages, nr_pages - nr_vmemmap_pages,
>> +			   online_type);
>> +
>> +	/*
>> +	 * In case online_pages() failed for some reason, we should cleanup vmemmap
>> +	 * accounting as well.
>> +	 */
>> +	return ret;
>> +}
> 
> Yes this is much better! Just a minor suggestion would be to push
> memory_block all the way to memory_block_online (it oline a memory
> block). I would also slightly prefer to provide 2 helpers that would make
> it clear that this is to reserve/cleanup the vmemamp space (defined in
> the memory_hotplug proper).
> 
> Thanks!
> 

Something else to note:


We'll not call the memory notifier (e.g., MEM_ONLINE) for the vmemmap. 
The result is that

1. We won't allocate extended struct pages for the range. Don't think 
this is really problematic (pages are never allocated/freed, so I guess 
we don't care - like ZONE_DEVICE code).

2. We won't allocate kasan shadow memory. We most probably have to do it 
explicitly via kasan_add_zero_shadow()/kasan_remove_zero_shadow(), see 
mm/memremap.c:pagemap_range()


Further a locking rework might be necessary. We hold the device hotplug 
lock, but not the memory hotplug lock. E.g., for get_online_mems(). 
Might have to move that out online_pages.

-- 
Thanks,

David / dhildenb