Re: [PATCH v3 4/7] mm/hotplug: Allow pageblock alignment via altmap reservation

[David Hildenbrand <david@redhat.com>]

On 12.07.23 15:50, Aneesh Kumar K.V wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 12.07.23 05:16, Aneesh Kumar K V wrote:
>>> On 7/11/23 10:49 PM, David Hildenbrand wrote:
>>>> On 11.07.23 06:48, Aneesh Kumar K.V wrote:
>>>>> Add a new kconfig option that can be selected if we want to allow
>>>>> pageblock alignment by reserving pages in the vmemmap altmap area.
>>>>> This implies we will be reserving some pages for every memoryblock
>>>>> This also allows the memmap on memory feature to be widely useful
>>>>> with different memory block size values.
>>>>
>>>> "reserving pages" is a nice way of saying "wasting memory". :) Let's spell that out.
>>>>
>>>> I think we have to find a better name for this, and I think we should have a toggle similar to memory_hotplug.memmap_on_memory. This should be an admin decision, not some kernel config option.
>>>>
>>>>
>>>> memory_hotplug.force_memmap_on_memory
>>>>
>>>> "Enable the memmap on memory feature even if it could result in memory waste due to memmap size limitations. For example, if the memmap for a memory block requires 1 MiB, but the pageblock size is 2 MiB, 1 MiB
>>>> of hotplugged memory will be wasted. Note that there are still cases where the feature cannot be enforced: for example, if the memmap is smaller than a single page, or if the architecture does not support the forced mode in all configurations."
>>>>
>>>> Thoughts?
>>>>
>>>
>>> With module parameter, do we still need the Kconfig option?
>>
>> No.
>>
>> Sleeping over this, maybe we can convert the existing
>> memory_hotplug.memmap_on_memory parameter to also accept "force".
>>
> 
> How about this?
> 
> modified   mm/memory_hotplug.c
> @@ -45,13 +45,67 @@
>   /*
>    * memory_hotplug.memmap_on_memory parameter
>    */
> -static bool memmap_on_memory __ro_after_init;
> -module_param(memmap_on_memory, bool, 0444);
> -MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug");
> +enum {
> +	MEMMAP_ON_MEMORY_DISABLE = 0,
> +	MEMMAP_ON_MEMORY_ENABLE,
> +	FORCE_MEMMAP_ON_MEMORY,

MEMMAP_ON_MEMORY_FORCE ?

> +};
> +static int memmap_mode __read_mostly = MEMMAP_ON_MEMORY_DISABLE;
> +static const char *memmap_on_memory_to_str[] = {
> +	[MEMMAP_ON_MEMORY_DISABLE]  = "disable",
> +	[MEMMAP_ON_MEMORY_ENABLE]   = "enable",
> +	[FORCE_MEMMAP_ON_MEMORY]    = "force",
> +};
> +
> +static inline unsigned long memory_block_align_base(unsigned long size)
> +{
> +	if (memmap_mode == FORCE_MEMMAP_ON_MEMORY) {
> +		unsigned long align;
> +		unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT;
> +		unsigned long vmemmap_size;
> +
> +		vmemmap_size = DIV_ROUND_UP(nr_vmemmap_pages * sizeof(struct page), PAGE_SIZE);
> +		align = pageblock_align(vmemmap_size) - vmemmap_size;
> +		return align;
> +	} else
> +		return 0;

^ have to see that in action :)

> +}
> +
> +static int set_memmap_mode(const char *val, const struct kernel_param *kp)
> +{
> +	int ret = sysfs_match_string(memmap_on_memory_to_str, val);
> +

That would break existing cmdlines that eat Y/N/0/..., no?

Maybe try parsing "force/FORCE" first and then fallback to the common 
bool parsing (kstrtobool).

Same when printing: handle "force" separately and then just print Y/N 
like param_get_bool() used to do.

So you'd end up with Y/N/FORCE as output and Y/N/0/.../FORCE/force as input.

But I'm happy to hear about alternatives. Maybe a second parameter is 
better ... but what name should it have "memmap_on_memory_force" sounds 
wrong. We'd need a name that expresses that we might be wasting memory, 
hm ...

> +	if (ret < 0)
> +		return ret;
> +	*((int *)kp->arg) = ret;
> +	if (ret == FORCE_MEMMAP_ON_MEMORY) {
> +		pr_info("Memory hotplug will reserve %ld pages in each memory block\n",
> +			memory_block_align_base(memory_block_size_bytes()));
> +	}
> +	return 0;
> +}
> +
> +static int get_memmap_mode(char *buffer, const struct kernel_param *kp)
> +{
> +	return sprintf(buffer, "%s\n", memmap_on_memory_to_str[*((int *)kp->arg)]);
> +}
> +
> +static const struct kernel_param_ops memmap_mode_ops = {
> +	.set = set_memmap_mode,
> +	.get = get_memmap_mode,
> +};
> +module_param_cb(memmap_on_memory, &memmap_mode_ops, &memmap_mode, 0644);
> +MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug\n"
> +	"With value \"force\" it could result in memory waste due to memmap size limitations \n"
> +	"For example, if the memmap for a memory block requires 1 MiB, but the pageblock \n"
> +	"size is 2 MiB, 1 MiB of hotplugged memory will be wasted. Note that there are \n"
> +	"still cases where the feature cannot be enforced: for example, if the memmap is \n"
> +	"smaller than a single page, or if the architecture does not support the forced \n"
> +	"mode in all configurations. (disable/enable/force)");
>   
>   static inline bool mhp_memmap_on_memory(void)
>   {
> -	return memmap_on_memory;
> +	return !!memmap_mode;
>   }
>   #else
> 
> We can also enable runtime enable/disable/force the feature. We just
> need to make sure on try_remove_memory we lookup for altmap correctly.
> 

Yes, that's already been asked for. But let's do that as a separate 
change later.

-- 
Cheers,

David / dhildenb