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

[David Hildenbrand <david@redhat.com>]

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