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
