Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <087817e3-98ce-09f6-9ae9-68e544f43775@redhat.com>
Date:   Wed, 1 Jun 2022 11:28:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Content-Language: en-US
To:     Muchun Song <songmuchun@bytedance.com>, mike.kravetz@oracle.com,
        akpm@linux-foundation.org
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        smuchun@bytedance.com
References: <20220404074652.68024-1-songmuchun@bytedance.com>
 <20220404074652.68024-2-songmuchun@bytedance.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related
 functions
In-Reply-To: <20220404074652.68024-2-songmuchun@bytedance.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 04.04.22 09:46, Muchun Song wrote:
> The word of "free" is not expressive enough to express the feature of optimizing
> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> And some function names are prefixed with "huge_page" instead of "hugetlb", it is
> easily to be confused with THP.  In this patch , cheanup related functions to make
> code more clear and expressive.

No strong opinion (I remember I kicked of the discussion), but I was
wondering if instead of alloc vs. free we could be using something like
optimize vs. restore/rollback.

E.g., hugetlb_vmemmap_optimize() vs. hugetlb_vmemmap_restore().


Maybe there are other suggestions?

> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  include/linux/hugetlb.h |  2 +-
>  mm/hugetlb.c            | 10 +++++-----
>  mm/hugetlb_vmemmap.c    | 42 ++++++++++++++++++++----------------------
>  mm/hugetlb_vmemmap.h    | 20 ++++++++++----------
>  4 files changed, 36 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 53c1b6082a4c..c16fbb1228a3 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -618,7 +618,7 @@ struct hstate {
>  	unsigned int free_huge_pages_node[MAX_NUMNODES];
>  	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
>  #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> -	unsigned int nr_free_vmemmap_pages;
> +	unsigned int optimize_vmemmap_pages;

I suggest converting that into a bool and just calling it

"bool optimize_vmemmap_pages".

You can easily compute what hugetlb_vmemmap_init() at runtime from the
page and RESERVE_VMEMMAP_NR, right?

At least the calculation in alloc_huge_page_vmemmap() and
free_huge_page_vmemmap() become *less* weird for me if the magic value
RESERVE_VMEMMAP_NR isn't used explicitly for vmemmap_addr but implicitly
for vmemmap_end.

>  #endif
>  #ifdef CONFIG_CGROUP_HUGETLB
>  	/* cgroup control files */
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index dd642cfc538b..1f9fbdddc86b 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1540,7 +1540,7 @@ static void __update_and_free_page(struct hstate *h, struct page *page)
>  	if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
>  		return;
>  
> -	if (alloc_huge_page_vmemmap(h, page)) {
> +	if (hugetlb_vmemmap_alloc(h, page)) {
>  		spin_lock_irq(&hugetlb_lock);
>  		/*
>  		 * If we cannot allocate vmemmap pages, just refuse to free the
> @@ -1617,7 +1617,7 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
>  
>  static inline void flush_free_hpage_work(struct hstate *h)
>  {
> -	if (free_vmemmap_pages_per_hpage(h))
> +	if (hugetlb_optimize_vmemmap_pages(h))

It might be reasonable to call that hugetlb_should_optimize_vmemmap()
then, letting it return a bool.


-- 
Thanks,

David / dhildenb