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

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

[Gregory Price]

When memory hotplug auto-online is enabled, hotplug memory blocks are
onlined into ZONE_NORMAL by default. The `memhp_default_state` boot
param allows runtime configuration, but no build-time config exists.

Add a build-time configuration option to change default hotplug zone.

build config:
  MEMHP_DEFAULT_TYPE

Selections:
  MEMHP_DEFAULT_TYPE_NORMAL  => mhp_default_online_type = "online"
  MEMHP_DEFAULT_TYPE_MOVABLE => mhp_default_online_type = "online_movable"

When MEMORY_HOTPLUG_DEFAULT_ONLINE is disabled, MEMHP_DEFAULT_TYPE is
set to "offline" to match the current system behavior.

ZONE_NORMAL still remains the default, because for systems with a large
amount of hotplug memory, defaulting it to ZONE_MOVABLE may result in
portions failing to online if sufficient ZONE_NORMAL memory does not
exist to describe it.

Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/base/memory.c          |  4 ++--
 include/linux/memory_hotplug.h |  5 +++--
 mm/Kconfig                     | 33 +++++++++++++++++++++++++++++++++
 mm/memory_hotplug.c            | 29 ++++++++++++++++++++++-------
 4 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 67858eeb92ed..6f69b01abe51 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -512,7 +512,7 @@ static ssize_t auto_online_blocks_show(struct device *dev,
 				       struct device_attribute *attr, char *buf)
 {
 	return sysfs_emit(buf, "%s\n",
-			  online_type_to_str[mhp_default_online_type]);
+			  online_type_to_str[memhp_default_type()]);
 }
 
 static ssize_t auto_online_blocks_store(struct device *dev,
@@ -524,7 +524,7 @@ static ssize_t auto_online_blocks_store(struct device *dev,
 	if (online_type < 0)
 		return -EINVAL;
 
-	mhp_default_online_type = online_type;
+	memhp_set_default_type(online_type);
 	return count;
 }
 
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index b27ddce5d324..a43470f0d93c 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -144,8 +144,6 @@ extern u64 max_mem_size;
 
 extern int mhp_online_type_from_str(const char *str);
 
-/* Default online_type (MMOP_*) when new memory blocks are added. */
-extern int mhp_default_online_type;
 /* If movable_node boot option specified */
 extern bool movable_node_enabled;
 static inline bool movable_node_is_enabled(void)
@@ -303,6 +301,9 @@ static inline void __remove_memory(u64 start, u64 size) {}
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 #ifdef CONFIG_MEMORY_HOTPLUG
+/* Default online_type (MMOP_*) when new memory blocks are added. */
+extern int memhp_default_type(void);
+extern void memhp_set_default_type(int online_type);
 extern void __ref free_area_init_core_hotplug(struct pglist_data *pgdat);
 extern int __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags);
 extern int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags);
diff --git a/mm/Kconfig b/mm/Kconfig
index 7949ab121070..b6e63e5306b1 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -565,6 +565,39 @@ config MEMORY_HOTPLUG_DEFAULT_ONLINE
 	  Say N here if you want the default policy to keep all hot-plugged
 	  memory blocks in 'offline' state.
 
