Re: [PATCH] mm: Convert memory block states (MEM_*) macros to enum

[David Hildenbrand <david@redhat.com>]

On 27.10.25 19:15, Omar Sandoval wrote:
> On Mon, Oct 27, 2025 at 10:29:15AM +0100, David Hildenbrand wrote:
>> On 26.10.25 17:22, Israel Batista wrote:
>>> The MEM_* constants indicating the state of the memory block are
>>> currently defined as macros, meaning their definitions will be omitted
>>> from the debuginfo on most kernel builds. This makes it harder for
>>> debuggers to correctly map the block state at runtime, which can be
>>> quite useful when analysing errors related to memory hot plugging and
>>> unplugging with tools such as drgn and eBPF.
>>>
>>> Converting the constants to an enum will ensure the correct information
>>> is emitted by the compiler and available for the debugger, without needing
>>> to hard-code them into the debugger and track their changes.
>>>
>>> Signed-off-by: Israel Batista <linux@israelbatista.dev.br>
>>> ---
>>>    include/linux/memory.h | 16 +++++++++-------
>>>    1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>>> index ba1515160894..8feba3bfcd18 100644
>>> --- a/include/linux/memory.h
>>> +++ b/include/linux/memory.h
>>> @@ -89,13 +89,15 @@ int arch_get_memory_phys_device(unsigned long start_pfn);
>>>    unsigned long memory_block_size_bytes(void);
>>>    int set_memory_block_size_order(unsigned int order);
>>> -/* These states are exposed to userspace as text strings in sysfs */
>>> -#define	MEM_ONLINE		(1<<0) /* exposed to userspace */
>>> -#define	MEM_GOING_OFFLINE	(1<<1) /* exposed to userspace */
>>> -#define	MEM_OFFLINE		(1<<2) /* exposed to userspace */
>>> -#define	MEM_GOING_ONLINE	(1<<3)
>>> -#define	MEM_CANCEL_ONLINE	(1<<4)
>>> -#define	MEM_CANCEL_OFFLINE	(1<<5)
>>> +enum mem_states {
>>> +	/* These states are exposed to userspace as text strings in sysfs */
>>> +	MEM_ONLINE		 = (1<<0), /* exposed to userspace */
>>> +	MEM_GOING_OFFLINE	 = (1<<1), /* exposed to userspace */
>>> +	MEM_OFFLINE		 = (1<<2), /* exposed to userspace */
>>> +	MEM_GOING_ONLINE	 = (1<<3),
>>> +	MEM_CANCEL_ONLINE	 = (1<<4),
>>> +	MEM_CANCEL_OFFLINE	 = (1<<5),
>>> +};
>>>    struct memory_notify {
>>>    	unsigned long start_pfn;
>>
>> CCing Lorenzo, we recently had a discussion about such conversions.
> 
> Yeah, I've been asking people to send out these conversions as we
> encounter them in drgn, but ONLY when the absence of a value in the
> kernel debugging symbols causes actual problems for drgn. I want it to
> be clear that we're not spamming these just to cause churn. This is an
> unfortunate corner case of debug info that leaves us with no other
> option.

Well, I think we use the opportunity to clean all this further up, so 
all good :)

> 
>> The states are mutually exclusive (so no flags), so I wonder if we can just
>> drop the (1<< X) setting completely.
> 
> FWIW, putting my C standard committee hat on, there is nothing wrong
> with combining flags in an enum. 

Right.

> C11 is silent on the matter, but C23
> made this explicit. Quoting 6.7.3.3, paragraph 16: "After possible
> lvalue conversion a value of the enumerated type behaves the same as the
> value with the underlying type, in particular with all aspects of
> promotion, conversion, and arithmetic." Lorenzo may have been thinking
> of the stricter rules in C++.

Right, it was around semantics.

For example, instead of defining the flags as an enum, define the actual 
bits in an enum.

> 
> Of course, semantically, it makes more sense to use distinct values in
> cases like this where the values are not actually flags.

Right. And that's the case here, we just made it look like flags for no 
apparent reason.

So I hope we can clean this up in the same go (same patch or addon patch).

> 
>> IIRC, these values are not exposed to
>> user space, only the corresponding names are, see state_show().
>>
>>
>> Won't the compiler now complain that e.g., kcore_callback() does snot handle
>> all cases? (no default statement)
> 
> Only if the controlling expression of the switch statement actually has
> the enum type. All existing code uses unsigned long, so the compiler
> doesn't care.

Ah, indeed, inherited from the notifier_call implementation. Which 
brings us to another topic: likely we should see where we can then 
actually used this named enum.

struct memory_block -> state looks like *the* candidate ;)

-- 
Cheers

David / dhildenb