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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9a845c21-5cfb-4535-97bd-0b02f5852457@redhat.com>
Date: Wed, 4 Jun 2025 14:47:28 +0200
From: David Hildenbrand <david@...hat.com>
To: Oscar Salvador <osalvador@...e.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 Vlastimil Babka <vbabka@...e.cz>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Harry Yoo <harry.yoo@...cle.com>, Rakie Kim <rakie.kim@...com>,
 Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] mm,memory_hotplug: Implement numa node notifier

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