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;
Date:   Wed, 1 Jun 2022 18:48:13 +0800
From:   Muchun Song <songmuchun@bytedance.com>
To:     David Hildenbrand <david@redhat.com>, mike.kravetz@oracle.com
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, smuchun@bytedance.com
Subject: Re: [PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related
 functions
Message-ID: <YpdD7WCYlfCkvv2L@FVFYT0MHHV2J.googleapis.com>
References: <20220404074652.68024-1-songmuchun@bytedance.com>
 <20220404074652.68024-2-songmuchun@bytedance.com>
 <087817e3-98ce-09f6-9ae9-68e544f43775@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <087817e3-98ce-09f6-9ae9-68e544f43775@redhat.com>
Precedence: bulk

On Wed, Jun 01, 2022 at 11:28:44AM +0200, David Hildenbrand wrote:
> 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().
>

I think it is a good suggestion.
 
> 
> 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?
> 

Right. A little overhead, but hugetlb_vmemmap_alloc() is not hot path,
maybe we can accept the increased overhead of calculating at runtime.

Hi Mike,

Do you have any objections on this? If no, I think we can do this in a
separate patch.

> 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.
>

How about the name of "hugetlb_vmemmap_optimizable()"?  "should" seems to
tell the user that this hugetlb should be optimized, however, optimization
also depends on "hugetlb_free_vmemmap=on".  "optimizable" seems to be more
appropriate, right?

Thanks.