Re: [PATCH v4 2/3] mm,memory_hotplug: Implement numa node notifier

[Oscar Salvador <osalvador@suse.de>]

On Wed, Jun 04, 2025 at 02:03:23PM +0200, David Hildenbrand wrote:
> > diff --git a/include/linux/memory.h b/include/linux/memory.h
> > index 5ec4e6d209b9..8c5c88eaffb3 100644
> > --- a/include/linux/memory.h
> > +++ b/include/linux/memory.h
> > @@ -99,6 +99,14 @@ int set_memory_block_size_order(unsigned int order);
> >   #define	MEM_PREPARE_ONLINE	(1<<6)
> >   #define	MEM_FINISH_OFFLINE	(1<<7)
> > +/* These states are used for numa node notifiers */
> > +#define NODE_BECOMING_MEM_AWARE		(1<<0)
> > +#define NODE_BECAME_MEM_AWARE		(1<<1)
> > +#define NODE_BECOMING_MEMORYLESS	(1<<2)
> > +#define NODE_BECAME_MEMORYLESS		(1<<3)
> > +#define NODE_CANCEL_MEM_AWARE		(1<<4)
> > +#define NODE_CANCEL_MEMORYLESS		(1<<5)
> 
> Very nitpicky: MEM vs. MEMORY inconsistency. Also, I am not sure about
> "MEMORYLESS vs. MEMORY AWARE" terminology (opposite of aware is not less)
> and "BECOMING" vs. "CANCEL" ...

Heh, that is why I'm not in the marketing field :-)

> There must be something better ... but what is it. :)
> 
> NODE_ADDING_FIRST_MEMORY
> NODE_ADDED_FIRST_MEMORY
> NODE_CANCEL_ADDING_FIRST_MEMORY
> 
> NODE_REMOVING_LAST_MEMORY
> NODE_REMOVED_LAST_MEMORY
> NODE_CANCEL_REMOVING_LAST_MEMORY
> 
> Maybe something like that? I still don't quite like the "CANCEL" stuff.
> 
> NODE_ADDING_FIRST_MEMORY
> NODE_ADDED_FIRST_MEMORY
> NODE_NOT_ADDED_FIRST_MEMORY
> 
> NODE_REMOVING_LAST_MEMORY
> NODE_REMOVED_LAST_MEMORY
> NODE_NOT_REMOVED_LAST_MEMORY

If I were to pick one, I'd go with NODE_ADDING_FIRST_MEMORY/NODE_REMOVING_LAST_MEMORY.
I think those make it easier to grasp.


> Hm ...
> 
> > +
> >   struct memory_notify {
> >   	/*
> >   	 * The altmap_start_pfn and altmap_nr_pages fields are designated for
> > @@ -109,7 +117,10 @@ struct memory_notify {
> >   	unsigned long altmap_nr_pages;
> >   	unsigned long start_pfn;
> >   	unsigned long nr_pages;
> > -	int status_change_nid_normal;
> > +	int status_change_nid;
> > +};
> 
> Could/should that be a separate patch after patch #1 removed the last user?
> 
> Also, I think the sequence should be (this patch is getting hard to review
> for me due to the size):
> 
> #1 existing patch 1
> #2 remove status_change_nid_normal
> #3 introduce node notifier
> #4-#X: convert individual users to node notifier
> #X+1: change status_change_nid to always just indicate the nid, renaming
>       it on the way (incl current patch #3)

When you say #4-#X, you mean a separate patch per converting user?
So, one for memtier, one for cxl, one for hmat, etc.?


> > +
> > +struct node_notify {
> >   	int status_change_nid;
> 
> This should be called "nid" right from the start.

Copy that.

> > @@ -157,15 +168,34 @@ static inline unsigned long memory_block_advised_max_size(void)
> >   {
> >   	return 0;
> >   }
> > +
> 
> [...]
> 
> >   	 * {on,off}lining is constrained to full memory sections (or more
> > @@ -1194,11 +1172,22 @@ 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);
> > -	arg.start_pfn = pfn;
> > -	arg.nr_pages = nr_pages;
> > -	node_states_check_changes_online(nr_pages, zone, &arg);
> > +	node_arg.status_change_nid = NUMA_NO_NODE;
> > +	if (!node_state(nid, N_MEMORY)) {
> > +		/* Node is becoming memory aware. Notify consumers */
> > +		cancel_node_notifier_on_err = true;
> > +		node_arg.status_change_nid = nid;
> > +		ret = node_notify(NODE_BECOMING_MEM_AWARE, &node_arg);
> > +		ret = notifier_to_errno(ret);
> > +		if (ret)
> > +			goto failed_addition;
> > +	}
> 
> I assume without NUMA, that code would never trigger? I mean, the whole
> notifier doesn't make sense without CONFIG_NUMA :)

So, glad you asked because it forced me to look again.
And I think we might have some divergences there, so I will sort it out
in the next respin.

Thanks for the feedback ;-)


-- 
Oscar Salvador
SUSE Labs