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@xxxxxxxxxxxxx/ [1]
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
Mike Kravetz
