Re: [PATCH] mm: add build-time option to set hotplug default type

[Gregory Price <gourry@gourry.net>]

On Fri, Dec 20, 2024 at 05:04:42PM +0100, David Hildenbrand wrote:
> On 20.12.24 15:45, Gregory Price wrote:
> > +extern int memhp_default_type(void);
> > +extern void memhp_set_default_type(int online_type);
> 
> Please call these "default_online_type". Further keep the "mhp" terminology,
> it's more commonly used. We cannot rename the "memhp_default_state"
> parameter name unfortunately.

so
 mhp_default_online_type()
and
 mhp_set_default_online_type()

ack.

> > +config MEMHP_DEFAULT_TYPE
> > +       string
> > +       default "online" if MEMHP_DEFAULT_TYPE_NORMAL
> > +       default "online_movable" if MEMHP_DEFAULT_TYPE_MOVABLE
> > +       default "offline"
> > +
> 
> Could we get rid of CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE and simply have
> all three types as choices? normal/movable/offline?
>

Obviously doable, wasn't sure what the consensus was on changing or
removing build options, So I tried for the least disruptive.

summarizing:

- MEMORY_HOTPLUG_DEFAULT_ONLINE
+ MEMHP_DEFAULT_ONLINE_TYPE_OFFLINE
+ MEMHP_DEFAULT_ONLINE_TYPE_NORMAL
+ MEMHP_DEFAULT_ONLINE_TYPE_MOVABLE
+ MEMHP_DEFAULT_ONLINE_TYPE = (offline|online|movable)

> > +int memhp_default_type(void)
> > +{
> > +	int type;
> > +
> > +	if (mhp_default_online_type >= 0)
> > +		return mhp_default_online_type;
> > +
> > +	type = mhp_online_type_from_str(CONFIG_MEMHP_DEFAULT_TYPE);
> > +	if (type < 0)
> > +		type = MMOP_OFFLINE;
> 
> How could that ever happen?

It shouldn't unless someone does something silly like

MEMHP_DEFAULT_ONLINE_TYPE="i have no idea what i'm doing"

I just tend towards defensive programming.

> It's a bit weird that we are parsing strings
> when we can just decide that at compile-time using IS_ENABLED() etc?
> 

I tried to reuse the existing logic attached to the sysfs entry
controlling the same thing.

I wasn't sure how to deal with the fact that MEMHP_DEFAULT_ONLINE_TYPE
could be one of three values and did not think encoding 
   MMOP_OFFLINE/ONLINE/MOVABLE 
into the config was a good idea.

If you have another suggestion, I'm open


Will do the renames and rip out CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE and
submit a v2.

~Gregory