Re: [PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[Marc Herbert <marc.herbert@linux.intel.com>]

On 2025-08-18 07:08, Dave Jiang wrote:
> 
> 
> On 8/16/25 12:29 AM, David Hildenbrand wrote:
>> On 14.08.25 19:16, Dave Jiang wrote:
>>> Add clarification to comment for memory hotplug callback ordering as the
>>> current comment does not provide clear language on which callback happens
>>> first.
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>   include/linux/memory.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>>> index 40eb70ccb09d..02314723e5bd 100644
>>> --- a/include/linux/memory.h
>>> +++ b/include/linux/memory.h
>>> @@ -116,7 +116,7 @@ struct mem_section;
>>>     /*
>>>    * Priorities for the hotplug memory callback routines (stored in decreasing
>>> - * order in the callback chain)
>>> + * order in the callback chain). The callback ordering happens from high to low.
>>>    */
>>>   #define DEFAULT_CALLBACK_PRI    0
>>>   #define SLAB_CALLBACK_PRI    1
>>
>> "stored in decreasing order in the callback chain"
>>
>> is pretty clear? It's a chain after all that gets called.
> 
> I can drop the patch. For some reason when I read it I'm thinking the opposite, and when Marc was also confused I started questioning things.
> 

I think we both found the current comment confusing (even together!)
because:

- It very briefly alludes to an implementation detail (the chain)
  without really getting into detail. A "chain" could be bi-directional;
  why not? This one is... "most likely" not. Doubt.

- Higher priorities can have lower numbers, example: "P1 bugs". Not the
  case here, but this "double standards" situation makes _all_
  priorities suspicious and confusing.

- Constants that come first in the file are called last.


I would go further than Dave and also drop the "chain" implementation
detail because it makes the reader to think too much.  Not needed and
distracting at this particular point in the file.

/*
 * Priorities for the hotplug memory callback routines.
 * Invoked from high to low.
 */

  => Hopefully zero cognitive load.