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

On Mon, Oct 27, 2025 at 11:15:35AM -0700, 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 <[email protected]>
> > > ---
> > >   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 {

Why are we naming a type we never use...

> > > +	/* 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),
> > > +};

If it has to be named, can we just expose the bits as an enum and the values as
BIT(...)?

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

Right. That really sucks, but I like drgn so if reasonable I do want us to
make life easier there... :)

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

I don't really understand the argument being made there.

That's just saying the enum behaves as if it's the underlying type? I'm not
arguing otherwise.

Consider:

enum some_name {
	X,
	Y
};

void some_func(enum some_name val)
{
	switch (val) {
	case X:
		...
	case Y:
		...
	}

	// compiler doesn't warn about missing cases.
}

This is already giving some sense as to the intuition that enums specify
all the values as declared specific enumeration values.

But intuitively, with the enum as a _named_ type, it's _weird_ for there to
be possible values for it that are not listed.

The problem here for me is the type being _named_.

If it's unnamed, then it doesn't really matter, it's just another way of
declaring the values.

If drgn needs it named, then just name bit values and use BIT(...).

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

enum's are kinda defined by being distinct values...

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

So why are we naming the type... does drgn require it?

Thanks, Lorenzo