Re: [PATCH v5 6/7] mm/hotplug: Embed vmem_altmap details in memory block

[David Hildenbrand <david@redhat.com>]

On 25.07.23 12:02, Aneesh Kumar K.V wrote:
> With memmap on memory, some architecture needs more details w.r.t altmap
> such as base_pfn, end_pfn, etc to unmap vmemmap memory. Instead of
> computing them again when we remove a memory block, embed vmem_altmap
> details in struct memory_block if we are using memmap on memory block
> feature.
> 
> No functional change in this patch
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---

[...]

>   
>   static int add_memory_block(unsigned long block_id, unsigned long state,
> -			    unsigned long nr_vmemmap_pages,
> +			    struct vmem_altmap *altmap,
>   			    struct memory_group *group)
>   {
>   	struct memory_block *mem;
> @@ -744,7 +751,14 @@ static int add_memory_block(unsigned long block_id, unsigned long state,
>   	mem->start_section_nr = block_id * sections_per_block;
>   	mem->state = state;
>   	mem->nid = NUMA_NO_NODE;
> -	mem->nr_vmemmap_pages = nr_vmemmap_pages;
> +	if (altmap) {
> +		mem->altmap = kmalloc(sizeof(struct vmem_altmap), GFP_KERNEL);
> +		if (!mem->altmap) {
> +			kfree(mem);
> +			return -ENOMEM;
> +		}
> +		memcpy(mem->altmap, altmap, sizeof(*altmap));
> +	}

I'm wondering if we should instead let the caller do the alloc/free. So we would alloc
int the caller and would only store the pointer.

Before removing the memory block, we would clear the pointer and free it in the caller.

IOW, when removing a memory block and we still have an altmap set, something would be wrong.

See below on try_remove_memory() handling.

[...]

> -static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
> +static int get_vmemmap_altmap_cb(struct memory_block *mem, void *arg)
>   {
> +	struct vmem_altmap *altmap = (struct vmem_altmap *)arg;
>   	/*
> -	 * If not set, continue with the next block.
> +	 * If we have any pages allocated from altmap
> +	 * return the altmap details and break callback.
>   	 */
> -	return mem->nr_vmemmap_pages;
> +	if (mem->altmap) {
> +		memcpy(altmap, mem->altmap, sizeof(struct vmem_altmap));
> +		return 1;
> +	}
> +	return 0;
>   }
>   
>   static int check_cpu_on_node(int nid)
> @@ -2146,9 +2152,8 @@ EXPORT_SYMBOL(try_offline_node);
>   
>   static int __ref try_remove_memory(u64 start, u64 size)
>   {
> -	struct vmem_altmap mhp_altmap = {};
> -	struct vmem_altmap *altmap = NULL;
> -	unsigned long nr_vmemmap_pages;
> +	int ret;
> +	struct vmem_altmap mhp_altmap, *altmap = NULL;
>   	int rc = 0, nid = NUMA_NO_NODE;
>   
>   	BUG_ON(check_hotplug_memory_range(start, size));
> @@ -2171,24 +2176,15 @@ static int __ref try_remove_memory(u64 start, u64 size)
>   	 * the same granularity it was added - a single memory block.
>   	 */
>   	if (mhp_memmap_on_memory()) {
> -		nr_vmemmap_pages = walk_memory_blocks(start, size, NULL,
> -						      get_nr_vmemmap_pages_cb);
> -		if (nr_vmemmap_pages) {
> +		ret = walk_memory_blocks(start, size, &mhp_altmap,
> +					 get_vmemmap_altmap_cb);
> +		if (ret) {
>   			if (size != memory_block_size_bytes()) {
>   				pr_warn("Refuse to remove %#llx - %#llx,"
>   					"wrong granularity\n",
>   					start, start + size);
>   				return -EINVAL;
>   			}
> -
> -			/*
> -			 * Let remove_pmd_table->free_hugepage_table do the
> -			 * right thing if we used vmem_altmap when hot-adding
> -			 * the range.
> -			 */
> -			mhp_altmap.base_pfn = PHYS_PFN(start);
> -			mhp_altmap.free = nr_vmemmap_pages;
> -			mhp_altmap.alloc = nr_vmemmap_pages;
>   			altmap = &mhp_altmap;
>   		}


Instead of that, I suggest (whitespace damage expected):

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 3f231cf1b410..f6860df64549 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1956,12 +1956,19 @@ static int check_memblock_offlined_cb(struct memory_block *mem, void *arg)
         return 0;
  }
  
-static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
+static int test_has_altmap_cb(struct memory_block *mem, void *arg)
  {
-       /*
-        * If not set, continue with the next block.
-        */
-       return mem->nr_vmemmap_pages;
+       struct memory_block **mem_ptr = (struct memory_block **)arg;
+
+       if (mem->altmap) {
+               /*
+                * We're not taking a reference on the memory block; it
+                * it cannot vanish while we're about to that memory ourselves.
+                */
+               *mem_ptr = mem;
+               return 1;
+       }
+       return 0;
  }
  
  static int check_cpu_on_node(int nid)
@@ -2036,9 +2043,7 @@ EXPORT_SYMBOL(try_offline_node);
  
  static int __ref try_remove_memory(u64 start, u64 size)
  {
-       struct vmem_altmap mhp_altmap = {};
         struct vmem_altmap *altmap = NULL;
-       unsigned long nr_vmemmap_pages;
         int rc = 0, nid = NUMA_NO_NODE;
  
         BUG_ON(check_hotplug_memory_range(start, size));
@@ -2061,9 +2066,9 @@ static int __ref try_remove_memory(u64 start, u64 size)
          * the same granularity it was added - a single memory block.
          */
         if (mhp_memmap_on_memory()) {
-               nr_vmemmap_pages = walk_memory_blocks(start, size, NULL,
-                                                     get_nr_vmemmap_pages_cb);
-               if (nr_vmemmap_pages) {
+               struct memory_block *mem;
+
+               if (walk_memory_blocks(start, size, &mem, test_has_altmap_cb)) {
                         if (size != memory_block_size_bytes()) {
                                 pr_warn("Refuse to remove %#llx - %#llx,"
                                         "wrong granularity\n",
@@ -2072,12 +2077,11 @@ static int __ref try_remove_memory(u64 start, u64 size)
                         }
  
                         /*
-                        * Let remove_pmd_table->free_hugepage_table do the
-                        * right thing if we used vmem_altmap when hot-adding
-                        * the range.
+                        * Clear the altmap from the memory block before we
+                        * remove it; we'll take care of freeing the altmap.
                          */
-                       mhp_altmap.alloc = nr_vmemmap_pages;
-                       altmap = &mhp_altmap;
+                       altmap = mem->altmap;
+                       mem->altmap = NULL;
                 }
         }
  
@@ -2094,6 +2098,9 @@ static int __ref try_remove_memory(u64 start, u64 size)
  
         arch_remove_memory(start, size, altmap);
  
+       if (altmap)
+               kfree(altmap);
+
         if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK)) {
                 memblock_phys_free(start, size);
                 memblock_remove(start, size);


-- 
Cheers,

David / dhildenb