On 12.07.23 15:50, Aneesh Kumar K.V wrote:
David Hildenbrand <da...@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
