On Mon, 2023-10-02 at 11:28 +0200, David Hildenbrand wrote:
>
> > +
> > +static int __ref try_remove_memory(u64 start, u64 size)
> > +{
> > + int rc, nid = NUMA_NO_NODE;
> > +
> > + BUG_ON(check_hotplug_memory_range(start, size));
> > +
> > + /*
> > + * All memory blocks must be offlined before removing memory. Check
> > + * whether all memory blocks in question are offline and return error
> > + * if this is not the case.
> > + *
> > + * While at it, determine the nid. Note that if we'd have mixed nodes,
> > + * we'd only try to offline the last determined one -- which is good
> > + * enough for the cases we care about.
> > + */
> > + rc = walk_memory_blocks(start, size, &nid, check_memblock_offlined_cb);
> > + if (rc)
> > + return rc;
> > +
> > + /*
> > + * For memmap_on_memory, the altmaps could have been added on
> > + * a per-memblock basis. Loop through the entire range if so,
> > + * and remove each memblock and its altmap.
> > + */
> > + if (mhp_memmap_on_memory()) {
> > + unsigned long memblock_size = memory_block_size_bytes();
> > + u64 cur_start;
> > +
> > + for (cur_start = start; cur_start < start + size;
> > + cur_start += memblock_size)
> > + __try_remove_memory(nid, cur_start, memblock_size);
> > + } else {
> > + __try_remove_memory(nid, start, size);
> > + }
> > +
> > return 0;
> > }
>
> Why is the firmware, memblock and nid handling not kept in this outer
> function?
>
> We really shouldn't be doing per memory block what needs to be done per
> memblock: remove_memory_block_devices() and arch_remove_memory().
Ah yes makes sense since we only do create_memory_block_devices() and
arch_add_memory() in the per memory block inner loop during addition.
How should the locking work in this case though?
The original code holds the mem_hotplug_begin() lock over
arch_remove_memory() and all of the nid and memblock stuff. Should I
just hold the lock and release it in the inner loop for each memory
block, and then also acquire and release it separately for the memblock
and nid stuff in the outer function?
Here's the incremental diff for what I'm thinking:
---
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 43dbd71a4910..13380178173d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -2171,7 +2171,7 @@ void try_offline_node(int nid)
}
EXPORT_SYMBOL(try_offline_node);
-static void __ref __try_remove_memory(int nid, u64 start, u64 size)
+static void __ref remove_memory_block_and_altmap(int nid, u64 start, u64 size)
{
int rc = 0;
struct memory_block *mem;
@@ -2187,9 +2187,6 @@ static void __ref __try_remove_memory(int nid, u64 start, u64 size)
mem->altmap = NULL;
}
- /* remove memmap entry */
- firmware_map_remove(start, start + size, "System RAM");
-
/*
* Memory block device removal under the device_hotplug_lock is
* a barrier against racing online attempts.
@@ -2206,16 +2203,6 @@ static void __ref __try_remove_memory(int nid, u64 start, u64 size)
kfree(altmap);
}
- if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK)) {
- memblock_phys_free(start, size);
- memblock_remove(start, size);
- }
-
- release_mem_region_adjustable(start, size);
-
- if (nid != NUMA_NO_NODE)
- try_offline_node(nid);
-
mem_hotplug_done();
}
@@ -2249,11 +2236,29 @@ static int __ref try_remove_memory(u64 start, u64 size)
for (cur_start = start; cur_start < start + size;
cur_start += memblock_size)
- __try_remove_memory(nid, cur_start, memblock_size);
+ remove_memory_block_and_altmap(nid, cur_start,
+ memblock_size);
} else {
- __try_remove_memory(nid, start, size);
+ remove_memory_block_and_altmap(nid, start, size);
}
+ /* remove memmap entry */
+ firmware_map_remove(start, start + size, "System RAM");
+
+ mem_hotplug_begin();
+
+ if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK)) {
+ memblock_phys_free(start, size);
+ memblock_remove(start, size);
+ }
+
+ release_mem_region_adjustable(start, size);
+
+ if (nid != NUMA_NO_NODE)
+ try_offline_node(nid);
+
+ mem_hotplug_done();
+
return 0;
}
