On 4/29/22 05:18, Muchun Song wrote:
> When "hugetlb_free_vmemmap=on" and "memory_hotplug.memmap_on_memory"
> are both passed to boot cmdline, the variable of "memmap_on_memory"
> will be set to 1 even if the vmemmap pages will not be allocated from
> the hotadded memory since the former takes precedence over the latter.
I had to read that sentence a few times before understanding what it was
trying to say. Not insisting, but how about this instead:
Freeing HugeTLB vmemmap pages is not compatible with allocating memmap on
hot added memory. If "hugetlb_free_vmemmap=on" and
memory_hotplug.memmap_on_memory" are both passed on the kernel command line,
freeing hugetlb pages takes precedence. However, the global variable
memmap_on_memory will still be set to 1, even though we will not try to
allocate memmap on hot added memory.
Not sure if that is more clear or not.
> In the next patch, we want to enable or disable the feature of freeing
> vmemmap pages of HugeTLB via sysctl. We need a way to know if the
> feature of memory_hotplug.memmap_on_memory is enabled when enabling
> the feature of freeing vmemmap pages since those two features are not
> compatible, however, the variable of "memmap_on_memory" cannot indicate
> this nowadays. Do not set "memmap_on_memory" to 1 when both parameters
> are passed to cmdline, in this case, "memmap_on_memory" could indicate
> if this feature is enabled by the users.
>
> Also introduce mhp_memmap_on_memory() helper to move the definition of
> "memmap_on_memory" to the scope of CONFIG_MHP_MEMMAP_ON_MEMORY. In the
> next patch, mhp_memmap_on_memory() will also be exported to be used in
> hugetlb_vmemmap.c.
>
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> ---
> mm/memory_hotplug.c | 32 ++++++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
No issues with the changes,
Acked-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
--
Mike Kravetz
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 111684878fd9..a6101ae402f9 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -42,14 +42,36 @@
> #include "internal.h"
> #include "shuffle.h"
>
> +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY
> +static int memmap_on_memory_set(const char *val, const struct kernel_param *kp)
> +{
> + if (hugetlb_optimize_vmemmap_enabled())
> + return 0;
> + return param_set_bool(val, kp);
> +}
> +
> +static const struct kernel_param_ops memmap_on_memory_ops = {
> + .flags = KERNEL_PARAM_OPS_FL_NOARG,
> + .set = memmap_on_memory_set,
> + .get = param_get_bool,
> +};
>
> /*
> * 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_param_cb(memmap_on_memory, &memmap_on_memory_ops, &memmap_on_memory, 0444);
> MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug");
> +
> +static inline bool mhp_memmap_on_memory(void)
> +{
> + return memmap_on_memory;
> +}
> +#else
> +static inline bool mhp_memmap_on_memory(void)
> +{
> + return false;
> +}
> #endif
>
> enum {
> @@ -1263,9 +1285,7 @@ bool mhp_supports_memmap_on_memory(unsigned long size)
> * altmap as an alternative source of memory, and we do not exactly
> * populate a single PMD.
> */
> - return memmap_on_memory &&
> - !hugetlb_optimize_vmemmap_enabled() &&
> - IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
> + return mhp_memmap_on_memory() &&
> size == memory_block_size_bytes() &&
> IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
> IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));
> @@ -2083,7 +2103,7 @@ static int __ref try_remove_memory(u64 start, u64 size)
> * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in
> * the same granularity it was added - a single memory block.
> */
> - if (memmap_on_memory) {
> + if (mhp_memmap_on_memory()) {
> nr_vmemmap_pages = walk_memory_blocks(start, size, NULL,
> get_nr_vmemmap_pages_cb);
> if (nr_vmemmap_pages) {