+choice
+	prompt "Memory Hotplug Default Type"
+	depends on MEMORY_HOTPLUG_DEFAULT_ONLINE
+	default MEMHP_DEFAULT_TYPE_NORMAL
+	help
+	  Default memory zone for driver managed hotplug memory.
+	  Select normal to generally allow kernel usage of this memory.
+	  Select movable to generally disallow kernel usage of this memory.
+	  Example typical kernel usage would be page structs and page tables.
+
+
+config MEMHP_DEFAULT_TYPE_NORMAL
+	bool "Normal"
+	help
+	  Online driver managed hotplug memory into zone normal.
+	  Select this if you want the kernel to be able to utilize
+	  this memory for kernel data (e.g. page tables).
+
+config MEMHP_DEFAULT_TYPE_MOVABLE
+	bool "Movable"
+	help
+	  Online driver managed hotplug memory into zone movable.
+	  Select this if you do not want the kernel to be able to
+	  utilize this memory for kernel data (e.g. page tables).
+
+endchoice
+
+config MEMHP_DEFAULT_TYPE
+       string
+       default "online" if MEMHP_DEFAULT_TYPE_NORMAL
+       default "online_movable" if MEMHP_DEFAULT_TYPE_MOVABLE
+       default "offline"
+
 config MEMORY_HOTREMOVE
 	bool "Allow for memory hot remove"
 	select HAVE_BOOTMEM_INFO_NODE if (X86_64 || PPC64)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 3b6f93962481..1e4340fa9f07 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -219,11 +219,26 @@ void put_online_mems(void)
 
 bool movable_node_enabled = false;
 
