Re: [PATCH v9 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on

[Mike Kravetz]

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) {