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

[David Hildenbrand <david@redhat.com>]

On 17.03.21 15:08, Oscar Salvador wrote:
> On Tue, Mar 16, 2021 at 06:45:17PM +0100, David Hildenbrand wrote:
>>> I find that cross reference to vmemmap code a little hard to digest.
>>> I would have assume that we don't have to care about PMDs in this
>>> code here at all. The vmemmap population code should handle that.
>>>
>>> I think I already mentioned that somewhere, I think it should be like this:
>>>
>>> a) vmemmap code should *never* populate more memory than requested for
>>> a single memory section when we are populating from the altmap.
>>> If that cannot be guaranteed for PMDs, then we have to fallback
>>> to populating base pages. Populating PMDs from an altmap with
>>> sizeof(struct page) == 64 is highly dangerous.
> 
> I guess you meant sizeof(struct page) != 64
> 

Yes!

> But other usecases of using altmap (ZONE_DEVICE stuff) might not care whether
> they have sub-populated PMDs when populating sections from altmap?

Just assume you have two ranges

[ ZONE_DEVICE 0 ][ ZONE_DEVICE 1]

If the vmemmap of ZONE_DEVICE 1 could be taken from the altmap of 
ZONE_DEVICE 0, we could be in trouble, as both parts can be 
removed/repurposed independently ...

> 
> Current vmemmap code populates PMD with PMD_SIZE if empty, and with basepages
> if there are still holes.
> 
>>> Assume we have sizeof(struct page) == 56. A 128 MiB section
>>> spans 32768 pages -  we need 32768 * sizeof(struct page)
>>> space for the vmemmap.
>>> With 64k pages we *can* use exactly one PMD. With 56k pages
>>> we need 448 individual (full!) pages for the vmemmap.
>>>
>>> IOW, we don't care how vmemmap code will do the mapping.
>>> vmemmap code has to get it right. IMHO, asserting it in
>>> this code is wrong.
>>>
>>>
>>> b) In this code, we really should only care about what
>>> memory onlining/offlining code can or can't do.
>>> We really only care that
>>>
>>> 1) size == memory_block_size_bytes()
>>> 2) remaining_size
>>> 3) IS_ALIGNED(remaining_size, pageblock_size);
> 
> I agree with the above, but see below:
> 
>>> Okay, please document the statement about single sections, that's
>>> important to understand what's happening.
>>>
>>> My take would be
>>>
>>> bool mhp_supports_memmap_on_memory(unsigned long size)
>>> {
>>> 	/*
>>> 	 * Note: We calculate for a single memory section. The calculation
>>> 	 */
>>> 	unsigned long nr_vmemmap_pages = SECTION_SIZE / PAGE_SIZE;
>>> 	unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
>>> 	unsigned long remaining_size = size - vmemmap_size;
> 
> While it might be true that we need to back off from populating with altmap in
> case PMDs are not going to be fully populated because of the size of the struct
> page (I am not still not sure though as I said above, other usecases might not
> care at all), I would go __for now__ with placing vmemmap_size == PMD_SIZE in
> the check below as well.
> 
> If the check comes true, we know that we fully populate PMDs when populating
> sections, so the feature can be used.
> 
> Then I commit to have a look whether we need to back off in vmemmap-populating
> code in case altmap && !NOT_FULLY_POPULATED_PMDS.
> 
> What do you think?

If we check for

IS_ALIGNED(nr_vmemmap_pages, PMD_SIZE), please add a proper TODO comment 
that this is most probably the wrong place to take care of this.


-- 
Thanks,

David / dhildenb