Re: [PATCH v2 1/5] mm/hotplug: Embed vmem_altmap details in memory block

[David Hildenbrand <david@redhat.com>]

On 06.07.23 11:36, Aneesh Kumar K V wrote:
> On 7/6/23 2:48 PM, David Hildenbrand wrote:
>> On 06.07.23 10:50, Aneesh Kumar K.V wrote:
>>> With memmap on memory, some architecture needs more details w.r.t altmap
>>> such as base_pfn, end_pfn, etc to unmap vmemmap memory.
>>
>> Can you elaborate why ppc64 needs that and x86-64 + aarch64 don't?
>>
>> IOW, why can't ppc64 simply allocate the vmemmap from the start of the memblock (-> base_pfn) and use the stored number of vmemmap pages to calculate the end_pfn?
>>
>> To rephrase: if the vmemmap is not at the beginning and doesn't cover full apgeblocks, memory onlining/offlining would be broken.
>>
>> [...]
> 
> 
> With ppc64 and 64K pagesize and different memory block sizes, we can end up allocating vmemmap backing memory from outside altmap because
> a single page vmemmap can cover 1024 pages (64 *1024/sizeof(struct page)). and that can point to pages outside the dev_pagemap range.
> So on free we  check

So you end up with a mixture of altmap and ordinarily-allocated vmemmap 
pages? That sound wrong (and is counter-intuitive to the feature in 
general, where we *don't* want to allocate the vmemmap from outside the 
altmap).

(64 * 1024) / sizeof(struct page) -> 1024 pages

1024 pages * 64k = 64 MiB.

What's the memory block size on these systems? If it's >= 64 MiB the 
vmemmap of a single memory block fits into a single page and we should 
be fine.

Smells like you want to disable the feature on a 64k system.

> 
> vmemmap_free() {
> ...
> 	if (altmap) {
> 		alt_start = altmap->base_pfn;
> 		alt_end = altmap->base_pfn + altmap->reserve +
> 			  altmap->free + altmap->alloc + altmap->align;
> 	}
> 
> ...
> 		if (base_pfn >= alt_start && base_pfn < alt_end) {
> 			vmem_altmap_free(altmap, nr_pages);
> 
> to see whether we did use altmap for the vmemmap allocation.
> 
>>
>>>    +/**
>>> + * struct vmem_altmap - pre-allocated storage for vmemmap_populate
>>> + * @base_pfn: base of the entire dev_pagemap mapping
>>> + * @reserve: pages mapped, but reserved for driver use (relative to @base)
>>> + * @free: free pages set aside in the mapping for memmap storage
>>> + * @align: pages reserved to meet allocation alignments
>>> + * @alloc: track pages consumed, private to vmemmap_populate()
>>> + */
>>> +struct vmem_altmap {
>>> +    unsigned long base_pfn;
>>> +    const unsigned long end_pfn;
>>> +    const unsigned long reserve;
>>> +    unsigned long free;
>>> +    unsigned long align;
>>> +    unsigned long alloc;
>>> +};
>>
>> Instead of embedding that, what about conditionally allocating it and store a pointer to it in the "struct memory_block"?
>>
>> In the general case as of today, we don't have an altmap.
>>
> 
> Sure but with memmap on memory option it is essentially adding that right?.

At least on x86_64 and aarch64 only for 128 MiB DIMMs (and especially, 
not memory added by hv-balloon, virtio-mem, xen-balloon).

So in the general case it's not that frequently used. Maybe on ppc64 
once wired up.

Is the concern related to the increase in the size of
> struct memory_block  ?

Partially. It looks cleaner to have !mem->altmap if there is no altmap.

-- 
Cheers,

David / dhildenb