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

[Oscar Salvador <osalvador@suse.de>]

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