On Tue, 2019-04-09 at 12:01 +0200, David Hildenbrand wrote:
> __add_pages() doesn't add the memory resource, so __remove_pages()
> shouldn't remove it. Let's factor it out. Especially as it is a
> special
> case for memory used as system memory, added via add_memory() and
> friends.
I would call the special case the other way, aka: zone_device hooking
into hotplug path.
>
> We now remove the resource after removing the sections instead of
> doing
> it the other way around. I don't think this change is problematic.
>
> add_memory()
> register memory resource
> arch_add_memory()
>
> remove_memory
> arch_remove_memory()
> release memory resource
>
> While at it, explain why we ignore errors and that it only happeny if
> we remove memory in a different granularity as we added it.
In the future we may want to allow drivers to hook directly into
arch_add_memory()/arch_remove_memory(), and this will lead to different
granularity in hot_add/hot_remove operations.
At least that was one of the conclusions I drew from the last vmemmap-
patchset.
So, we will have to see how we can handle those kind of errors.
>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Oscar Salvador <osalvador@xxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> Cc: Wei Yang <richard.weiyang@xxxxxxxxx>
> Cc: Qian Cai <cai@xxxxxx>
> Cc: Arun KS <arunks@xxxxxxxxxxxxxx>
> Cc: Mathieu Malaterre <malat@xxxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
Besides what Andrew pointed out about the types of start,size, I do not
see anything wrong:
Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>
> ---
> mm/memory_hotplug.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4970ff658055..696ed7ee5e28 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -562,20 +562,6 @@ int __remove_pages(struct zone *zone, unsigned
> long phys_start_pfn,
> if (is_dev_zone(zone)) {
> if (altmap)
> map_offset = vmem_altmap_offset(altmap);
> - } else {
> - resource_size_t start, size;
> -
> - start = phys_start_pfn << PAGE_SHIFT;
> - size = nr_pages * PAGE_SIZE;
> -
> - 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);
> - }
> }
>
> clear_zone_contiguous(zone);
> @@ -1820,6 +1806,25 @@ void try_offline_node(int nid)
> }
> EXPORT_SYMBOL(try_offline_node);
>
> +static void __release_memory_resource(u64 start, u64 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);
> + }
> +}
> +
> /**
> * remove_memory
> * @nid: the node ID
> @@ -1854,6 +1859,7 @@ void __ref __remove_memory(int nid, u64 start,
> u64 size)
> memblock_remove(start, size);
>
> arch_remove_memory(nid, start, size, NULL);
> + __release_memory_resource(start, size);
>
> try_offline_node(nid);
>
--
Oscar Salvador
SUSE L3
