| Message ID | 20230706085041.826340-2-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for memmap on memory feature on ppc64 | expand |
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. [...] > > +/** > + * 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. > + > 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?
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? >
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.
On 7/6/23 4:44 PM, David Hildenbrand wrote: > 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. > But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea of adding vmemmap_altmap to struct memory_block >> >> 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. >> -aneesh
On 06.07.23 14:32, Aneesh Kumar K V wrote: > On 7/6/23 4:44 PM, David Hildenbrand wrote: >> 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. >> > > But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require > a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea > of adding vmemmap_altmap to struct memory_block I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it). Then, you can reconstruct the altmap layout trivially base_pfn: start of the range to unplug end_pfn: base_pfn + nr_vmemmap_pages and pass that to the removal code, which will do the right thing, no? Sure, remembering the altmap might be a potential cleanup (eventually?), but the basic reasoning why this is required as patch #1 IMHO is wrong: if you say you support memmap_on_memory for a configuration, then you should also properly support it (allocate from the hotplugged memory), not silently fall back to something else.
On 7/6/23 6:29 PM, David Hildenbrand wrote: > On 06.07.23 14:32, Aneesh Kumar K V wrote: >> On 7/6/23 4:44 PM, David Hildenbrand wrote: >>> 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. >>> >> >> But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require >> a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea >> of adding vmemmap_altmap to struct memory_block > > I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it). > Sure. How about? bool mhp_supports_memmap_on_memory(unsigned long size) { unsigned long nr_pages = size >> PAGE_SHIFT; unsigned long vmemmap_size = nr_pages * sizeof(struct page); if (!radix_enabled()) return false; /* * memmap on memory only supported with memory block size add/remove */ if (size != memory_block_size_bytes()) return false; /* * Also make sure the vmemmap allocation is fully contianed * so that we always allocate vmemmap memory from altmap area. */ if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE)) return false; /* * The pageblock alignment requirement is met by using * reserve blocks in altmap. */ return true; } > Then, you can reconstruct the altmap layout trivially > > base_pfn: start of the range to unplug > end_pfn: base_pfn + nr_vmemmap_pages > > and pass that to the removal code, which will do the right thing, no? > > > Sure, remembering the altmap might be a potential cleanup (eventually?), but the basic reasoning why this is required as patch #1 IMHO is wrong: if you say you support memmap_on_memory for a configuration, then you should also properly support it (allocate from the hotplugged memory), not silently fall back to something else. I guess you want to keep the altmap introduction as a later patch in the series and not the preparatory patch? Or are you ok with just adding the additional check I mentioned above w.r.t size value and keep this patch as patch 1 as a generic cleanup (avoiding the recomputation of altmap->alloc/base_pfn/end_pfn? -aneesh
On 06.07.23 18:06, Aneesh Kumar K V wrote: > On 7/6/23 6:29 PM, David Hildenbrand wrote: >> On 06.07.23 14:32, Aneesh Kumar K V wrote: >>> On 7/6/23 4:44 PM, David Hildenbrand wrote: >>>> 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. >>>> >>> >>> But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require >>> a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea >>> of adding vmemmap_altmap to struct memory_block >> >> I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it). >> > > Sure. How about? > > bool mhp_supports_memmap_on_memory(unsigned long size) > { > > unsigned long nr_pages = size >> PAGE_SHIFT; > unsigned long vmemmap_size = nr_pages * sizeof(struct page); > > if (!radix_enabled()) > return false; > /* > * memmap on memory only supported with memory block size add/remove > */ > if (size != memory_block_size_bytes()) > return false; > /* > * Also make sure the vmemmap allocation is fully contianed > * so that we always allocate vmemmap memory from altmap area. > */ > if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE)) > return false; > /* > * The pageblock alignment requirement is met by using > * reserve blocks in altmap. > */ > return true; > } Better, but the PAGE_SIZE that could be added to common code as well. ... but, the pageblock check in common code implies a PAGE_SIZE check, so why do we need any other check besides the radix_enabled() check for arm64 and just keep all the other checks in common code as they are? If your vmemmap does not cover full pageblocks (which implies full pages), the feature cannot be used *unless* we'd waste altmap space in the vmemmap to cover one pageblock. Wasting hotplugged memory certainly sounds wrong? So I appreciate if you could explain why the pageblock check should not be had for ppc64? > > > > >> Then, you can reconstruct the altmap layout trivially >> >> base_pfn: start of the range to unplug >> end_pfn: base_pfn + nr_vmemmap_pages >> >> and pass that to the removal code, which will do the right thing, no? >> >> >> Sure, remembering the altmap might be a potential cleanup (eventually?), but the basic reasoning why this is required as patch #1 IMHO is wrong: if you say you support memmap_on_memory for a configuration, then you should also properly support it (allocate from the hotplugged memory), not silently fall back to something else. > > I guess you want to keep the altmap introduction as a later patch in the series and not the preparatory patch? Or are you ok with just adding the additional check I mentioned above w.r.t size value and keep this patch as patch 1 as a generic cleanup (avoiding > the recomputation of altmap->alloc/base_pfn/end_pfn? Yes, if it's not required better remove it completely from this patchset. We can alter discuss if keeping the altmap around is actually a cleanup or rather unnecessary.
On 7/7/23 5:47 PM, David Hildenbrand wrote: > On 06.07.23 18:06, Aneesh Kumar K V wrote: >> On 7/6/23 6:29 PM, David Hildenbrand wrote: >>> On 06.07.23 14:32, Aneesh Kumar K V wrote: >>>> On 7/6/23 4:44 PM, David Hildenbrand wrote: >>>>> 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. >>>>> >>>> >>>> But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require >>>> a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea >>>> of adding vmemmap_altmap to struct memory_block >>> >>> I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it). >>> >> >> Sure. How about? >> >> bool mhp_supports_memmap_on_memory(unsigned long size) >> { >> >> unsigned long nr_pages = size >> PAGE_SHIFT; >> unsigned long vmemmap_size = nr_pages * sizeof(struct page); >> >> if (!radix_enabled()) >> return false; >> /* >> * memmap on memory only supported with memory block size add/remove >> */ >> if (size != memory_block_size_bytes()) >> return false; >> /* >> * Also make sure the vmemmap allocation is fully contianed >> * so that we always allocate vmemmap memory from altmap area. >> */ >> if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE)) >> return false; >> /* >> * The pageblock alignment requirement is met by using >> * reserve blocks in altmap. >> */ >> return true; >> } > > Better, but the PAGE_SIZE that could be added to common code as well. > > ... but, the pageblock check in common code implies a PAGE_SIZE check, so why do we need any other check besides the radix_enabled() check for arm64 and just keep all the other checks in common code as they are? > > If your vmemmap does not cover full pageblocks (which implies full pages), the feature cannot be used *unless* we'd waste altmap space in the vmemmap to cover one pageblock. > > Wasting hotplugged memory certainly sounds wrong? > > > So I appreciate if you could explain why the pageblock check should not be had for ppc64? > If we want things to be aligned to pageblock (2M) we will have to use 2M vmemmap space and that implies a memory block of 2G with 64K page size. That requirements makes the feature not useful at all on power. The compromise i came to was what i mentioned in the commit message for enabling the feature on ppc64. We use altmap.reserve feature to align things correctly at pageblock granularity. We can end up loosing some pages in memory with this. For ex: with 256MB memory block size, we require 4 pages to map vmemmap pages, In order to align things correctly we end up adding a reserve of 28 pages. ie, for every 4096 pages 28 pages get reserved. -aneesh
On 07.07.23 15:30, Aneesh Kumar K V wrote: > On 7/7/23 5:47 PM, David Hildenbrand wrote: >> On 06.07.23 18:06, Aneesh Kumar K V wrote: >>> On 7/6/23 6:29 PM, David Hildenbrand wrote: >>>> On 06.07.23 14:32, Aneesh Kumar K V wrote: >>>>> On 7/6/23 4:44 PM, David Hildenbrand wrote: >>>>>> 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. >>>>>> >>>>> >>>>> But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require >>>>> a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea >>>>> of adding vmemmap_altmap to struct memory_block >>>> >>>> I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it). >>>> >>> >>> Sure. How about? >>> >>> bool mhp_supports_memmap_on_memory(unsigned long size) >>> { >>> >>> unsigned long nr_pages = size >> PAGE_SHIFT; >>> unsigned long vmemmap_size = nr_pages * sizeof(struct page); >>> >>> if (!radix_enabled()) >>> return false; >>> /* >>> * memmap on memory only supported with memory block size add/remove >>> */ >>> if (size != memory_block_size_bytes()) >>> return false; >>> /* >>> * Also make sure the vmemmap allocation is fully contianed >>> * so that we always allocate vmemmap memory from altmap area. >>> */ >>> if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE)) >>> return false; >>> /* >>> * The pageblock alignment requirement is met by using >>> * reserve blocks in altmap. >>> */ >>> return true; >>> } >> >> Better, but the PAGE_SIZE that could be added to common code as well. >> >> ... but, the pageblock check in common code implies a PAGE_SIZE check, so why do we need any other check besides the radix_enabled() check for arm64 and just keep all the other checks in common code as they are? >> >> If your vmemmap does not cover full pageblocks (which implies full pages), the feature cannot be used *unless* we'd waste altmap space in the vmemmap to cover one pageblock. >> >> Wasting hotplugged memory certainly sounds wrong? >> >> >> So I appreciate if you could explain why the pageblock check should not be had for ppc64? >> > > If we want things to be aligned to pageblock (2M) we will have to use 2M vmemmap space and that implies a memory block of 2G with 64K page size. That requirements makes the feature not useful at all > on power. The compromise i came to was what i mentioned in the commit message for enabling the feature on ppc64. As we'll always handle a 2M pageblock, you'll end up wasting memory. Assume a 64MiB memory block: With 64k: 1024 pages -> 64k vmemmap, almost 2 MiB wasted. ~3.1 % With 4k: 16384 pages -> 1 MiB vmemmap, 1 MiB wasted. ~1.5% It gets worse with smaller memory block sizes. > > We use altmap.reserve feature to align things correctly at pageblock granularity. We can end up loosing some pages in memory with this. For ex: with 256MB memory block > size, we require 4 pages to map vmemmap pages, In order to align things correctly we end up adding a reserve of 28 pages. ie, for every 4096 pages > 28 pages get reserved. You can simply align-up the nr_vmemmap_pages up to pageblocks in the memory hotplug code (e.g., depending on a config/arch knob whether wasting memory is supported). Because the pageblock granularity is a memory onlining/offlining limitation and should be checked+handled exactly there.
On 7/7/23 9:12 PM, David Hildenbrand wrote: > On 07.07.23 15:30, Aneesh Kumar K V wrote: >> On 7/7/23 5:47 PM, David Hildenbrand wrote: >>> On 06.07.23 18:06, Aneesh Kumar K V wrote: >>>> On 7/6/23 6:29 PM, David Hildenbrand wrote: >>>>> On 06.07.23 14:32, Aneesh Kumar K V wrote: >>>>>> On 7/6/23 4:44 PM, David Hildenbrand wrote: >>>>>>> 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. >>>>>>> >>>>>> >>>>>> But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require >>>>>> a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea >>>>>> of adding vmemmap_altmap to struct memory_block >>>>> >>>>> I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it). >>>>> >>>> >>>> Sure. How about? >>>> >>>> bool mhp_supports_memmap_on_memory(unsigned long size) >>>> { >>>> >>>> unsigned long nr_pages = size >> PAGE_SHIFT; >>>> unsigned long vmemmap_size = nr_pages * sizeof(struct page); >>>> >>>> if (!radix_enabled()) >>>> return false; >>>> /* >>>> * memmap on memory only supported with memory block size add/remove >>>> */ >>>> if (size != memory_block_size_bytes()) >>>> return false; >>>> /* >>>> * Also make sure the vmemmap allocation is fully contianed >>>> * so that we always allocate vmemmap memory from altmap area. >>>> */ >>>> if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE)) >>>> return false; >>>> /* >>>> * The pageblock alignment requirement is met by using >>>> * reserve blocks in altmap. >>>> */ >>>> return true; >>>> } >>> >>> Better, but the PAGE_SIZE that could be added to common code as well. >>> >>> ... but, the pageblock check in common code implies a PAGE_SIZE check, so why do we need any other check besides the radix_enabled() check for arm64 and just keep all the other checks in common code as they are? >>> >>> If your vmemmap does not cover full pageblocks (which implies full pages), the feature cannot be used *unless* we'd waste altmap space in the vmemmap to cover one pageblock. >>> >>> Wasting hotplugged memory certainly sounds wrong? >>> >>> >>> So I appreciate if you could explain why the pageblock check should not be had for ppc64? >>> >> >> If we want things to be aligned to pageblock (2M) we will have to use 2M vmemmap space and that implies a memory block of 2G with 64K page size. That requirements makes the feature not useful at all >> on power. The compromise i came to was what i mentioned in the commit message for enabling the feature on ppc64. > > As we'll always handle a 2M pageblock, you'll end up wasting memory. > > Assume a 64MiB memory block: > > With 64k: 1024 pages -> 64k vmemmap, almost 2 MiB wasted. ~3.1 % > With 4k: 16384 pages -> 1 MiB vmemmap, 1 MiB wasted. ~1.5% > > It gets worse with smaller memory block sizes. > > >> >> We use altmap.reserve feature to align things correctly at pageblock granularity. We can end up loosing some pages in memory with this. For ex: with 256MB memory block >> size, we require 4 pages to map vmemmap pages, In order to align things correctly we end up adding a reserve of 28 pages. ie, for every 4096 pages >> 28 pages get reserved. > > > You can simply align-up the nr_vmemmap_pages up to pageblocks in the memory hotplug code (e.g., depending on a config/arch knob whether wasting memory is supported). > > Because the pageblock granularity is a memory onlining/offlining limitation and should be checked+handled exactly there. That is what the changes in the patches are doing. A rewritten patch showing this exact details is below. If arch want's to avoid wasting pages due to this aligment they can add the page aligment restrictions in static inline bool arch_supports_memmap_on_memory(unsigned long size) { unsigned long nr_vmemmap_pages = size / PAGE_SIZE; unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page); unsigned long remaining_size = size - vmemmap_size; return IS_ALIGNED(vmemmap_size, PMD_SIZE) && IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT)); } modified mm/memory_hotplug.c @@ -1285,6 +1285,16 @@ bool mhp_supports_memmap_on_memory(unsigned long size) IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT)); } +unsigned long memory_block_align_base(unsigned long size) +{ + unsigned long align; + unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT; + unsigned long vmemmap_size = (nr_vmemmap_pages * sizeof(struct page)) >> PAGE_SHIFT; + + align = pageblock_align(vmemmap_size) - vmemmap_size; + return align; +} + /* * NOTE: The caller must call lock_device_hotplug() to serialize hotplug * and online/offline operations (triggered e.g. by sysfs). @@ -1295,7 +1305,11 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags) { struct mhp_params params = { .pgprot = pgprot_mhp(PAGE_KERNEL) }; enum memblock_flags memblock_flags = MEMBLOCK_NONE; - struct vmem_altmap mhp_altmap = {}; + struct vmem_altmap mhp_altmap = { + .base_pfn = PHYS_PFN(res->start), + .end_pfn = PHYS_PFN(res->end), + .reserve = memory_block_align_base(resource_size(res)), + }; struct memory_group *group = NULL; u64 start, size; bool new_node = false; @@ -1340,8 +1354,7 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags) */ if (mhp_flags & MHP_MEMMAP_ON_MEMORY) { if (mhp_supports_memmap_on_memory(size)) { - mhp_altmap.free = PHYS_PFN(size); - mhp_altmap.base_pfn = PHYS_PFN(start); + mhp_altmap.free = PHYS_PFN(size) - mhp_altmap.reserve; params.altmap = &mhp_altmap; } /* fallback to not using altmap */ @@ -1353,7 +1366,7 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags) goto error; /* create memory block devices after memory was added */ - ret = create_memory_block_devices(start, size, mhp_altmap.alloc, + ret = create_memory_block_devices(start, size, mhp_altmap.alloc + mhp_altmap.reserve, group); if (ret) { arch_remove_memory(start, size, NULL); @@ -2253,3 +2266,14 @@ int offline_and_remove_memory(u64 start, u64 size) } EXPORT_SYMBOL_GPL(offline_and_remove_memory); #endif /* CONFIG_MEMORY_HOTREMOVE */ + +static int __init memory_hotplug_init(void) +{ + if (mhp_memmap_on_memory()) { + pr_info("Memory hotplug will reserve %ld pages in each memory block\n", + memory_block_align_base(memory_block_size_bytes())); + + } + return 0; +} +module_init(memory_hotplug_init);
On 07.07.23 18:25, Aneesh Kumar K V wrote: > On 7/7/23 9:12 PM, David Hildenbrand wrote: >> On 07.07.23 15:30, Aneesh Kumar K V wrote: >>> On 7/7/23 5:47 PM, David Hildenbrand wrote: >>>> On 06.07.23 18:06, Aneesh Kumar K V wrote: >>>>> On 7/6/23 6:29 PM, David Hildenbrand wrote: >>>>>> On 06.07.23 14:32, Aneesh Kumar K V wrote: >>>>>>> On 7/6/23 4:44 PM, David Hildenbrand wrote: >>>>>>>> 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. >>>>>>>> >>>>>>> >>>>>>> But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require >>>>>>> a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea >>>>>>> of adding vmemmap_altmap to struct memory_block >>>>>> >>>>>> I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it). >>>>>> >>>>> >>>>> Sure. How about? >>>>> >>>>> bool mhp_supports_memmap_on_memory(unsigned long size) >>>>> { >>>>> >>>>> unsigned long nr_pages = size >> PAGE_SHIFT; >>>>> unsigned long vmemmap_size = nr_pages * sizeof(struct page); >>>>> >>>>> if (!radix_enabled()) >>>>> return false; >>>>> /* >>>>> * memmap on memory only supported with memory block size add/remove >>>>> */ >>>>> if (size != memory_block_size_bytes()) >>>>> return false; >>>>> /* >>>>> * Also make sure the vmemmap allocation is fully contianed >>>>> * so that we always allocate vmemmap memory from altmap area. >>>>> */ >>>>> if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE)) >>>>> return false; >>>>> /* >>>>> * The pageblock alignment requirement is met by using >>>>> * reserve blocks in altmap. >>>>> */ >>>>> return true; >>>>> } >>>> >>>> Better, but the PAGE_SIZE that could be added to common code as well. >>>> >>>> ... but, the pageblock check in common code implies a PAGE_SIZE check, so why do we need any other check besides the radix_enabled() check for arm64 and just keep all the other checks in common code as they are? >>>> >>>> If your vmemmap does not cover full pageblocks (which implies full pages), the feature cannot be used *unless* we'd waste altmap space in the vmemmap to cover one pageblock. >>>> >>>> Wasting hotplugged memory certainly sounds wrong? >>>> >>>> >>>> So I appreciate if you could explain why the pageblock check should not be had for ppc64? >>>> >>> >>> If we want things to be aligned to pageblock (2M) we will have to use 2M vmemmap space and that implies a memory block of 2G with 64K page size. That requirements makes the feature not useful at all >>> on power. The compromise i came to was what i mentioned in the commit message for enabling the feature on ppc64. >> >> As we'll always handle a 2M pageblock, you'll end up wasting memory. >> >> Assume a 64MiB memory block: >> >> With 64k: 1024 pages -> 64k vmemmap, almost 2 MiB wasted. ~3.1 % >> With 4k: 16384 pages -> 1 MiB vmemmap, 1 MiB wasted. ~1.5% >> >> It gets worse with smaller memory block sizes. >> >> >>> >>> We use altmap.reserve feature to align things correctly at pageblock granularity. We can end up loosing some pages in memory with this. For ex: with 256MB memory block >>> size, we require 4 pages to map vmemmap pages, In order to align things correctly we end up adding a reserve of 28 pages. ie, for every 4096 pages >>> 28 pages get reserved. >> >> >> You can simply align-up the nr_vmemmap_pages up to pageblocks in the memory hotplug code (e.g., depending on a config/arch knob whether wasting memory is supported). >> >> Because the pageblock granularity is a memory onlining/offlining limitation and should be checked+handled exactly there. > > That is what the changes in the patches are doing. A rewritten patch showing this exact details is below. If arch want's to avoid > wasting pages due to this aligment they can add the page aligment restrictions in > > static inline bool arch_supports_memmap_on_memory(unsigned long size) > { > unsigned long nr_vmemmap_pages = size / PAGE_SIZE; > unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page); > unsigned long remaining_size = size - vmemmap_size; > > return IS_ALIGNED(vmemmap_size, PMD_SIZE) && > IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT)); > } I tend towards that this should be a config option (something that expresses that wasting memory is acceptable), then we can move it to common code. There, we simply allow aligning the vmemmap size up to the next pageblock (if the config allows for it). Further, we have to make sure that our single memblock is not a single pageblock (and adding memory would imply only consuming memmap and not providing any memory).
diff --git a/drivers/base/memory.c b/drivers/base/memory.c index b456ac213610..523cc1d37c81 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -106,6 +106,7 @@ static void memory_block_release(struct device *dev) { struct memory_block *mem = to_memory_block(dev); + WARN(mem->altmap.alloc, "Altmap not fully unmapped"); kfree(mem); } @@ -183,7 +184,7 @@ static int memory_block_online(struct memory_block *mem) { unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr); unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block; - unsigned long nr_vmemmap_pages = mem->nr_vmemmap_pages; + unsigned long nr_vmemmap_pages = 0; struct zone *zone; int ret; @@ -200,6 +201,9 @@ static int memory_block_online(struct memory_block *mem) * stage helps to keep accounting easier to follow - e.g vmemmaps * belong to the same zone as the memory they backed. */ + if (mem->altmap.alloc) + nr_vmemmap_pages = mem->altmap.alloc + mem->altmap.reserve; + if (nr_vmemmap_pages) { ret = mhp_init_memmap_on_memory(start_pfn, nr_vmemmap_pages, zone); if (ret) @@ -230,7 +234,7 @@ static int memory_block_offline(struct memory_block *mem) { unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr); unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block; - unsigned long nr_vmemmap_pages = mem->nr_vmemmap_pages; + unsigned long nr_vmemmap_pages = 0; int ret; if (!mem->zone) @@ -240,6 +244,9 @@ static int memory_block_offline(struct memory_block *mem) * Unaccount before offlining, such that unpopulated zone and kthreads * can properly be torn down in offline_pages(). */ + if (mem->altmap.alloc) + nr_vmemmap_pages = mem->altmap.alloc + mem->altmap.reserve; + if (nr_vmemmap_pages) adjust_present_page_count(pfn_to_page(start_pfn), mem->group, -nr_vmemmap_pages); @@ -726,7 +733,7 @@ void memory_block_add_nid(struct memory_block *mem, int nid, #endif static int add_memory_block(unsigned long block_id, unsigned long state, - unsigned long nr_vmemmap_pages, + struct vmem_altmap *altmap, struct memory_group *group) { struct memory_block *mem; @@ -744,7 +751,10 @@ static int add_memory_block(unsigned long block_id, unsigned long state, mem->start_section_nr = block_id * sections_per_block; mem->state = state; mem->nid = NUMA_NO_NODE; - mem->nr_vmemmap_pages = nr_vmemmap_pages; + if (altmap) + memcpy(&mem->altmap, altmap, sizeof(*altmap)); + else + mem->altmap.alloc = 0; INIT_LIST_HEAD(&mem->group_next); #ifndef CONFIG_NUMA @@ -783,14 +793,14 @@ static int __init add_boot_memory_block(unsigned long base_section_nr) if (section_count == 0) return 0; return add_memory_block(memory_block_id(base_section_nr), - MEM_ONLINE, 0, NULL); + MEM_ONLINE, NULL, NULL); } static int add_hotplug_memory_block(unsigned long block_id, - unsigned long nr_vmemmap_pages, + struct vmem_altmap *altmap, struct memory_group *group) { - return add_memory_block(block_id, MEM_OFFLINE, nr_vmemmap_pages, group); + return add_memory_block(block_id, MEM_OFFLINE, altmap, group); } static void remove_memory_block(struct memory_block *memory) @@ -818,7 +828,7 @@ static void remove_memory_block(struct memory_block *memory) * Called under device_hotplug_lock. */ int create_memory_block_devices(unsigned long start, unsigned long size, - unsigned long vmemmap_pages, + struct vmem_altmap *altmap, struct memory_group *group) { const unsigned long start_block_id = pfn_to_block_id(PFN_DOWN(start)); @@ -832,7 +842,7 @@ int create_memory_block_devices(unsigned long start, unsigned long size, return -EINVAL; for (block_id = start_block_id; block_id != end_block_id; block_id++) { - ret = add_hotplug_memory_block(block_id, vmemmap_pages, group); + ret = add_hotplug_memory_block(block_id, altmap, group); if (ret) break; } diff --git a/include/linux/memory.h b/include/linux/memory.h index 31343566c221..87f12924250f 100644 --- a/include/linux/memory.h +++ b/include/linux/memory.h @@ -64,6 +64,23 @@ struct memory_group { }; }; +/** + * 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; +}; + 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, - unsigned long vmemmap_pages, + struct vmem_altmap *altmap, struct memory_group *group); void remove_memory_block_devices(unsigned long start, unsigned long size); extern void memory_dev_init(void); diff --git a/include/linux/memremap.h b/include/linux/memremap.h index 1314d9c5f05b..4cb326f85302 100644 --- a/include/linux/memremap.h +++ b/include/linux/memremap.h @@ -2,6 +2,7 @@ #ifndef _LINUX_MEMREMAP_H_ #define _LINUX_MEMREMAP_H_ +#include <linux/memory.h> #include <linux/mmzone.h> #include <linux/range.h> #include <linux/ioport.h> @@ -10,23 +11,6 @@ struct resource; struct device; -/** - * 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; -}; - /* * Specialize ZONE_DEVICE memory into multiple types each has a different * usage. diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 3f231cf1b410..c4bac38cc147 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1354,7 +1354,7 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags) goto error; /* create memory block devices after memory was added */ - ret = create_memory_block_devices(start, size, mhp_altmap.alloc, + ret = create_memory_block_devices(start, size, &mhp_altmap, group); if (ret) { arch_remove_memory(start, size, NULL); @@ -1956,12 +1956,18 @@ static int check_memblock_offlined_cb(struct memory_block *mem, void *arg) return 0; } -static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg) +static int get_vmemmap_altmap_cb(struct memory_block *mem, void *arg) { + struct vmem_altmap **altmap = (struct vmem_altmap **)arg; /* - * If not set, continue with the next block. + * If we have any pages allocated from altmap + * return the altmap details and break callback. */ - return mem->nr_vmemmap_pages; + if (mem->altmap.alloc) { + *altmap = &mem->altmap; + return 1; + } + return 0; } 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. */ + if (mhp_memmap_on_memory()) { - nr_vmemmap_pages = walk_memory_blocks(start, size, NULL, - get_nr_vmemmap_pages_cb); - if (nr_vmemmap_pages) { + ret = walk_memory_blocks(start, size, &altmap, get_vmemmap_altmap_cb); + if (ret) { if (size != memory_block_size_bytes()) { pr_warn("Refuse to remove %#llx - %#llx," "wrong granularity\n", start, start + size); return -EINVAL; } - - /* - * Let remove_pmd_table->free_hugepage_table do the - * right thing if we used vmem_altmap when hot-adding - * the range. - */ - mhp_altmap.alloc = nr_vmemmap_pages; - altmap = &mhp_altmap; } }
With memmap on memory, some architecture needs more details w.r.t altmap such as base_pfn, end_pfn, etc to unmap vmemmap memory. Embed vmem_altmap data structure to memory_bock and use that instead of nr_vmemmap_pages. On memory unplug, if the kernel finds any memory block in the range to be using vmem_altmap, the kernel fails to unplug the memory if the request is not a single memory block unplug. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- drivers/base/memory.c | 28 +++++++++++++++++++--------- include/linux/memory.h | 25 +++++++++++++++++++------ include/linux/memremap.h | 18 +----------------- mm/memory_hotplug.c | 31 ++++++++++++++----------------- 4 files changed, 53 insertions(+), 49 deletions(-)