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