Re: [PATCH v2 6/8] mm: hugetlb_vmemmap: improve hugetlb_vmemmap code readability

From: Mike Kravetz <mike.kravetz@oracle.com>
To: Muchun Song <songmuchun@bytedance.com>
Cc: david@redhat.com, akpm@linux-foundation.org, corbet@lwn.net,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org, duanxiongchun@bytedance.com
Subject: Re: [PATCH v2 6/8] mm: hugetlb_vmemmap: improve hugetlb_vmemmap code readability
Date: Tue, 28 Jun 2022 17:23:21 -0700	[thread overview]
Message-ID: <YrubeXg8tSxJeGxj@monkey> (raw)
In-Reply-To: <20220628092235.91270-7-songmuchun@bytedance.com>

On 06/28/22 17:22, Muchun Song wrote:
> There is a discussion about the name of hugetlb_vmemmap_alloc/free in
> thread [1].  The suggestion suggested by David is rename "alloc/free"
> to "optimize/restore" to make functionalities clearer to users,
> "optimize" means the function will optimize vmemmap pages, while
> "restore" means restoring its vmemmap pages discared before. This
> commit does this.
> 
> Another discussion is the confusion RESERVE_VMEMMAP_NR isn't used
> explicitly for vmemmap_addr but implicitly for vmemmap_end in
> hugetlb_vmemmap_alloc/free.  David suggested we can compute what
> hugetlb_vmemmap_init() does now at runtime.  We do not need to worry
> for the overhead of computing at runtime since the calculation is
> simple enough and those functions are not in a hot path.  This commit
> has the following improvements:
> 
>   1) The function suffixed name ("optimize/restore") is more expressive.
>   2) The logic becomes less weird in hugetlb_vmemmap_optimize/restore().
>   3) The hugetlb_vmemmap_init() does not need to be exported anymore.
>   4) A ->optimize_vmemmap_pages field in struct hstate is killed.
>   5) There is only one place where checks is_power_of_2(sizeof(struct
>      page)) instead of two places.
>   6) Add more comments for hugetlb_vmemmap_optimize/restore().
>   7) For external users, hugetlb_optimize_vmemmap_pages() is used for
>      detecting if the HugeTLB's vmemmap pages is optimizable originally.
>      In this commit, it is killed and we introduce a new helper
>      hugetlb_vmemmap_optimizable() to replace it.  The name is more
>      expressive.
> 
> Link: https://lore.kernel.org/all/20220404074652.68024-2-songmuchun@bytedance.com/ [1]
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  include/linux/hugetlb.h |   7 +--
>  include/linux/sysctl.h  |   4 ++
>  mm/hugetlb.c            |  15 ++---
>  mm/hugetlb_vmemmap.c    | 143 ++++++++++++++++++++----------------------------
>  mm/hugetlb_vmemmap.h    |  41 +++++++++-----
>  5 files changed, 102 insertions(+), 108 deletions(-)

Thanks!  I like the removal of hugetlb_vmemmap_init and printing directly
from report_hugepages.  Still need to look at your your command parsing
patches.

> @@ -3191,8 +3191,10 @@ static void __init report_hugepages(void)
>  		char buf[32];
>  
>  		string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
> -		pr_info("HugeTLB registered %s page size, pre-allocated %ld pages\n",
> +		pr_info("HugeTLB: registered %s page size, pre-allocated %ld pages\n",
>  			buf, h->free_huge_pages);
> +		pr_info("HugeTLB: %d KiB vmemmap can be freed for a %s page\n",
> +			hugetlb_vmemmap_optimizable_size(h) / SZ_1K, buf);
>  	}
>  }

My first thought was "Why report vmemmap freed pages if not enabled?".
However, since it can be enabled at runtime it is best always print.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz