| Message ID | 20220404074652.68024-2-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cleanup hugetlb_vmemmap | expand |
On 4/4/22 00: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. Some typos in the commit message. Might be better as: The keyword "free" is not expressive enough to describe the feature of optimizing vmemmap pages associated with each HugeTLB page. Rename the keyword to "optimize". Some function names are prefixed with "huge_page" instead of "hugetlb". These can easily be confused with THP. Change function names to make code more clear and expressive. > 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(-) Chnages look fine, Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
On 5/31/22 16:47, Mike Kravetz wrote: > On 4/4/22 00: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. > > Some typos in the commit message. Might be better as: > > The keyword "free" is not expressive enough to describe the feature of optimizing vmemmap pages associated with each HugeTLB page. Rename the > keyword to "optimize". > > Some function names are prefixed with "huge_page" instead of "hugetlb". These can easily be confused with THP. Change function names to make > code more clear and expressive. > Sorry for the long lines :(
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.
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.
>>> #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? No strong opinion. Either is clearer to me compared to what we have right now :)
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; #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)) flush_work(&free_hpage_work); } @@ -1739,7 +1739,7 @@ static void __prep_account_new_huge_page(struct hstate *h, int nid) static void __prep_new_huge_page(struct hstate *h, struct page *page) { - free_huge_page_vmemmap(h, page); + hugetlb_vmemmap_free(h, page); INIT_LIST_HEAD(&page->lru); set_compound_page_dtor(page, HUGETLB_PAGE_DTOR); hugetlb_set_page_subpool(page, NULL); @@ -2112,7 +2112,7 @@ int dissolve_free_huge_page(struct page *page) * Attempt to allocate vmemmmap here so that we can take * appropriate action on failure. */ - rc = alloc_huge_page_vmemmap(h, head); + rc = hugetlb_vmemmap_alloc(h, head); if (!rc) { /* * Move PageHWPoison flag from head page to the raw @@ -3427,7 +3427,7 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) remove_hugetlb_page_for_demote(h, page, false); spin_unlock_irq(&hugetlb_lock); - rc = alloc_huge_page_vmemmap(h, page); + rc = hugetlb_vmemmap_alloc(h, page); if (rc) { /* Allocation of vmemmmap failed, we can not demote page */ spin_lock_irq(&hugetlb_lock); diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index 791626983c2e..91b79b9d9e25 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Free some vmemmap pages of HugeTLB + * Optimize vmemmap pages associated with HugeTLB * * Copyright (c) 2020, Bytedance. All rights reserved. * @@ -192,7 +192,7 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON, hugetlb_free_vmemmap_enabled_key); EXPORT_SYMBOL(hugetlb_free_vmemmap_enabled_key); -static int __init early_hugetlb_free_vmemmap_param(char *buf) +static int __init hugetlb_vmemmap_early_param(char *buf) { /* We cannot optimize if a "struct page" crosses page boundaries. */ if (!is_power_of_2(sizeof(struct page))) { @@ -212,29 +212,26 @@ static int __init early_hugetlb_free_vmemmap_param(char *buf) return 0; } -early_param("hugetlb_free_vmemmap", early_hugetlb_free_vmemmap_param); - -static inline unsigned long free_vmemmap_pages_size_per_hpage(struct hstate *h) -{ - return (unsigned long)free_vmemmap_pages_per_hpage(h) << PAGE_SHIFT; -} +early_param("hugetlb_free_vmemmap", hugetlb_vmemmap_early_param); /* * Previously discarded vmemmap pages will be allocated and remapping * after this function returns zero. */ -int alloc_huge_page_vmemmap(struct hstate *h, struct page *head) +int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head) { int ret; unsigned long vmemmap_addr = (unsigned long)head; - unsigned long vmemmap_end, vmemmap_reuse; + unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages; if (!HPageVmemmapOptimized(head)) return 0; - vmemmap_addr += RESERVE_VMEMMAP_SIZE; - vmemmap_end = vmemmap_addr + free_vmemmap_pages_size_per_hpage(h); - vmemmap_reuse = vmemmap_addr - PAGE_SIZE; + vmemmap_addr += RESERVE_VMEMMAP_SIZE; + vmemmap_pages = hugetlb_optimize_vmemmap_pages(h); + vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT); + vmemmap_reuse = vmemmap_addr - PAGE_SIZE; + /* * The pages which the vmemmap virtual address range [@vmemmap_addr, * @vmemmap_end) are mapped to are freed to the buddy allocator, and @@ -250,17 +247,18 @@ int alloc_huge_page_vmemmap(struct hstate *h, struct page *head) return ret; } -void free_huge_page_vmemmap(struct hstate *h, struct page *head) +void hugetlb_vmemmap_free(struct hstate *h, struct page *head) { unsigned long vmemmap_addr = (unsigned long)head; - unsigned long vmemmap_end, vmemmap_reuse; + unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages; - if (!free_vmemmap_pages_per_hpage(h)) + vmemmap_pages = hugetlb_optimize_vmemmap_pages(h); + if (!vmemmap_pages) return; - vmemmap_addr += RESERVE_VMEMMAP_SIZE; - vmemmap_end = vmemmap_addr + free_vmemmap_pages_size_per_hpage(h); - vmemmap_reuse = vmemmap_addr - PAGE_SIZE; + vmemmap_addr += RESERVE_VMEMMAP_SIZE; + vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT); + vmemmap_reuse = vmemmap_addr - PAGE_SIZE; /* * Remap the vmemmap virtual address range [@vmemmap_addr, @vmemmap_end) @@ -297,8 +295,8 @@ void __init hugetlb_vmemmap_init(struct hstate *h) * hugetlbpage.rst for more details. */ if (likely(vmemmap_pages > RESERVE_VMEMMAP_NR)) - h->nr_free_vmemmap_pages = vmemmap_pages - RESERVE_VMEMMAP_NR; + h->optimize_vmemmap_pages = vmemmap_pages - RESERVE_VMEMMAP_NR; - pr_info("can free %d vmemmap pages for %s\n", h->nr_free_vmemmap_pages, - h->name); + pr_info("can optimize %d vmemmap pages for %s\n", + h->optimize_vmemmap_pages, h->name); } diff --git a/mm/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h index cb2bef8f9e73..9760537849b5 100644 --- a/mm/hugetlb_vmemmap.h +++ b/mm/hugetlb_vmemmap.h @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Free some vmemmap pages of HugeTLB + * Optimize vmemmap pages associated with HugeTLB * * Copyright (c) 2020, Bytedance. All rights reserved. * @@ -11,25 +11,25 @@ #include <linux/hugetlb.h> #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP -int alloc_huge_page_vmemmap(struct hstate *h, struct page *head); -void free_huge_page_vmemmap(struct hstate *h, struct page *head); +int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head); +void hugetlb_vmemmap_free(struct hstate *h, struct page *head); void hugetlb_vmemmap_init(struct hstate *h); /* - * How many vmemmap pages associated with a HugeTLB page that can be freed - * to the buddy allocator. + * How many vmemmap pages associated with a HugeTLB page that can be + * optimized and freed to the buddy allocator. */ -static inline unsigned int free_vmemmap_pages_per_hpage(struct hstate *h) +static inline unsigned int hugetlb_optimize_vmemmap_pages(struct hstate *h) { - return h->nr_free_vmemmap_pages; + return h->optimize_vmemmap_pages; } #else -static inline int alloc_huge_page_vmemmap(struct hstate *h, struct page *head) +static inline int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head) { return 0; } -static inline void free_huge_page_vmemmap(struct hstate *h, struct page *head) +static inline void hugetlb_vmemmap_free(struct hstate *h, struct page *head) { } @@ -37,7 +37,7 @@ static inline void hugetlb_vmemmap_init(struct hstate *h) { } -static inline unsigned int free_vmemmap_pages_per_hpage(struct hstate *h) +static inline unsigned int hugetlb_optimize_vmemmap_pages(struct hstate *h) { return 0; }
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. 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(-)