-#ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
-int mhp_default_online_type = MMOP_OFFLINE;
-#else
-int mhp_default_online_type = MMOP_ONLINE;
-#endif
+static int mhp_default_online_type = -1;
+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;
+
+	mhp_default_online_type = type;
+	return mhp_default_online_type;
+}
+
+void memhp_set_default_type(int online_type)
+{
+	mhp_default_online_type = online_type;
+}
 
 static int __init setup_memhp_default_state(char *str)
 {
@@ -1328,7 +1343,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
 
 static int online_memory_block(struct memory_block *mem, void *arg)
 {
-	mem->online_type = mhp_default_online_type;
+	mem->online_type = memhp_default_type();
 	return device_online(&mem->dev);
 }
 
@@ -1575,7 +1590,7 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
 		merge_system_ram_resource(res);
 
 	/* online pages if requested */
-	if (mhp_default_online_type != MMOP_OFFLINE)
+	if (memhp_default_type() != MMOP_OFFLINE)
 		walk_memory_blocks(start, size, NULL, online_memory_block);
 
 	return ret;
-- 
2.47.1

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

[David Hildenbrand]

On 20.12.24 15:45, Gregory Price wrote:
> When memory hotplug auto-online is enabled, hotplug memory blocks are
> onlined into ZONE_NORMAL by default. The `memhp_default_state` boot
> param allows runtime configuration, but no build-time config exists.

+ you can configure it at runtime.

> 
> Add a build-time configuration option to change default hotplug zone.
> 
> build config:
>    MEMHP_DEFAULT_TYPE
> 
> Selections:
>    MEMHP_DEFAULT_TYPE_NORMAL  => mhp_default_online_type = "online"
>    MEMHP_DEFAULT_TYPE_MOVABLE => mhp_default_online_type = "online_movable"
> 
> When MEMORY_HOTPLUG_DEFAULT_ONLINE is disabled, MEMHP_DEFAULT_TYPE is
> set to "offline" to match the current system behavior.
> 
> ZONE_NORMAL still remains the default, because for systems with a large
> amount of hotplug memory, defaulting it to ZONE_MOVABLE may result in
> portions failing to online if sufficient ZONE_NORMAL memory does not
> exist to describe it.
> 

What's the use case?

I'm hoping that we can move away from the compile-time option and let 
user space, who better knows what to do (especially with different kinds 
of memory having different requirements) configure auto-onlining or 
online manually (e.g., devdax).

For example, in RHEL we traditionally use udev rules, because we want a 
different behavior on bare-metal vs. VMs, but they are not particularly 
easy to extend to implement wilder policies.

For a while I worked on a systemd unit [1] to configure+handle memory 
onlining so we can get rid of the udev rules we use in RHEL. But it only 
configured+handled having "one type of hotplugged memort".

I'm planning on picking that up again at some point, to also make it 
possible to handle different policies for different memory types.

For example, maybe someone wants to auto-online virtio-mem memory to 
ZONE_NORMAL, but let onlining of devdax memory be handled by the devdax 
utility (e.g. ZONE_MOVABLE). We can identify in some cases "what" memory 
was added using /proc/iomem.

[1] https://github.com/davidhildenbrand/systemd/tree/memoryhotplugd

-- 
Cheers,

David / dhildenb

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

[Gregory Price]

On Fri, Dec 20, 2024 at 03:59:58PM +0100, David Hildenbrand wrote:
> On 20.12.24 15:45, Gregory Price wrote:
> > When memory hotplug auto-online is enabled, hotplug memory blocks are
> > onlined into ZONE_NORMAL by default. The `memhp_default_state` boot
> > param allows runtime configuration, but no build-time config exists.
> 
> + you can configure it at runtime.
> 
> > 
> > Add a build-time configuration option to change default hotplug zone.
> > 
> > build config:
> >    MEMHP_DEFAULT_TYPE
> > 
> > Selections:
> >    MEMHP_DEFAULT_TYPE_NORMAL  => mhp_default_online_type = "online"
> >    MEMHP_DEFAULT_TYPE_MOVABLE => mhp_default_online_type = "online_movable"
> > 
> > When MEMORY_HOTPLUG_DEFAULT_ONLINE is disabled, MEMHP_DEFAULT_TYPE is
> > set to "offline" to match the current system behavior.
> > 
> > ZONE_NORMAL still remains the default, because for systems with a large
> > amount of hotplug memory, defaulting it to ZONE_MOVABLE may result in
> > portions failing to online if sufficient ZONE_NORMAL memory does not
> > exist to describe it.
> > 
> 
> What's the use case?
> 
> I'm hoping that we can move away from the compile-time option and let user
> space, who better knows what to do (especially with different kinds of
> memory having different requirements) configure auto-onlining or online
> manually (e.g., devdax).
> 

At Meta we have a fairly complex boot process that goes through multiple
kernels before we get to a target kernel to run workloads. Each of those
kernels may have health-checks that want to see the memory is online.

The build switch makes this particular feature consistent for us across
all those kernels without having to carry the boot parameter. Eventually
we'd like to move to udev, but it's not feasible for us right now due
to the state of CXL BIOS/Platform/Drivers - driver-management does not
work reliable for all platforms and all devices.

This gets us where we're going while the rest catch up.

> For example, in RHEL we traditionally use udev rules, because we want a
> different behavior on bare-metal vs. VMs, but they are not particularly easy
> to extend to implement wilder policies.
> 
> For a while I worked on a systemd unit [1] to configure+handle memory
> onlining so we can get rid of the udev rules we use in RHEL. But it only
> configured+handled having "one type of hotplugged memort".
> 
> I'm planning on picking that up again at some point, to also make it
> possible to handle different policies for different memory types.
> 
> For example, maybe someone wants to auto-online virtio-mem memory to
> ZONE_NORMAL, but let onlining of devdax memory be handled by the devdax
> utility (e.g. ZONE_MOVABLE). We can identify in some cases "what" memory was
> added using /proc/iomem.
> 

Generally I agree this is the way to go. But in those cases - you're
probably not turning MEMORY_HOTPLUG_DEFAULT_ONLINE on anyway. So this
doesn't really affect that usage pattern.

~Gregory

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

[David Hildenbrand]

On 20.12.24 16:17, Gregory Price wrote:
> On Fri, Dec 20, 2024 at 03:59:58PM +0100, David Hildenbrand wrote:
>> On 20.12.24 15:45, Gregory Price wrote:
>>> When memory hotplug auto-online is enabled, hotplug memory blocks are
>>> onlined into ZONE_NORMAL by default. The `memhp_default_state` boot
>>> param allows runtime configuration, but no build-time config exists.
>>
>> + you can configure it at runtime.
>>
>>>
>>> Add a build-time configuration option to change default hotplug zone.
>>>
>>> build config:
>>>     MEMHP_DEFAULT_TYPE
>>>
>>> Selections:
>>>     MEMHP_DEFAULT_TYPE_NORMAL  => mhp_default_online_type = "online"
>>>     MEMHP_DEFAULT_TYPE_MOVABLE => mhp_default_online_type = "online_movable"
>>>
>>> When MEMORY_HOTPLUG_DEFAULT_ONLINE is disabled, MEMHP_DEFAULT_TYPE is
>>> set to "offline" to match the current system behavior.
>>>
>>> ZONE_NORMAL still remains the default, because for systems with a large
>>> amount of hotplug memory, defaulting it to ZONE_MOVABLE may result in
>>> portions failing to online if sufficient ZONE_NORMAL memory does not
>>> exist to describe it.
>>>
>>
>> What's the use case?
>>
>> I'm hoping that we can move away from the compile-time option and let user
>> space, who better knows what to do (especially with different kinds of
>> memory having different requirements) configure auto-onlining or online
>> manually (e.g., devdax).
>>
> 
> At Meta we have a fairly complex boot process that goes through multiple
> kernels before we get to a target kernel to run workloads. Each of those> kernels may have health-checks that want to see the memory is online.

Fancy, I'd love to learn some more about that if there is public 
information available somewhere.

> 
> The build switch makes this particular feature consistent for us across
> all those kernels without having to carry the boot parameter. Eventually
> we'd like to move to udev, but it's not feasible for us right now due
> to the state of CXL BIOS/Platform/Drivers - driver-management does not
> work reliable for all platforms and all devices.

I see.

> 
> This gets us where we're going while the rest catch up.

Understood, let me review the patch briefly.

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

On 20.12.24 15:45, Gregory Price wrote:
> When memory hotplug auto-online is enabled, hotplug memory blocks are
> onlined into ZONE_NORMAL by default. The `memhp_default_state` boot
> param allows runtime configuration, but no build-time config exists.
> 
> Add a build-time configuration option to change default hotplug zone.
> 
> build config:
>    MEMHP_DEFAULT_TYPE
> 
> Selections:
>    MEMHP_DEFAULT_TYPE_NORMAL  => mhp_default_online_type = "online"
>    MEMHP_DEFAULT_TYPE_MOVABLE => mhp_default_online_type = "online_movable"
> 
> When MEMORY_HOTPLUG_DEFAULT_ONLINE is disabled, MEMHP_DEFAULT_TYPE is
> set to "offline" to match the current system behavior.
> 
> ZONE_NORMAL still remains the default, because for systems with a large
> amount of hotplug memory, defaulting it to ZONE_MOVABLE may result in
> portions failing to online if sufficient ZONE_NORMAL memory does not
> exist to describe it.
> 
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>   drivers/base/memory.c          |  4 ++--
>   include/linux/memory_hotplug.h |  5 +++--
>   mm/Kconfig                     | 33 +++++++++++++++++++++++++++++++++
>   mm/memory_hotplug.c            | 29 ++++++++++++++++++++++-------
>   4 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 67858eeb92ed..6f69b01abe51 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -512,7 +512,7 @@ static ssize_t auto_online_blocks_show(struct device *dev,
>   				       struct device_attribute *attr, char *buf)
>   {
>   	return sysfs_emit(buf, "%s\n",
> -			  online_type_to_str[mhp_default_online_type]);
> +			  online_type_to_str[memhp_default_type()]);
>   }
>   
>   static ssize_t auto_online_blocks_store(struct device *dev,
> @@ -524,7 +524,7 @@ static ssize_t auto_online_blocks_store(struct device *dev,
>   	if (online_type < 0)
>   		return -EINVAL;
>   
> -	mhp_default_online_type = online_type;
> +	memhp_set_default_type(online_type);
>   	return count;
>   }
>   
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index b27ddce5d324..a43470f0d93c 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -144,8 +144,6 @@ extern u64 max_mem_size;
>   
>   extern int mhp_online_type_from_str(const char *str);
>   
> -/* Default online_type (MMOP_*) when new memory blocks are added. */
> -extern int mhp_default_online_type;
>   /* If movable_node boot option specified */
>   extern bool movable_node_enabled;
>   static inline bool movable_node_is_enabled(void)
> @@ -303,6 +301,9 @@ static inline void __remove_memory(u64 start, u64 size) {}
>   #endif /* CONFIG_MEMORY_HOTREMOVE */
>   
>   #ifdef CONFIG_MEMORY_HOTPLUG
> +/* Default online_type (MMOP_*) when new memory blocks are added. */
> +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.

>   extern void __ref free_area_init_core_hotplug(struct pglist_data *pgdat);
>   extern int __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags);
>   extern int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags);
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 7949ab121070..b6e63e5306b1 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -565,6 +565,39 @@ config MEMORY_HOTPLUG_DEFAULT_ONLINE
>   	  Say N here if you want the default policy to keep all hot-plugged
>   	  memory blocks in 'offline' state.
>   
> +choice
> +	prompt "Memory Hotplug Default Type"
> +	depends on MEMORY_HOTPLUG_DEFAULT_ONLINE
> +	default MEMHP_DEFAULT_TYPE_NORMAL
> +	help
> +	  Default memory zone for driver managed hotplug memory.
> +	  Select normal to generally allow kernel usage of this memory.
> +	  Select movable to generally disallow kernel usage of this memory.
> +	  Example typical kernel usage would be page structs and page tables.
> +
> +
> +config MEMHP_DEFAULT_TYPE_NORMAL
> +	bool "Normal"
> +	help
> +	  Online driver managed hotplug memory into zone normal.
> +	  Select this if you want the kernel to be able to utilize
> +	  this memory for kernel data (e.g. page tables).
> +
> +config MEMHP_DEFAULT_TYPE_MOVABLE
> +	bool "Movable"
> +	help
> +	  Online driver managed hotplug memory into zone movable.
> +	  Select this if you do not want the kernel to be able to
> +	  utilize this memory for kernel data (e.g. page tables).
> +
> +endchoice
> +
> +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?

