David Hildenbrand
2020-Jul-31 09:18 UTC
[PATCH RFCv1 0/5] mm/memory_hotplug: selective merging of memory resources
Some add_memory*() users add memory in small, contiguous memory blocks. Examples include virtio-mem, hyper-v balloon, and the XEN balloon. This can quickly result in a lot of memory resources, whereby the actual resource boundaries are not of interest (e.g., it might be relevant for DIMMs, exposed via /proc/iomem to user space). We really want to merge added resources in this scenario where possible. Resources are effectively stored in a list-based tree. Having a lot of resources not only wastes memory, it also makes traversing that tree more expensive, and makes /proc/iomem explode in size (e.g., requiring kexec-tools to manually merge resources when creating a kdump header. The current kexec-tools resource count limit does not allow more than ~100GB of memory with a memory block size of 128MB on x86-64). Let's allow to selectively merge resources, speciyfing a parent node and a resource idendifier string. The memory unplug path will properly split up merged resources again. Patch #3 contains a /proc/iomem example. Only tested with virtio-mem. Note: This gets the job done and is comparably simple. More complicated approaches would require introducing IORESOURCE_MERGEABLE and extending our add_memory*() interfaces with a flag, specifying that merging after adding succeeded is acceptable. I'd like to avoid that complexity and code churn for now. David Hildenbrand (5): kernel/resource: make release_mem_region_adjustable() never fail kernel/resource: merge_child_mem_resources() to merge memory resources after adding succeeded virtio-mem: try to merge "System RAM (virtio_mem)" resources xen/balloon: try to merge "System RAM" resources hv_balloon:: try to merge "System RAM" resources drivers/hv/hv_balloon.c | 3 ++ drivers/virtio/virtio_mem.c | 14 ++++- drivers/xen/balloon.c | 4 ++ include/linux/ioport.h | 7 ++- kernel/resource.c | 105 ++++++++++++++++++++++++++++-------- mm/memory_hotplug.c | 22 +------- 6 files changed, 109 insertions(+), 46 deletions(-) -- 2.26.2
David Hildenbrand
2020-Jul-31 09:18 UTC
[PATCH RFCv1 1/5] kernel/resource: make release_mem_region_adjustable() never fail
Let's make sure splitting a resource on memory hotunplug will never fail. This will become more relevant once we merge selected system ram resources - then, we'll trigger that case more often un memory unplug. In general, this function is already unlikely to fail. When we remove memory, we free up quite a lot of metadata (memmap, page tables, memory block device, etc.). All other error cases inside release_mem_region_adjustable() seem to be sanity checks if the function would be abused in different context - let's add WARN_ON_ONCE(). Cc: Andrew Morton <akpm at linux-foundation.org> Cc: Michal Hocko <mhocko at suse.com> Cc: Dan Williams <dan.j.williams at intel.com> Cc: Jason Gunthorpe <jgg at ziepe.ca> Cc: Kees Cook <keescook at chromium.org> Cc: Ard Biesheuvel <ardb at kernel.org> Cc: Wei Yang <richardw.yang at linux.intel.com> Signed-off-by: David Hildenbrand <david at redhat.com> --- include/linux/ioport.h | 4 ++-- kernel/resource.c | 49 ++++++++++++++++++++++++------------------ mm/memory_hotplug.c | 22 +------------------ 3 files changed, 31 insertions(+), 44 deletions(-) diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 6c2b06fe8beb7..52a91f5fa1a36 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -248,8 +248,8 @@ extern struct resource * __request_region(struct resource *, extern void __release_region(struct resource *, resource_size_t, resource_size_t); #ifdef CONFIG_MEMORY_HOTREMOVE -extern int release_mem_region_adjustable(struct resource *, resource_size_t, - resource_size_t); +extern void release_mem_region_adjustable(struct resource *, resource_size_t, + resource_size_t); #endif /* Wrappers for managed devices */ diff --git a/kernel/resource.c b/kernel/resource.c index 841737bbda9e5..249c6b54014de 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1255,21 +1255,28 @@ EXPORT_SYMBOL(__release_region); * assumes that all children remain in the lower address entry for * simplicity. Enhance this logic when necessary. */ -int release_mem_region_adjustable(struct resource *parent, - resource_size_t start, resource_size_t size) +void release_mem_region_adjustable(struct resource *parent, + resource_size_t start, resource_size_t size) { + struct resource *new_res = NULL; + bool alloc_nofail = false; struct resource **p; struct resource *res; - struct resource *new_res; resource_size_t end; - int ret = -EINVAL; end = start + size - 1; - if ((start < parent->start) || (end > parent->end)) - return ret; + if (WARN_ON_ONCE((start < parent->start) || (end > parent->end))) + return; - /* The alloc_resource() result gets checked later */ - new_res = alloc_resource(GFP_KERNEL); + /* + * We free up quite a lot of memory on memory hotunplug (esp. memap), + * just before releasing the region. This is highly unlikely to + * fail - let's play save and make it never fail as the caller cannot + * perform any error handling (e.g., trying to re-add memory will fail + * similarly). + */ +retry: + new_res = alloc_resource(GFP_KERNEL | alloc_nofail ? __GFP_NOFAIL : 0); p = &parent->child; write_lock(&resource_lock); @@ -1295,7 +1302,6 @@ int release_mem_region_adjustable(struct resource *parent, * so if we are dealing with them, let us just back off here. */ if (!(res->flags & IORESOURCE_SYSRAM)) { - ret = 0; break; } @@ -1312,20 +1318,23 @@ int release_mem_region_adjustable(struct resource *parent, /* free the whole entry */ *p = res->sibling; free_resource(res); - ret = 0; } else if (res->start == start && res->end != end) { /* adjust the start */ - ret = __adjust_resource(res, end + 1, - res->end - end); + WARN_ON_ONCE(__adjust_resource(res, end + 1, + res->end - end)); } else if (res->start != start && res->end == end) { /* adjust the end */ - ret = __adjust_resource(res, res->start, - start - res->start); + WARN_ON_ONCE(__adjust_resource(res, res->start, + start - res->start)); } else { - /* split into two entries */ + /* split into two entries - we need a new resource */ if (!new_res) { - ret = -ENOMEM; - break; + new_res = alloc_resource(GFP_ATOMIC); + if (!new_res) { + alloc_nofail = true; + write_unlock(&resource_lock); + goto retry; + } } new_res->name = res->name; new_res->start = end + 1; @@ -1336,9 +1345,8 @@ int release_mem_region_adjustable(struct resource *parent, new_res->sibling = res->sibling; new_res->child = NULL; - ret = __adjust_resource(res, res->start, - start - res->start); - if (ret) + if (WARN_ON_ONCE(__adjust_resource(res, res->start, + start - res->start))) break; res->sibling = new_res; new_res = NULL; @@ -1349,7 +1357,6 @@ int release_mem_region_adjustable(struct resource *parent, write_unlock(&resource_lock); free_resource(new_res); - return ret; } #endif /* CONFIG_MEMORY_HOTREMOVE */ diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index da374cd3d45b3..258656b819dbe 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1709,26 +1709,6 @@ void try_offline_node(int nid) } EXPORT_SYMBOL(try_offline_node); -static void __release_memory_resource(resource_size_t start, - resource_size_t size) -{ - int ret; - - /* - * When removing memory in the same granularity as it was added, - * this function never fails. It might only fail if resources - * have to be adjusted or split. We'll ignore the error, as - * removing of memory cannot fail. - */ - ret = release_mem_region_adjustable(&iomem_resource, start, size); - if (ret) { - resource_size_t endres = start + size - 1; - - pr_warn("Unable to release resource <%pa-%pa> (%d)\n", - &start, &endres, ret); - } -} - static int __ref try_remove_memory(int nid, u64 start, u64 size) { int rc = 0; @@ -1762,7 +1742,7 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size) memblock_remove(start, size); } - __release_memory_resource(start, size); + release_mem_region_adjustable(&iomem_resource, start, size); try_offline_node(nid); -- 2.26.2
David Hildenbrand
2020-Jul-31 09:18 UTC
[PATCH RFCv1 2/5] kernel/resource: merge_child_mem_resources() to merge memory resources after adding succeeded
Some add_memory*() users add memory in small, contiguous memory blocks. Examples include virtio-mem, hyper-v balloon, and the XEN balloon. This can quickly result in a lot of memory resources, whereby the actual resource boundaries are not of interest (e.g., it might be relevant for DIMMs, exposed via /proc/iomem to user space). We really want to merge added resources in this scenario where possible. Let's provide an interface to trigger merging of applicable child resources. It will be, for example, used by virtio-mem to trigger merging of memory resources it added (via add_memory_driver() managed) to its resource container. Note: We really want to merge after the whole operation succeeded, not directly when adding a resource to the resource tree (it would break add_memory_resource() and require splitting resources again when the operation failed - e.g., due to -ENOMEM). Cc: Andrew Morton <akpm at linux-foundation.org> Cc: Michal Hocko <mhocko at suse.com> Cc: Dan Williams <dan.j.williams at intel.com> Cc: Jason Gunthorpe <jgg at ziepe.ca> Cc: Kees Cook <keescook at chromium.org> Cc: Ard Biesheuvel <ardb at kernel.org> Cc: Thomas Gleixner <tglx at linutronix.de> Cc: "K. Y. Srinivasan" <kys at microsoft.com> Cc: Haiyang Zhang <haiyangz at microsoft.com> Cc: Stephen Hemminger <sthemmin at microsoft.com> Cc: Wei Liu <wei.liu at kernel.org> Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com> Cc: Juergen Gross <jgross at suse.com> Cc: Stefano Stabellini <sstabellini at kernel.org> Cc: Roger Pau Monn? <roger.pau at citrix.com> Cc: Julien Grall <julien at xen.org> Signed-off-by: David Hildenbrand <david at redhat.com> --- include/linux/ioport.h | 3 +++ kernel/resource.c | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 52a91f5fa1a36..743b87fe2205b 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -251,6 +251,9 @@ extern void __release_region(struct resource *, resource_size_t, extern void release_mem_region_adjustable(struct resource *, resource_size_t, resource_size_t); #endif +#ifdef CONFIG_MEMORY_HOTPLUG +extern void merge_child_mem_resources(struct resource *res, const char *name); +#endif /* Wrappers for managed devices */ struct device; diff --git a/kernel/resource.c b/kernel/resource.c index 249c6b54014de..01ecc5b7956f5 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1360,6 +1360,62 @@ void release_mem_region_adjustable(struct resource *parent, } #endif /* CONFIG_MEMORY_HOTREMOVE */ +#ifdef CONFIG_MEMORY_HOTPLUG +static bool mem_resources_mergeable(struct resource *r1, struct resource *r2) +{ + return r1->end + 1 == r2->start && + r1->name == r2->name && + r1->flags == r2->flags && + (r1->flags & IORESOURCE_MEM) && + r1->desc == r2->desc && + !r1->child && !r2->child; +} + +/* + * merge_child_mem_resources - try to merge contiguous child IORESOURCE_MEM + * resources with the given name that match all + * other properties + * @parent: parent resource descriptor + * @name: name of the child resources to consider for merging + * + * This interface is intended for memory hotplug, whereby lots of consecutive + * memory resources are added (e.g., via add_memory*()) by a driver, and the + * actual resource boundaries are not of interest (e.g., it might be + * relevant for DIMMs). Only immediate child resources are considered. All + * applicable child resources must be immutable during the request. + * + * Note: + * - The caller has to make sure that no pointers to resources that might + * get merged are held anymore. Callers should only trigger merging of child + * resources when they are the only one adding such resources to the parent. + * E.g., if two mechanisms could add "System RAM" immediately below the + * same parent, this function is not safe to use. + * - release_mem_region_adjustable() will split on demand on memory hotunplug + */ +void merge_child_mem_resources(struct resource *parent, const char *name) +{ + struct resource *cur, *next; + + write_lock(&resource_lock); + + cur = parent->child; + while (cur && cur->sibling) { + next = cur->sibling; + if (!strcmp(cur->name, name) && + mem_resources_mergeable(cur, next)) { + cur->end = next->end; + cur->sibling = next->sibling; + free_resource(next); + next = cur->sibling; + } + cur = next; + } + + write_unlock(&resource_lock); +} +EXPORT_SYMBOL(merge_child_mem_resources); +#endif /* CONFIG_MEMORY_HOTPLUG */ + /* * Managed region resource */ -- 2.26.2
David Hildenbrand
2020-Jul-31 09:18 UTC
[PATCH RFCv1 3/5] virtio-mem: try to merge "System RAM (virtio_mem)" resources
virtio-mem adds memory in memory block granularity, to be able to remove it in the same granularity again later, and to grow slowly on demand. This, however, results in quite a lot of resources when adding a lot of memory. Resources are effectively stored in a list-based tree. Having a lot of resources not only wastes memory, it also makes traversing that tree more expensive, and makes /proc/iomem explode in size (e.g., requiring kexec-tools to manually merge resources later when e.g., trying to create a kdump header). Before this patch, we get (/proc/iomem) when hotplugging 2G via virtio-mem on x86-64: [...] 100000000-13fffffff : System RAM 140000000-33fffffff : virtio0 140000000-147ffffff : System RAM (virtio_mem) 148000000-14fffffff : System RAM (virtio_mem) 150000000-157ffffff : System RAM (virtio_mem) 158000000-15fffffff : System RAM (virtio_mem) 160000000-167ffffff : System RAM (virtio_mem) 168000000-16fffffff : System RAM (virtio_mem) 170000000-177ffffff : System RAM (virtio_mem) 178000000-17fffffff : System RAM (virtio_mem) 180000000-187ffffff : System RAM (virtio_mem) 188000000-18fffffff : System RAM (virtio_mem) 190000000-197ffffff : System RAM (virtio_mem) 198000000-19fffffff : System RAM (virtio_mem) 1a0000000-1a7ffffff : System RAM (virtio_mem) 1a8000000-1afffffff : System RAM (virtio_mem) 1b0000000-1b7ffffff : System RAM (virtio_mem) 1b8000000-1bfffffff : System RAM (virtio_mem) 3280000000-32ffffffff : PCI Bus 0000:00 With this patch, we get (/proc/iomem): [...] fffc0000-ffffffff : Reserved 100000000-13fffffff : System RAM 140000000-33fffffff : virtio0 140000000-1bfffffff : System RAM (virtio_mem) 3280000000-32ffffffff : PCI Bus 0000:00 Of course, with more hotplugged memory, it gets worse. When unplugging memory blocks again, try_remove_memory() (via offline_and_remove_memory()) will properly split the resource up again. Signed-off-by: David Hildenbrand <david at redhat.com> --- drivers/virtio/virtio_mem.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c index f26f5f64ae822..2396a8d67875e 100644 --- a/drivers/virtio/virtio_mem.c +++ b/drivers/virtio/virtio_mem.c @@ -415,6 +415,7 @@ static int virtio_mem_mb_add(struct virtio_mem *vm, unsigned long mb_id) { const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id); int nid = vm->nid; + int rc; if (nid == NUMA_NO_NODE) nid = memory_add_physaddr_to_nid(addr); @@ -431,8 +432,17 @@ static int virtio_mem_mb_add(struct virtio_mem *vm, unsigned long mb_id) } dev_dbg(&vm->vdev->dev, "adding memory block: %lu\n", mb_id); - return add_memory_driver_managed(nid, addr, memory_block_size_bytes(), - vm->resource_name); + rc = add_memory_driver_managed(nid, addr, memory_block_size_bytes(), + vm->resource_name); + if (!rc) { + /* + * Try to reduce the number of resources by merging them. The + * memory removal path will properly split them up again. + */ + merge_child_mem_resources(vm->parent_resource, + vm->resource_name); + } + return rc; } /* -- 2.26.2
David Hildenbrand
2020-Jul-31 09:18 UTC
[PATCH RFCv1 4/5] xen/balloon: try to merge "System RAM" resources
Let's reuse the new mechanism to merge "System RAM" resources below the root. We are the only one hotplugging "System RAM" and DIMMs don't apply, so this is safe to use. Cc: Andrew Morton <akpm at linux-foundation.org> Cc: Michal Hocko <mhocko at suse.com> Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com> Cc: Juergen Gross <jgross at suse.com> Cc: Stefano Stabellini <sstabellini at kernel.org> Cc: Roger Pau Monn? <roger.pau at citrix.com> Cc: Julien Grall <julien at xen.org> Signed-off-by: David Hildenbrand <david at redhat.com> --- drivers/xen/balloon.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c index 77c57568e5d7f..644ae2e3798e2 100644 --- a/drivers/xen/balloon.c +++ b/drivers/xen/balloon.c @@ -353,6 +353,10 @@ static enum bp_state reserve_additional_memory(void) if (rc) { pr_warn("Cannot add additional memory (%i)\n", rc); goto err; + } else { + resource = NULL; + /* Try to reduce the number of "System RAM" resources. */ + merge_child_mem_resources(&iomem_resource, "System RAM"); } balloon_stats.total_pages += balloon_hotplug; -- 2.26.2
David Hildenbrand
2020-Jul-31 09:18 UTC
[PATCH RFCv1 5/5] hv_balloon:: try to merge "System RAM" resources
Let's reuse the new mechanism to merge "System RAM" resources below the root. We are the only one hotplugging "System RAM" and DIMMs don't apply, so this is safe to use. Cc: Andrew Morton <akpm at linux-foundation.org> Cc: Michal Hocko <mhocko at suse.com> Cc: "K. Y. Srinivasan" <kys at microsoft.com> Cc: Haiyang Zhang <haiyangz at microsoft.com> Cc: Stephen Hemminger <sthemmin at microsoft.com> Cc: Wei Liu <wei.liu at kernel.org> Signed-off-by: David Hildenbrand <david at redhat.com> --- drivers/hv/hv_balloon.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c index 32e3bc0aa665a..0745f7cc1727b 100644 --- a/drivers/hv/hv_balloon.c +++ b/drivers/hv/hv_balloon.c @@ -745,6 +745,9 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size, has->covered_end_pfn -= processed_pfn; spin_unlock_irqrestore(&dm_device.ha_lock, flags); break; + } else { + /* Try to reduce the number of "System RAM" resources. */ + merge_child_mem_resources(&iomem_resource, "System RAM"); } /* -- 2.26.2
David Hildenbrand
2020-Jul-31 10:40 UTC
[PATCH RFCv1 3/5] virtio-mem: try to merge "System RAM (virtio_mem)" resources
On 31.07.20 11:18, David Hildenbrand wrote: Grml, forgot to add cc: list for this patch, ccing the right people.> virtio-mem adds memory in memory block granularity, to be able to > remove it in the same granularity again later, and to grow slowly on > demand. This, however, results in quite a lot of resources when > adding a lot of memory. Resources are effectively stored in a list-based > tree. Having a lot of resources not only wastes memory, it also makes > traversing that tree more expensive, and makes /proc/iomem explode in > size (e.g., requiring kexec-tools to manually merge resources later > when e.g., trying to create a kdump header). > > Before this patch, we get (/proc/iomem) when hotplugging 2G via virtio-mem > on x86-64: > [...] > 100000000-13fffffff : System RAM > 140000000-33fffffff : virtio0 > 140000000-147ffffff : System RAM (virtio_mem) > 148000000-14fffffff : System RAM (virtio_mem) > 150000000-157ffffff : System RAM (virtio_mem) > 158000000-15fffffff : System RAM (virtio_mem) > 160000000-167ffffff : System RAM (virtio_mem) > 168000000-16fffffff : System RAM (virtio_mem) > 170000000-177ffffff : System RAM (virtio_mem) > 178000000-17fffffff : System RAM (virtio_mem) > 180000000-187ffffff : System RAM (virtio_mem) > 188000000-18fffffff : System RAM (virtio_mem) > 190000000-197ffffff : System RAM (virtio_mem) > 198000000-19fffffff : System RAM (virtio_mem) > 1a0000000-1a7ffffff : System RAM (virtio_mem) > 1a8000000-1afffffff : System RAM (virtio_mem) > 1b0000000-1b7ffffff : System RAM (virtio_mem) > 1b8000000-1bfffffff : System RAM (virtio_mem) > 3280000000-32ffffffff : PCI Bus 0000:00 > > With this patch, we get (/proc/iomem): > [...] > fffc0000-ffffffff : Reserved > 100000000-13fffffff : System RAM > 140000000-33fffffff : virtio0 > 140000000-1bfffffff : System RAM (virtio_mem) > 3280000000-32ffffffff : PCI Bus 0000:00 > > Of course, with more hotplugged memory, it gets worse. When unplugging > memory blocks again, try_remove_memory() (via > offline_and_remove_memory()) will properly split the resource up again. > > Signed-off-by: David Hildenbrand <david at redhat.com> > --- > drivers/virtio/virtio_mem.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c > index f26f5f64ae822..2396a8d67875e 100644 > --- a/drivers/virtio/virtio_mem.c > +++ b/drivers/virtio/virtio_mem.c > @@ -415,6 +415,7 @@ static int virtio_mem_mb_add(struct virtio_mem *vm, unsigned long mb_id) > { > const uint64_t addr = virtio_mem_mb_id_to_phys(mb_id); > int nid = vm->nid; > + int rc; > > if (nid == NUMA_NO_NODE) > nid = memory_add_physaddr_to_nid(addr); > @@ -431,8 +432,17 @@ static int virtio_mem_mb_add(struct virtio_mem *vm, unsigned long mb_id) > } > > dev_dbg(&vm->vdev->dev, "adding memory block: %lu\n", mb_id); > - return add_memory_driver_managed(nid, addr, memory_block_size_bytes(), > - vm->resource_name); > + rc = add_memory_driver_managed(nid, addr, memory_block_size_bytes(), > + vm->resource_name); > + if (!rc) { > + /* > + * Try to reduce the number of resources by merging them. The > + * memory removal path will properly split them up again. > + */ > + merge_child_mem_resources(vm->parent_resource, > + vm->resource_name); > + } > + return rc; > } > > /* >-- Thanks, David / dhildenb
Reasonably Related Threads
- [PATCH v1] virtio-mem: add memory via add_memory_driver_managed()
- [PATCH RFCv1 0/5] mm/memory_hotplug: selective merging of memory resources
- [PATCH v1] virtio-mem: add memory via add_memory_driver_managed()
- [PATCH v1] virtio-mem: add memory via add_memory_driver_managed()
- [PATCH v2 3/3] device-dax: Add system ram (add_memory()) with MHP_NO_FIRMWARE_MEMMAP