Re: [PATCH v2] mm/hugetlb: fix set_max_huge_pages() when there are surplus pages

[Oscar Salvador <osalvador@suse.de>]

On Mon, Apr 07, 2025 at 03:23:08PM +0800, Jinjiang Tu wrote:
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 39f92aad7bd1..4afcd5ce417c 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3826,6 +3826,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
>                               nodemask_t *nodes_allowed)
>  {
>         unsigned long min_count;
> +       unsigned long persistent_free_count;
>         unsigned long allocated;
>         struct folio *folio;
>         LIST_HEAD(page_list);
> @@ -3960,7 +3961,14 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
>          * and won't grow the pool anywhere else. Not until one of the
>          * sysctls are changed, or the surplus pages go out of use.
>          */
> -       min_count = h->resv_huge_pages + h->nr_huge_pages - h->free_huge_pages;
> +       persistent_free_count = h->free_huge_pages;
> +       if (h->free_huge_pages > persistent_huge_pages(h)) {
> +               if (h->free_huge_pages > h->surplus_huge_pages)
> +                       persistent_free_count -= h->surplus_huge_pages;
> +               else
> +                       persistent_free_count = 0;
> +       }
> +       min_count = h->resv_huge_pages + persistent_huge_pages(h) - persistent_free_count;
>         min_count = max(count, min_count);
>         try_to_free_low(h, min_count, nodes_allowed);

I think this is ok, but this needs a big fat comment explaining why we
are doing what we are doing, because this us too subtle to grasp
without any context.

Like why it is ok not to subtract anything from persistent_huge_pages,
and the circumstances that might lead to this like that all free pages
are surplus because of hvo failing to restore and so we do not want
to account twice the same thing.

Actually, while you are at it, it would be great to rename variables and
give them a meaningful name (e.g: min_count and count).
That can be done in a follow-up patch of course.

The reason I am suggesting this is because it is not that easy to
untangle what is going on here, and have more meaningful names might
help.

Thanks


-- 
Oscar Salvador
SUSE Labs