>   config MEMORY_HOTREMOVE
>   	bool "Allow for memory hot remove"
>   	select HAVE_BOOTMEM_INFO_NODE if (X86_64 || PPC64)
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 3b6f93962481..1e4340fa9f07 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -219,11 +219,26 @@ void put_online_mems(void)
>   
>   bool movable_node_enabled = false;
>   
> -#ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
> -int mhp_default_online_type = MMOP_OFFLINE;
> -#else
> -int mhp_default_online_type = MMOP_ONLINE;
> -#endif
> +static int mhp_default_online_type = -1;
> +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's a bit weird that we are parsing strings 
when we can just decide that at compile-time using IS_ENABLED() etc?


Apart from that LGTM.

-- 
Cheers,

David / dhildenb

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

[Gregory Price]

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

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

[David Hildenbrand]

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

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

[Gregory Price]

On Fri, Dec 20, 2024 at 07:25:57PM +0100, David Hildenbrand wrote:
> > 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.
>

onward to V3 :P 

~Gregory

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

[Gregory Price]

On Fri, Dec 20, 2024 at 05:04:42PM +0100, David Hildenbrand wrote:
> On 20.12.24 15:45, Gregory Price wrote:
> 
> 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.
> 

fyi for v2 - i went ahead and cleaned this up (not including the param),
but basically also synced the build option with the boot parameter:
 - offline
 - online  ("Auto")
 - online_kernel
 - online_movable

Figured might as well.  Sending shortly

~Gregory

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

[David Hildenbrand]

On 20.12.24 19:25, 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:
>>
>> 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.
>>
> 
> fyi for v2 - i went ahead and cleaned this up (not including the param),
> but basically also synced the build option with the boot parameter:
>   - offline
>   - online  ("Auto")
>   - online_kernel
>   - online_movable
> 
> Figured might as well.  Sending shortly

Just replied to your other mail :)

-- 
Cheers,

David / dhildenb