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

[David Hildenbrand <david@redhat.com>]

On 16.06.25 14:32, Oscar Salvador wrote:
> 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? :-)

Looks cleaner to me at least :)

-- 
Cheers,

David / dhildenb