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
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?. Is the concern related to the increase in the size of
struct memory_block ?
>> +
>> struct memory_block {
>> unsigned long start_section_nr;
>> unsigned long state; /* serialized by the dev->lock */
>> @@ -77,11 +94,7 @@ struct memory_block {
>> */
>> struct zone *zone;
>> struct device dev;
>> - /*
>> - * Number of vmemmap pages. These pages
>> - * lay at the beginning of the memory block.
>> - */
>> - unsigned long nr_vmemmap_pages;
>> + struct vmem_altmap altmap;
>> struct memory_group *group; /* group (if any) for this block */
>> struct list_head group_next; /* next block inside memory group */
>> #if defined(CONFIG_MEMORY_FAILURE) && defined(CONFIG_MEMORY_HOTPLUG)
>> @@ -147,7 +160,7 @@ static inline int hotplug_memory_notifier(notifier_fn_t fn, int pri)
>> extern int register_memory_notifier(struct notifier_block *nb);
>> extern void unregister_memory_notifier(struct notifier_block *nb);
>> int create_memory_block_devices(unsigned long start, unsigned long size,
>
> [...]
>
>> static int check_cpu_on_node(int nid)
>> @@ -2036,9 +2042,8 @@ EXPORT_SYMBOL(try_offline_node);
>> static int __ref try_remove_memory(u64 start, u64 size)
>> {
>> - struct vmem_altmap mhp_altmap = {};
>> + int ret;
>> struct vmem_altmap *altmap = NULL;
>> - unsigned long nr_vmemmap_pages;
>> int rc = 0, nid = NUMA_NO_NODE;
>> BUG_ON(check_hotplug_memory_range(start, size));
>> @@ -2060,24 +2065,16 @@ static int __ref try_remove_memory(u64 start, u64 size)
>> * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in
>> * the same granularity it was added - a single memory block.
>> */
>> +
>
> ^ unrealted change?
>
