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

[David Hildenbrand <david@redhat.com>]

On 04.06.25 14:38, Oscar Salvador wrote:
> 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.

Just to clarify, these were the 3 notifiers each that belong together. I 
was not sure about NODE_CANCEL_ADDING_FIRST_MEMORY vs. 
NODE_NOT_ADDED_FIRST_MEMORY.

> 
> 
>> 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.?

Yes.

-- 
Cheers,

David / dhildenb