On Wed 24-03-21 09:45:01, Oscar Salvador wrote:

On Tue, Mar 23, 2021 at 11:47:53AM +0100, Michal Hocko wrote:

quoted

On Fri 19-03-21 10:26:33, Oscar Salvador wrote:

quoted

Self stored memmap leads to a sparse memory situation which is unsuitable
for workloads that requires large contiguous memory chunks, so make this
an opt-in which needs to be explicitly enabled.

To control this, let memory_hotplug have its own memory space, as suggested
by David, so we can add memory_hotplug.memmap_on_memory parameter.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
Reviewed-by: David Hildenbrand <redacted>

Acked-by: Michal Hocko <mhocko@suse.com>

I would just rephrased the help text to be less low level

...

quoted

			When enabled, runtime hotplugged memory will
			allocate its internal metadata (struct pages)
			from the hotadded memory which will allow to
			hotadd a lot of memory without requiring
			additional memory to do so.
			This feature is disabled by default because it
			has some implication on large (e.g. GB)
			allocations in some configurations (e.g. small
			memory blocks).

Ok, this sounds good as well, and I guess it might suit best for what admin-guide
is about.

quoted

The memmap_on_memory can be dropped from the 1st patch IIUC and only
introduce it now.

It could be done, and I __think__ in some previous persion it was that way, but
I am leaning to not do it.
In the 1st patch, memmap_on_memory is false by default, so I see it as a preparatory
step for later (this patchset) till it might be enabled.

Moreover, the big comment from mhp_support_memmap_on_memory() should change to not
mention it, and change here again to reflect it.

All in all, I think it can stay, but maybe place a comment in the 1st patch above
the variable saying something like "This is a noop now, it will be enabled later on"

I will leave that up to you. This is likely not worth a larger
discussion but it seems quite pointless to add a variable which never
changes. The resulting code might look different than you expect because
compiler is allowed to simply drop the whole condition.
 

quoted

quoted

+
+/*
+ * memory_hotplug.memmap_on_memory parameter
+ */
+static bool memmap_on_memory __ro_after_init;
+#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY
+module_param(memmap_on_memory, bool, 0444);
+MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug");
+#endif

I am not very much familiar with the machinery. Does this expose the
state to the userspace?

Kind of:

# ls /sys/module/memory_hotplug/parameters
memmap_on_memory
# cat /sys/module/memory_hotplug/parameters/memmap_on_memory 
Y

But that is not really the state, but rather it shows whether the user
opted-in the feature by passing "memory_hotplug.memmap_on_memory=yes".
It might be that the user opted-in the feature, but it cannot be used at
at runtime (e.g: mhp_support_memmap_on_memory() return false due to size !=
memory_block_size())

Thanks for the clarification.
-- 
Michal Hocko
SUSE Labs