Re: [PATCH v6 03/10] mm,memory_hotplug: Implement numa node notifier

Oscar Salvador
2025-06-16T12:33:01Z

On Mon, Jun 16, 2025 at 02:21:02PM +0200, David Hildenbrand wrote:
> Exactly. I recall I checked some of them in the past as well, when I
> stumbled over this behavior.

Now, about simplying the cancel_{mem,node}_notifier_on_err.
It would look like this:

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index d6df85452c72..ff887f10b114 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1150,11 +1150,16 @@ void mhp_deinit_memmap_on_memory(unsigned long pfn, unsigned long nr_pages)
int online_pages(unsigned long pfn, unsigned long nr_pages,
struct zone *zone, struct memory_group *group)
{
- bool cancel_mem_notifier_on_err = false, cancel_node_notifier_on_err = false;
const int nid = zone_to_nid(zone);
int need_zonelists_rebuild = 0;
- struct memory_notify mem_arg;
- struct node_notify node_arg;
+ struct memory_notify mem_arg = {
+ .start_pfn = pfn,
+ .nr_pages = nr_pages,
+ .status_change_nid = NUMA_NO_NODE,
+ };
+ struct node_notify node_arg = {
+ .nid = NUMA_NO_NODE,
+ };
unsigned long flags;
int ret;

@@ -1173,21 +1178,16 @@ int online_pages(unsigned long pfn, unsigned long nr_pages,
/* associate pfn range with the zone */
move_pfn_range_to_zone(zone, pfn, nr_pages, NULL, MIGRATE_ISOLATE);

- node_arg.nid = NUMA_NO_NODE;
if (!node_state(nid, N_MEMORY)) {
/* Adding memory to the node for the first time */
- cancel_node_notifier_on_err = true;
node_arg.nid = nid;
+ mem_arg.status_change_nid = nid;
ret = node_notify(NODE_ADDING_FIRST_MEMORY, &node_arg);
ret = notifier_to_errno(ret);
if (ret)
goto failed_addition;
}

- mem_arg.start_pfn = pfn;
- mem_arg.nr_pages = nr_pages;
- mem_arg.status_change_nid = node_arg.nid;
- cancel_mem_notifier_on_err = true;
ret = memory_notify(MEM_GOING_ONLINE, &mem_arg);
ret = notifier_to_errno(ret);
if (ret)
@@ -1249,9 +1249,8 @@ int online_pages(unsigned long pfn, unsigned long nr_pages,
pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
(unsigned long long) pfn << PAGE_SHIFT,
(((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
- if (cancel_mem_notifier_on_err)
- memory_notify(MEM_CANCEL_ONLINE, &mem_arg);
- if (cancel_node_notifier_on_err)
+ memory_notify(MEM_CANCEL_ONLINE, &mem_arg);
+ if (node_arg.nid != NUMA_NO_NODE)
node_notify(NODE_CANCEL_ADDING_FIRST_MEMORY, &node_arg);
remove_pfn_range_from_zone(zone, pfn, nr_pages);
return ret;
@@ -1899,13 +1898,18 @@ static int count_system_ram_pages_cb(unsigned long start_pfn,
int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
struct zone *zone, struct memory_group *group)
{
- bool cancel_mem_notifier_on_err = false, cancel_node_notifier_on_err = false;
unsigned long pfn, managed_pages, system_ram_pages = 0;
const unsigned long end_pfn = start_pfn + nr_pages;
struct pglist_data *pgdat = zone->zone_pgdat;
const int node = zone_to_nid(zone);
- struct memory_notify mem_arg;
- struct node_notify node_arg;
+ struct memory_notify mem_arg = {
+ .start_pfn = pfn,
+ .nr_pages = nr_pages,
+ .status_change_nid = NUMA_NO_NODE,
+ };
+ struct node_notify node_arg = {
+ .nid = NUMA_NO_NODE,
+ };
unsigned long flags;
char *reason;
int ret;
@@ -1970,20 +1974,15 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
* 'nr_pages' more. If so, we know that the node will become empty, and
* so we will clear N_MEMORY for it.
*/
- node_arg.nid = NUMA_NO_NODE;
if (nr_pages >= pgdat->node_present_pages) {
node_arg.nid = node;
- cancel_node_notifier_on_err = true;
+ mem_arg.status_change_nid = node;
ret = node_notify(NODE_REMOVING_LAST_MEMORY, &node_arg);
ret = notifier_to_errno(ret);
if (ret)
goto failed_removal_isolated;
}

- mem_arg.start_pfn = start_pfn;
- mem_arg.nr_pages = nr_pages;
- mem_arg.status_change_nid = node_arg.nid;
- cancel_mem_notifier_on_err = true;
ret = memory_notify(MEM_GOING_OFFLINE, &mem_arg);
ret = notifier_to_errno(ret);
if (ret) {
@@ -2087,9 +2086,8 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
failed_removal_isolated:
/* pushback to free area */
undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
- if (cancel_mem_notifier_on_err)
- memory_notify(MEM_CANCEL_OFFLINE, &mem_arg);
- if (cancel_node_notifier_on_err)
+ memory_notify(MEM_CANCEL_OFFLINE, &mem_arg);
+ if (node_arg.nid != NUMA_NO_NODE)
node_notify(NODE_CANCEL_REMOVING_LAST_MEMORY, &node_arg);
failed_removal_pcplists_disabled:
lru_cache_enable();

Not sure if I like keeping the cancel_* stuff.
Strong opinion here? Feelings? :-)

--
Oscar Salvador
SUSE Labs