Re: [PATCH v2] mm/hugetlb: fix surplus pages in dissolve_free_huge_page()

[Oscar Salvador <osalvador@suse.de>]

On Tue, Mar 04, 2025 at 09:21:06PM +0800, Jinjiang Tu wrote:
> In dissolve_free_huge_page(), free huge pages are dissolved without
> adjusting surplus count. However, free huge pages may be accounted as
> surplus pages, and will lead to wrong surplus count.
> 
> I reproduce this issue on qemu. The steps are:
> 1) Node1 is memory-less at first. Hot-add memory to node1 by executing
> the two commands in qemu monitor:
>   object_add memory-backend-ram,id=mem1,size=1G
>   device_add pc-dimm,id=dimm1,memdev=mem1,node=1
> 2) online one memory block of Node1 with：
>   echo online_movable > /sys/devices/system/node/node1/memoryX/state
> 3) create 64 huge pages for node1
> 4) run a program to reserve (don't consume) all the huge pages
> 5) echo 0 > nr_huge_pages for node1. After this step, free huge pages in
> Node1 are surplus.
> 6) create 80 huge pages for node0
> 7) offline memory of node1, The memory range to offline contains the free
> surplus huge pages created in step3) ~ step5)
>   echo offline > /sys/devices/system/node/node1/memoryX/state
> 8) kill the program in step 4)
> 
> The result:
>            Node0     Node1
> total       80        0
> free        80        0
> surplus     0         61
> 
> To fix it, adjust surplus when destroying huge pages if the node has
> surplus pages in dissolve_free_hugetlb_folio().
> 
> The result with this patch:
>            Node0     Node1
> total       80        0
> free        80        0
> surplus     0         0
> 
> Fixes: c8721bbbdd36 ("mm: memory-hotplug: enable memory hotplug to handle hugepage")
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>

Acked-by: Oscar Salvador <osalvador@suse.de>

> @@ -2157,7 +2159,9 @@ int dissolve_free_hugetlb_folio(struct folio *folio)
>  			goto retry;
>  		}
>  
> -		remove_hugetlb_folio(h, folio, false);
> +		if (h->surplus_huge_pages_node[folio_nid(folio)])
> +			adjust_surplus = true;
> +		remove_hugetlb_folio(h, folio, adjust_surplus);
>  		h->max_huge_pages--;
>  		spin_unlock_irq(&hugetlb_lock);
>  
> @@ -2177,7 +2181,7 @@ int dissolve_free_hugetlb_folio(struct folio *folio)
>  			rc = hugetlb_vmemmap_restore_folio(h, folio);
>  			if (rc) {
>  				spin_lock_irq(&hugetlb_lock);
> -				add_hugetlb_folio(h, folio, false);
> +				add_hugetlb_folio(h, folio, adjust_surplus);

I was about to point this out, but checking v1 I saw that David already
that.
My alternative would have been to just get rid of the adjust_surplus
boolean and to the checking right within the lock cycle e.g:

 if (h->surplus_huge_pages_node[folio_nid(folio)])
        add_hugetlb_folio(h, folio, true);
 else
        add_hugetlb_folio(h, folio, false);

But I guess that's fine as you already explained.

 

-- 
Oscar Salvador
SUSE Labs