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

On Tue, Oct 28, 2025 at 04:33:39PM +0000, Lorenzo Stoakes wrote:
> On Mon, Oct 27, 2025 at 04:34:29PM -0700, Omar Sandoval wrote:
> > On Mon, Oct 27, 2025 at 07:46:43PM +0000, Lorenzo Stoakes wrote:
> > > 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...
> >
> > As David suggested, naming it means that we can then use it in a way
> > that enables compiler warnings and documents the code better.
> 
> Which compiler warnings?

I was referring to a situation that David suggested where:

1. enum memory_block_state contains distinct values, not flags, and
2. We convert struct memory_block->state to enum memory_block_state.

Then, everywhere that does switch (memory_block->state) would get
warnings for forgotten cases. Maybe desirable, maybe not, but those are
the warnings I was referring to.

> enum foo {
> 	X = 1UL << 40,
> };
> 
> static void blah(enum foo foo)
> {
> }
> 
> ...
> blah(3);
> 
> Doesn't generate any.
> 
> Nor does W=1 or W=2.
> 
> Nor with clang, etiher.
> 
> The better solution is to do something like __bitwise with sparse.
> 
> Also re: type, I'm not sure it is all that safe, or you certainly lose
> control somewhat as the size of the integer you're passing around is
> 'whatever fits the values'. Technically it should be restricted to a signed
> integer (e.g. 32 bit) for C11 but I think gnu c11 is using some compiler
> extension stuff to just make it adapt it to the correct sizing.
> 
> Anyway, this is all probably moot as I believe David suggested these
> _aren't used as flags_.
> 
> In which case I'm fine with it.
> 
> I'm just confused as to why we're still debating the flag stuff?

Right, it's irrelevant in this case. I mainly wanted to know for the
future what MM's preference was for cases that really do need flags. My
bad for not making that clear. It sounds like the answer is a __bitwise
typedef with bit numbers in an anonymous enum, which is fine with me.

[snipping the rest as my question has been answered]

Thanks,
Omar