On Fri 17-05-19 17:54:37, Pavel Tatashin wrote:
> As of right now remove_memory() interface is inherently broken. It tries
> to remove memory but panics if some memory is not offline. The problem
> is that it is impossible to ensure that all memory blocks are offline as
> this function also takes lock_device_hotplug that is required to
> change memory state via sysfs.
>
> So, between calling this function and offlining all memory blocks there
> is always a window when lock_device_hotplug is released, and therefore,
> there is always a chance for a panic during this window.
>
> Make this interface to return an error if memory removal fails. This way
> it is safe to call this function without panicking machine, and also
> makes it symmetric to add_memory() which already returns an error.
I was about to object because of the acpi hotremove but looking closer
acpi_memory_remove_memory and few others already do use __remove_memory
instead of remove_memory so this is good to go. I really hate how we had
to BUG in remove_memory as well so this is definitely a good change.
> Signed-off-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>
Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
> include/linux/memory_hotplug.h | 8 +++--
> mm/memory_hotplug.c | 64 +++++++++++++++++++++++-----------
> 2 files changed, 49 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index ae892eef8b82..988fde33cd7f 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -324,7 +324,7 @@ static inline void pgdat_resize_init(struct pglist_data *pgdat) {}
> extern bool is_mem_section_removable(unsigned long pfn, unsigned long nr_pages);
> extern void try_offline_node(int nid);
> extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
> -extern void remove_memory(int nid, u64 start, u64 size);
> +extern int remove_memory(int nid, u64 start, u64 size);
> extern void __remove_memory(int nid, u64 start, u64 size);
>
> #else
> @@ -341,7 +341,11 @@ static inline int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
> return -EINVAL;
> }
>
> -static inline void remove_memory(int nid, u64 start, u64 size) {}
> +static inline int remove_memory(int nid, u64 start, u64 size)
> +{
> + return -EBUSY;
> +}
> +
> static inline void __remove_memory(int nid, u64 start, u64 size) {}
> #endif /* CONFIG_MEMORY_HOTREMOVE */
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 328878b6799d..ace2cc614da4 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1735,9 +1735,10 @@ static int check_memblock_offlined_cb(struct memory_block *mem, void *arg)
> endpa = PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1))-1;
> pr_warn("removing memory fails, because memory [%pa-%pa] is onlined\n",
> &beginpa, &endpa);
> - }
>
> - return ret;
> + return -EBUSY;
> + }
> + return 0;
> }
>
> static int check_cpu_on_node(pg_data_t *pgdat)
> @@ -1820,19 +1821,9 @@ static void __release_memory_resource(resource_size_t start,
> }
> }
>
> -/**
> - * remove_memory
> - * @nid: the node ID
> - * @start: physical address of the region to remove
> - * @size: size of the region to remove
> - *
> - * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> - * and online/offline operations before this call, as required by
> - * try_offline_node().
> - */
> -void __ref __remove_memory(int nid, u64 start, u64 size)
> +static int __ref try_remove_memory(int nid, u64 start, u64 size)
> {
> - int ret;
> + int rc = 0;
>
> BUG_ON(check_hotplug_memory_range(start, size));
>
> @@ -1840,13 +1831,13 @@ void __ref __remove_memory(int nid, u64 start, u64 size)
>
> /*
> * All memory blocks must be offlined before removing memory. Check
> - * whether all memory blocks in question are offline and trigger a BUG()
> + * whether all memory blocks in question are offline and return error
> * if this is not the case.
> */
> - ret = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
> - check_memblock_offlined_cb);
> - if (ret)
> - BUG();
> + rc = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
> + check_memblock_offlined_cb);
> + if (rc)
> + goto done;
>
> /* remove memmap entry */
> firmware_map_remove(start, start + size, "System RAM");
> @@ -1858,14 +1849,45 @@ void __ref __remove_memory(int nid, u64 start, u64 size)
>
> try_offline_node(nid);
>
> +done:
> mem_hotplug_done();
> + return rc;
> }
>
> -void remove_memory(int nid, u64 start, u64 size)
> +/**
> + * remove_memory
> + * @nid: the node ID
> + * @start: physical address of the region to remove
> + * @size: size of the region to remove
> + *
> + * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> + * and online/offline operations before this call, as required by
> + * try_offline_node().
> + */
> +void __remove_memory(int nid, u64 start, u64 size)
> +{
> +
> + /*
> + * trigger BUG() is some memory is not offlined prior to calling this
> + * function
> + */
> + if (try_remove_memory(nid, start, size))
> + BUG();
> +}
> +
> +/*
> + * Remove memory if every memory block is offline, otherwise return -EBUSY is
> + * some memory is not offline
> + */
> +int remove_memory(int nid, u64 start, u64 size)
> {
> + int rc;
> +
> lock_device_hotplug();
> - __remove_memory(nid, start, size);
> + rc = try_remove_memory(nid, start, size);
> unlock_device_hotplug();
> +
> + return rc;
> }
> EXPORT_SYMBOL_GPL(remove_memory);
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> --
> 2.21.0
>
--
Michal Hocko
SUSE Labs
