On 20.12.24 17:37, Gregory Price wrote:
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.
Yes.
+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.
I don't know of any rules what we can rename/add/remove regarding config
options. Distros have to pay attention, but that's why Kconfig asks you
what to do.
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)
s/movable/online_movable/
Maybe call them
MEMHP_DEFAULT_ONLINE_TYPE_ONLINE
MEMHP_DEFAULT_ONLINE_TYPE_ONLINE_MOVABLE
MEMHP_DEFAULT_ONLINE_TYPE_OFFLINE
if possible, to directly correspond to the actual names
+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
I'd just do it like CONFIG_TRANSPARENT_HUGEPAGE_ALWAYS, and simply do
if (IS_ENABLED(MEMHP_DEFAULT_ONLINE_TYPE_ONLINE))
...
else if (IS_ENABLED(MEMHP_DEFAULT_ONLINE_TYPE_ONLINE_MOVABLE))
...
No need for parsing strings and dealing with unexpected values.
--
Cheers,
David / dhildenb
