* [PATCH v10 0/4] add hugetlb_optimize_vmemmap sysctl @ 2022-05-09 6:26 Muchun Song 2022-05-09 6:27 ` [PATCH v10 1/4] mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries Muchun Song ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Muchun Song @ 2022-05-09 6:26 UTC (permalink / raw) To: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun, Muchun Song This series is based on next-20220428. This series amis to add hugetlb_optimize_vmemmap sysctl to enable or disable the feature of optimizing vmemmap pages associated with HugeTLB pages. v10: - Collect Reviewed-by from Mike. - Remove hugetlb_optimize_vmemmap_enabled() check from hugetlb_optimize_vmemmap_pages() (Mike). - Add more explanation to Documentation/admin-guide/sysctl/vm.rst. - Fix cannot disable the feature via hugetlb_optimize_vmemmap sysctl (Mike). - Update patch 2's commit log (Mike). v9: - Go back to v3 since checking the size of struct page at config time is very complex. v8: - Fix compilation (scripts/selinux/mdp/mdp.c) error when CONFIG_SECURITY_SELINUX is selected. v7: - Fix circular dependency issue reported by kernel test robot. - Introduce CONFIG_HUGETLB_PAGE_HAS_OPTIMIZE_VMEMMAP instead of STRUCT_PAGE_SIZE_IS_POWER_OF_2. - Add more comments into vm.rst to explain hugetlb_optimize_vmemmap (Andrew). - Drop the patch "sysctl: allow to set extra1 to SYSCTL_ONE". - Add a new patch "use kstrtobool for hugetlb_vmemmap param parsing". - Reuse static_key's refcount to count the number of HugeTLB pages with vmemmap pages optimized to simplify the lock scheme. v6: - Remove "make syncconfig" from Kbuild. v5: - Fix not working properly if one is workig off of a very clean build reported by Luis Chamberlain. - Add Suggested-by for Luis Chamberlain. v4: - Introduce STRUCT_PAGE_SIZE_IS_POWER_OF_2 inspired by Luis. v3: - Add pr_warn_once() (Mike). - Handle the transition from enabling to disabling (Luis) v2: - Fix compilation when !CONFIG_MHP_MEMMAP_ON_MEMORY reported by kernel test robot <lkp@intel.com>. - Move sysctl code from kernel/sysctl.c to mm/hugetlb_vmemmap.c. Muchun Song (4): mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl Documentation/admin-guide/kernel-parameters.txt | 6 +- Documentation/admin-guide/sysctl/vm.rst | 39 +++++++++ include/linux/memory_hotplug.h | 9 ++ mm/hugetlb.c | 3 + mm/hugetlb_vmemmap.c | 105 ++++++++++++++++++++---- mm/memory_hotplug.c | 27 ++++-- 6 files changed, 165 insertions(+), 24 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v10 1/4] mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries 2022-05-09 6:26 [PATCH v10 0/4] add hugetlb_optimize_vmemmap sysctl Muchun Song @ 2022-05-09 6:27 ` Muchun Song 2022-05-12 7:35 ` David Hildenbrand 2022-05-09 6:27 ` [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on Muchun Song ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-09 6:27 UTC (permalink / raw) To: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun, Muchun Song If the size of "struct page" is not the power of two but with the feature of minimizing overhead of struct page associated with each HugeTLB is enabled, then the vmemmap pages of HugeTLB will be corrupted after remapping (panic is about to happen in theory). But this only exists when !CONFIG_MEMCG && !CONFIG_SLUB on x86_64. However, it is not a conventional configuration nowadays. So it is not a real word issue, just the result of a code review. But we cannot prevent anyone from configuring that combined configure. This hugetlb_optimize_vmemmap should be disable in this case to fix this issue. Signed-off-by: Muchun Song <songmuchun@bytedance.com> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> --- mm/hugetlb_vmemmap.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index 29554c6ef2ae..6254bb2d4ae5 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -28,12 +28,6 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); 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))) { - pr_warn("cannot free vmemmap pages because \"struct page\" crosses page boundaries\n"); - return 0; - } - if (!buf) return -EINVAL; @@ -119,6 +113,12 @@ void __init hugetlb_vmemmap_init(struct hstate *h) if (!hugetlb_optimize_vmemmap_enabled()) return; + if (!is_power_of_2(sizeof(struct page))) { + pr_warn_once("cannot optimize vmemmap pages because \"struct page\" crosses page boundaries\n"); + static_branch_disable(&hugetlb_optimize_vmemmap_key); + return; + } + vmemmap_pages = (nr_pages * sizeof(struct page)) >> PAGE_SHIFT; /* * The head page is not to be freed to buddy allocator, the other tail -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v10 1/4] mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries 2022-05-09 6:27 ` [PATCH v10 1/4] mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries Muchun Song @ 2022-05-12 7:35 ` David Hildenbrand 0 siblings, 0 replies; 19+ messages in thread From: David Hildenbrand @ 2022-05-12 7:35 UTC (permalink / raw) To: Muchun Song, corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 09.05.22 08:27, Muchun Song wrote: > If the size of "struct page" is not the power of two but with the feature > of minimizing overhead of struct page associated with each HugeTLB is > enabled, then the vmemmap pages of HugeTLB will be corrupted after > remapping (panic is about to happen in theory). But this only exists when > !CONFIG_MEMCG && !CONFIG_SLUB on x86_64. However, it is not a conventional > configuration nowadays. So it is not a real word issue, just the result > of a code review. > > But we cannot prevent anyone from configuring that combined configure. > This hugetlb_optimize_vmemmap should be disable in this case to fix this > issue. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> Acked-by: David Hildenbrand <david@redhat.com> -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on 2022-05-09 6:26 [PATCH v10 0/4] add hugetlb_optimize_vmemmap sysctl Muchun Song 2022-05-09 6:27 ` [PATCH v10 1/4] mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries Muchun Song @ 2022-05-09 6:27 ` Muchun Song 2022-05-12 7:36 ` David Hildenbrand 2022-05-09 6:27 ` [PATCH v10 3/4] mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing Muchun Song 2022-05-09 6:27 ` [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl Muchun Song 3 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-09 6:27 UTC (permalink / raw) To: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun, Muchun Song Optimizing HugeTLB vmemmap pages is not compatible with allocating memmap on hot added memory. If "hugetlb_free_vmemmap=on" and memory_hotplug.memmap_on_memory" are both passed on the kernel command line, optimizing hugetlb pages takes precedence. However, the global variable memmap_on_memory will still be set to 1, even though we will not try to allocate memmap on hot added memory. Also introduce mhp_memmap_on_memory() helper to move the definition of "memmap_on_memory" to the scope of CONFIG_MHP_MEMMAP_ON_MEMORY. In the next patch, mhp_memmap_on_memory() will also be exported to be used in hugetlb_vmemmap.c. Signed-off-by: Muchun Song <songmuchun@bytedance.com> Acked-by: Mike Kravetz <mike.kravetz@oracle.com> --- mm/memory_hotplug.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 111684878fd9..a6101ae402f9 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -42,14 +42,36 @@ #include "internal.h" #include "shuffle.h" +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY +static int memmap_on_memory_set(const char *val, const struct kernel_param *kp) +{ + if (hugetlb_optimize_vmemmap_enabled()) + return 0; + return param_set_bool(val, kp); +} + +static const struct kernel_param_ops memmap_on_memory_ops = { + .flags = KERNEL_PARAM_OPS_FL_NOARG, + .set = memmap_on_memory_set, + .get = param_get_bool, +}; /* * memory_hotplug.memmap_on_memory parameter */ static bool memmap_on_memory __ro_after_init; -#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY -module_param(memmap_on_memory, bool, 0444); +module_param_cb(memmap_on_memory, &memmap_on_memory_ops, &memmap_on_memory, 0444); MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug"); + +static inline bool mhp_memmap_on_memory(void) +{ + return memmap_on_memory; +} +#else +static inline bool mhp_memmap_on_memory(void) +{ + return false; +} #endif enum { @@ -1263,9 +1285,7 @@ bool mhp_supports_memmap_on_memory(unsigned long size) * altmap as an alternative source of memory, and we do not exactly * populate a single PMD. */ - return memmap_on_memory && - !hugetlb_optimize_vmemmap_enabled() && - IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) && + return mhp_memmap_on_memory() && size == memory_block_size_bytes() && IS_ALIGNED(vmemmap_size, PMD_SIZE) && IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT)); @@ -2083,7 +2103,7 @@ static int __ref try_remove_memory(u64 start, u64 size) * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in * the same granularity it was added - a single memory block. */ - if (memmap_on_memory) { + if (mhp_memmap_on_memory()) { nr_vmemmap_pages = walk_memory_blocks(start, size, NULL, get_nr_vmemmap_pages_cb); if (nr_vmemmap_pages) { -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on 2022-05-09 6:27 ` [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on Muchun Song @ 2022-05-12 7:36 ` David Hildenbrand 2022-05-12 12:50 ` Muchun Song 0 siblings, 1 reply; 19+ messages in thread From: David Hildenbrand @ 2022-05-12 7:36 UTC (permalink / raw) To: Muchun Song, corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 09.05.22 08:27, Muchun Song wrote: > Optimizing HugeTLB vmemmap pages is not compatible with allocating memmap on > hot added memory. If "hugetlb_free_vmemmap=on" and > memory_hotplug.memmap_on_memory" are both passed on the kernel command line, > optimizing hugetlb pages takes precedence. Why? -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on 2022-05-12 7:36 ` David Hildenbrand @ 2022-05-12 12:50 ` Muchun Song 2022-05-12 13:04 ` David Hildenbrand 0 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-12 12:50 UTC (permalink / raw) To: David Hildenbrand Cc: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On Thu, May 12, 2022 at 09:36:15AM +0200, David Hildenbrand wrote: > On 09.05.22 08:27, Muchun Song wrote: > > Optimizing HugeTLB vmemmap pages is not compatible with allocating memmap on > > hot added memory. If "hugetlb_free_vmemmap=on" and > > memory_hotplug.memmap_on_memory" are both passed on the kernel command line, > > optimizing hugetlb pages takes precedence. > > Why? > Because both two features are not compatible since hugetlb_free_vmemmap cannot optimize the vmemmap pages allocated from alternative allocator (when memory_hotplug.memmap_on_memory=1). So when the feature of hugetlb_free_vmemmap is introduced, I made hugetlb_free_vmemmap take precedence. BTW, I have a plan to remove this restriction, I'll post it out ASAP. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on 2022-05-12 12:50 ` Muchun Song @ 2022-05-12 13:04 ` David Hildenbrand 2022-05-12 13:59 ` Muchun Song 0 siblings, 1 reply; 19+ messages in thread From: David Hildenbrand @ 2022-05-12 13:04 UTC (permalink / raw) To: Muchun Song Cc: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 12.05.22 14:50, Muchun Song wrote: > On Thu, May 12, 2022 at 09:36:15AM +0200, David Hildenbrand wrote: >> On 09.05.22 08:27, Muchun Song wrote: >>> Optimizing HugeTLB vmemmap pages is not compatible with allocating memmap on >>> hot added memory. If "hugetlb_free_vmemmap=on" and >>> memory_hotplug.memmap_on_memory" are both passed on the kernel command line, >>> optimizing hugetlb pages takes precedence. >> >> Why? >> > > Because both two features are not compatible since hugetlb_free_vmemmap cannot > optimize the vmemmap pages allocated from alternative allocator (when > memory_hotplug.memmap_on_memory=1). So when the feature of hugetlb_free_vmemmap > is introduced, I made hugetlb_free_vmemmap take precedence. BTW, I have a plan > to remove this restriction, I'll post it out ASAP. I was asking why vmemmap optimization should take precedence. memmap_on_memory makes it more likely to succeed memory hotplug in close-to-OOM situations -- which is IMHO more important than a vmemmap optimization. But anyhow, the proper approach should most probably be to simply not mess with the vmemmap if we stumble over a vmemmap that's special due to memmap_on_memory. I assume that's what you're talking about sending out. -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on 2022-05-12 13:04 ` David Hildenbrand @ 2022-05-12 13:59 ` Muchun Song 2022-05-12 16:38 ` David Hildenbrand 0 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-12 13:59 UTC (permalink / raw) To: David Hildenbrand Cc: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On Thu, May 12, 2022 at 03:04:57PM +0200, David Hildenbrand wrote: > On 12.05.22 14:50, Muchun Song wrote: > > On Thu, May 12, 2022 at 09:36:15AM +0200, David Hildenbrand wrote: > >> On 09.05.22 08:27, Muchun Song wrote: > >>> Optimizing HugeTLB vmemmap pages is not compatible with allocating memmap on > >>> hot added memory. If "hugetlb_free_vmemmap=on" and > >>> memory_hotplug.memmap_on_memory" are both passed on the kernel command line, > >>> optimizing hugetlb pages takes precedence. > >> > >> Why? > >> > > > > Because both two features are not compatible since hugetlb_free_vmemmap cannot > > optimize the vmemmap pages allocated from alternative allocator (when > > memory_hotplug.memmap_on_memory=1). So when the feature of hugetlb_free_vmemmap > > is introduced, I made hugetlb_free_vmemmap take precedence. BTW, I have a plan > > to remove this restriction, I'll post it out ASAP. > > I was asking why vmemmap optimization should take precedence. > memmap_on_memory makes it more likely to succeed memory hotplug in > close-to-OOM situations -- which is IMHO more important than a vmemmap > optimization. > I thought the users who enable hugetlb_free_vmemmap value memory savings more, so I made a decision in commit 4bab4964a59f. Seems I made a bad decision from your description. > But anyhow, the proper approach should most probably be to simply not > mess with the vmemmap if we stumble over a vmemmap that's special due to > memmap_on_memory. I assume that's what you're talking about sending out. > I mean I want to have hugetlb_vmemmap.c do the check whether the section which the HugeTLB pages belong to can be optimized instead of making hugetlb_free_vmemmap take precedence. E.g. If the section's vmemmap pages are allocated from the added memory block itself, hugetlb_free_vmemmap will refuse to optimize the vmemmap, otherwise, do the optimization. Then both kernel parameters are compatible. I have done those patches, but haven't send them out. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on 2022-05-12 13:59 ` Muchun Song @ 2022-05-12 16:38 ` David Hildenbrand 0 siblings, 0 replies; 19+ messages in thread From: David Hildenbrand @ 2022-05-12 16:38 UTC (permalink / raw) To: Muchun Song Cc: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 12.05.22 15:59, Muchun Song wrote: > On Thu, May 12, 2022 at 03:04:57PM +0200, David Hildenbrand wrote: >> On 12.05.22 14:50, Muchun Song wrote: >>> On Thu, May 12, 2022 at 09:36:15AM +0200, David Hildenbrand wrote: >>>> On 09.05.22 08:27, Muchun Song wrote: >>>>> Optimizing HugeTLB vmemmap pages is not compatible with allocating memmap on >>>>> hot added memory. If "hugetlb_free_vmemmap=on" and >>>>> memory_hotplug.memmap_on_memory" are both passed on the kernel command line, >>>>> optimizing hugetlb pages takes precedence. >>>> >>>> Why? >>>> >>> >>> Because both two features are not compatible since hugetlb_free_vmemmap cannot >>> optimize the vmemmap pages allocated from alternative allocator (when >>> memory_hotplug.memmap_on_memory=1). So when the feature of hugetlb_free_vmemmap >>> is introduced, I made hugetlb_free_vmemmap take precedence. BTW, I have a plan >>> to remove this restriction, I'll post it out ASAP. >> >> I was asking why vmemmap optimization should take precedence. >> memmap_on_memory makes it more likely to succeed memory hotplug in >> close-to-OOM situations -- which is IMHO more important than a vmemmap >> optimization. >> > > I thought the users who enable hugetlb_free_vmemmap value memory > savings more, so I made a decision in commit 4bab4964a59f. Seems > I made a bad decision from your description. Depends on the perspective I guess. :) > >> But anyhow, the proper approach should most probably be to simply not >> mess with the vmemmap if we stumble over a vmemmap that's special due to >> memmap_on_memory. I assume that's what you're talking about sending out. >> > > I mean I want to have hugetlb_vmemmap.c do the check whether the section > which the HugeTLB pages belong to can be optimized instead of making > hugetlb_free_vmemmap take precedence. E.g. If the section's vmemmap pages > are allocated from the added memory block itself, hugetlb_free_vmemmap will > refuse to optimize the vmemmap, otherwise, do the optimization. Then > both kernel parameters are compatible. I have done those patches, but > haven't send them out. Yeah, that's exactly what I thought. How complicated are they? If they are easy, can we just avoid this patch here and do it "properly"? :) -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v10 3/4] mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing 2022-05-09 6:26 [PATCH v10 0/4] add hugetlb_optimize_vmemmap sysctl Muchun Song 2022-05-09 6:27 ` [PATCH v10 1/4] mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries Muchun Song 2022-05-09 6:27 ` [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on Muchun Song @ 2022-05-09 6:27 ` Muchun Song 2022-05-12 7:41 ` David Hildenbrand 2022-05-09 6:27 ` [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl Muchun Song 3 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-09 6:27 UTC (permalink / raw) To: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun, Muchun Song Use kstrtobool rather than open coding "on" and "off" parsing in mm/hugetlb_vmemmap.c, which is more powerful to handle all kinds of parameters like 'Yy1Nn0' or [oO][NnFf] for "on" and "off". Signed-off-by: Muchun Song <songmuchun@bytedance.com> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> --- Documentation/admin-guide/kernel-parameters.txt | 6 +++--- mm/hugetlb_vmemmap.c | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 308da668bbb1..43b8385073ad 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1703,10 +1703,10 @@ enabled. Allows heavy hugetlb users to free up some more memory (7 * PAGE_SIZE for each 2MB hugetlb page). - Format: { on | off (default) } + Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 (default) } - on: enable the feature - off: disable the feature + [oO][Nn]/Y/y/1: enable the feature + [oO][Ff]/N/n/0: disable the feature Built with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON=y, the default is on. diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index 6254bb2d4ae5..cc4ec752ec16 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -28,15 +28,15 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); static int __init hugetlb_vmemmap_early_param(char *buf) { - if (!buf) + bool enable; + + if (kstrtobool(buf, &enable)) return -EINVAL; - if (!strcmp(buf, "on")) + if (enable) static_branch_enable(&hugetlb_optimize_vmemmap_key); - else if (!strcmp(buf, "off")) - static_branch_disable(&hugetlb_optimize_vmemmap_key); else - return -EINVAL; + static_branch_disable(&hugetlb_optimize_vmemmap_key); return 0; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v10 3/4] mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing 2022-05-09 6:27 ` [PATCH v10 3/4] mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing Muchun Song @ 2022-05-12 7:41 ` David Hildenbrand 2022-05-12 11:23 ` Muchun Song 0 siblings, 1 reply; 19+ messages in thread From: David Hildenbrand @ 2022-05-12 7:41 UTC (permalink / raw) To: Muchun Song, corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 09.05.22 08:27, Muchun Song wrote: > Use kstrtobool rather than open coding "on" and "off" parsing in > mm/hugetlb_vmemmap.c, which is more powerful to handle all kinds > of parameters like 'Yy1Nn0' or [oO][NnFf] for "on" and "off". > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > Documentation/admin-guide/kernel-parameters.txt | 6 +++--- > mm/hugetlb_vmemmap.c | 10 +++++----- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 308da668bbb1..43b8385073ad 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1703,10 +1703,10 @@ > enabled. > Allows heavy hugetlb users to free up some more > memory (7 * PAGE_SIZE for each 2MB hugetlb page). > - Format: { on | off (default) } > + Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 (default) } Really? Can we make the syntax even harder to parse for human beings?! :) Not to mention that it's partially wrong? What about "oFf" ? That would have to be [oO][Ff][Ff] Honestly, "on | off" is good enough. That "oN" and friends work is just a "nice to have" IMHO. No need to over-complicate this description. > > - on: enable the feature > - off: disable the feature > + [oO][Nn]/Y/y/1: enable the feature > + [oO][Ff]/N/n/0: disable the feature > > Built with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON=y, > the default is on. > diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c > index 6254bb2d4ae5..cc4ec752ec16 100644 > --- a/mm/hugetlb_vmemmap.c > +++ b/mm/hugetlb_vmemmap.c > @@ -28,15 +28,15 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); > > static int __init hugetlb_vmemmap_early_param(char *buf) > { > - if (!buf) > + bool enable; > + > + if (kstrtobool(buf, &enable)) > return -EINVAL; > > - if (!strcmp(buf, "on")) > + if (enable) > static_branch_enable(&hugetlb_optimize_vmemmap_key); > - else if (!strcmp(buf, "off")) > - static_branch_disable(&hugetlb_optimize_vmemmap_key); > else > - return -EINVAL; > + static_branch_disable(&hugetlb_optimize_vmemmap_key); > > return 0; > } Apart from that Acked-by: David Hildenbrand <david@redhat.com> -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 3/4] mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing 2022-05-12 7:41 ` David Hildenbrand @ 2022-05-12 11:23 ` Muchun Song 0 siblings, 0 replies; 19+ messages in thread From: Muchun Song @ 2022-05-12 11:23 UTC (permalink / raw) To: David Hildenbrand Cc: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On Thu, May 12, 2022 at 09:41:22AM +0200, David Hildenbrand wrote: > On 09.05.22 08:27, Muchun Song wrote: > > Use kstrtobool rather than open coding "on" and "off" parsing in > > mm/hugetlb_vmemmap.c, which is more powerful to handle all kinds > > of parameters like 'Yy1Nn0' or [oO][NnFf] for "on" and "off". > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > > --- > > Documentation/admin-guide/kernel-parameters.txt | 6 +++--- > > mm/hugetlb_vmemmap.c | 10 +++++----- > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index 308da668bbb1..43b8385073ad 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -1703,10 +1703,10 @@ > > enabled. > > Allows heavy hugetlb users to free up some more > > memory (7 * PAGE_SIZE for each 2MB hugetlb page). > > - Format: { on | off (default) } > > + Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 (default) } > > Really? Can we make the syntax even harder to parse for human beings?! :) > > Not to mention that it's partially wrong? What about "oFf" ? That would > have to be [oO][Ff][Ff] > > Honestly, "on | off" is good enough. That "oN" and friends work is just > a "nice to have" IMHO. No need to over-complicate this description. Got it. How about also telling users "on/1 | off/0"? Because 0 and 1 are also widely used to disable/enable a knob. > > > > - on: enable the feature > > - off: disable the feature > > + [oO][Nn]/Y/y/1: enable the feature > > + [oO][Ff]/N/n/0: disable the feature > > > > Built with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON=y, > > the default is on. > > diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c > > index 6254bb2d4ae5..cc4ec752ec16 100644 > > --- a/mm/hugetlb_vmemmap.c > > +++ b/mm/hugetlb_vmemmap.c > > @@ -28,15 +28,15 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); > > > > static int __init hugetlb_vmemmap_early_param(char *buf) > > { > > - if (!buf) > > + bool enable; > > + > > + if (kstrtobool(buf, &enable)) > > return -EINVAL; > > > > - if (!strcmp(buf, "on")) > > + if (enable) > > static_branch_enable(&hugetlb_optimize_vmemmap_key); > > - else if (!strcmp(buf, "off")) > > - static_branch_disable(&hugetlb_optimize_vmemmap_key); > > else > > - return -EINVAL; > > + static_branch_disable(&hugetlb_optimize_vmemmap_key); > > > > return 0; > > } > > > Apart from that > > Acked-by: David Hildenbrand <david@redhat.com> > Thanks. > -- > Thanks, > > David / dhildenb > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl 2022-05-09 6:26 [PATCH v10 0/4] add hugetlb_optimize_vmemmap sysctl Muchun Song ` (2 preceding siblings ...) 2022-05-09 6:27 ` [PATCH v10 3/4] mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing Muchun Song @ 2022-05-09 6:27 ` Muchun Song 2022-05-10 21:30 ` Mike Kravetz 3 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-09 6:27 UTC (permalink / raw) To: corbet, mike.kravetz, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun, Muchun Song We must add hugetlb_free_vmemmap=on (or "off") to the boot cmdline and reboot the server to enable or disable the feature of optimizing vmemmap pages associated with HugeTLB pages. However, rebooting usually takes a long time. So add a sysctl to enable or disable the feature at runtime without rebooting. Why we need this? There are 3 use cases. 1) The feature of minimizing overhead of struct page associated with each HugeTLB is disabled by default without passing "hugetlb_free_vmemmap=on" to the boot cmdline. When we (ByteDance) deliver the servers to the users who want to enable this feature, they have to configure the grub (change boot cmdline) and reboot the servers, whereas rebooting usually takes a long time (we have thousands of servers). It's a very bad experience for the users. So we need a approach to enable this feature after rebooting. This is a use case in our practical environment. 2) Some use cases are that HugeTLB pages are allocated 'on the fly' instead of being pulled from the HugeTLB pool, those workloads would be affected with this feature enabled. Those workloads could be identified by the characteristics of they never explicitly allocating huge pages with 'nr_hugepages' but only set 'nr_overcommit_hugepages' and then let the pages be allocated from the buddy allocator at fault time. We can confirm it is a real use case from the commit 099730d67417. For those workloads, the page fault time could be ~2x slower than before. We suspect those users want to disable this feature if the system has enabled this before and they don't think the memory savings benefit is enough to make up for the performance drop. 3) If the workload which wants vmemmap pages to be optimized and the workload which wants to set 'nr_overcommit_hugepages' and does not want the extera overhead at fault time when the overcommitted pages be allocated from the buddy allocator are deployed in the same server. The user could enable this feature and set 'nr_hugepages' and 'nr_overcommit_hugepages', then disable the feature. In this case, the overcommited HugeTLB pages will not encounter the extra overhead at fault time. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- Documentation/admin-guide/sysctl/vm.rst | 39 ++++++++++++++ include/linux/memory_hotplug.h | 9 ++++ mm/hugetlb.c | 3 ++ mm/hugetlb_vmemmap.c | 93 +++++++++++++++++++++++++++++---- mm/memory_hotplug.c | 7 +-- 5 files changed, 136 insertions(+), 15 deletions(-) diff --git a/Documentation/admin-guide/sysctl/vm.rst b/Documentation/admin-guide/sysctl/vm.rst index 747e325ebcd0..5c9aa171a0d3 100644 --- a/Documentation/admin-guide/sysctl/vm.rst +++ b/Documentation/admin-guide/sysctl/vm.rst @@ -562,6 +562,45 @@ Change the minimum size of the hugepage pool. See Documentation/admin-guide/mm/hugetlbpage.rst +hugetlb_optimize_vmemmap +======================== + +This knob is not available when memory_hotplug.memmap_on_memory (kernel parameter) +is configured or the size of 'struct page' (a structure defined in +include/linux/mm_types.h) is not power of two (an unusual system config could +result in this). + +Enable (set to 1) or disable (set to 0) the feature of optimizing vmemmap pages +associated with each HugeTLB page. + +Once enabled, the vmemmap pages of subsequent allocation of HugeTLB pages from +buddy allocator will be optimized (7 pages per 2MB HugeTLB page and 4095 pages +per 1GB HugeTLB page), whereas already allocated HugeTLB pages will not be +optimized. When those optimized HugeTLB pages are freed from the HugeTLB pool +to the buddy allocator, the vmemmap pages representing that range needs to be +remapped again and the vmemmap pages discarded earlier need to be rellocated +again. If your use case is that HugeTLB pages are allocated 'on the fly' (e.g. +never explicitly allocating HugeTLB pages with 'nr_hugepages' but only set +'nr_overcommit_hugepages', those overcommitted HugeTLB pages are allocated 'on +the fly') instead of being pulled from the HugeTLB pool, you should weigh the +benefits of memory savings against the more overhead (~2x slower than before) +of allocation or freeing HugeTLB pages between the HugeTLB pool and the buddy +allocator. Another behavior to note is that if the system is under heavy memory +pressure, it could prevent the user from freeing HugeTLB pages from the HugeTLB +pool to the buddy allocator since the allocation of vmemmap pages could be +failed, you have to retry later if your system encounter this situation. + +Once disabled, the vmemmap pages of subsequent allocation of HugeTLB pages from +buddy allocator will not be optimized meaning the extra overhead at allocation +time from buddy allocator disappears, whereas already optimized HugeTLB pages +will not be affected. If you want to make sure there are no optimized HugeTLB +pages, you can set "nr_hugepages" to 0 first and then disable this. Note that +writing 0 to nr_hugepages will make any "in use" HugeTLB pages become surplus +pages. So, those surplus pages are still optimized until they are no longer +in use. You would need to wait for those surplus pages to be released before +there are no optimized pages in the system. + + nr_hugepages_mempolicy ====================== diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h index 029fb7e26504..917112661b5c 100644 --- a/include/linux/memory_hotplug.h +++ b/include/linux/memory_hotplug.h @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size); extern bool mhp_supports_memmap_on_memory(unsigned long size); #endif /* CONFIG_MEMORY_HOTPLUG */ +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY +bool mhp_memmap_on_memory(void); +#else +static inline bool mhp_memmap_on_memory(void) +{ + return false; +} +#endif + #endif /* __LINUX_MEMORY_HOTPLUG_H */ diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 8605d7eb7f5c..86158eb9da70 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); static inline void flush_free_hpage_work(struct hstate *h) { + if (!hugetlb_optimize_vmemmap_enabled()) + return; + if (hugetlb_optimize_vmemmap_pages(h)) flush_work(&free_hpage_work); } diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index cc4ec752ec16..fcd9f7872064 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -10,6 +10,7 @@ */ #define pr_fmt(fmt) "HugeTLB: " fmt +#include <linux/memory_hotplug.h> #include "hugetlb_vmemmap.h" /* @@ -22,21 +23,40 @@ #define RESERVE_VMEMMAP_NR 1U #define RESERVE_VMEMMAP_SIZE (RESERVE_VMEMMAP_NR << PAGE_SHIFT) +enum vmemmap_optimize_mode { + VMEMMAP_OPTIMIZE_OFF, + VMEMMAP_OPTIMIZE_ON, +}; + DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON, hugetlb_optimize_vmemmap_key); EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); +static enum vmemmap_optimize_mode vmemmap_optimize_mode = + IS_ENABLED(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON); + +static void vmemmap_optimize_mode_switch(enum vmemmap_optimize_mode to) +{ + if (vmemmap_optimize_mode == to) + return; + + if (to == VMEMMAP_OPTIMIZE_OFF) + static_branch_dec(&hugetlb_optimize_vmemmap_key); + else + static_branch_inc(&hugetlb_optimize_vmemmap_key); + WRITE_ONCE(vmemmap_optimize_mode, to); +} + static int __init hugetlb_vmemmap_early_param(char *buf) { bool enable; + enum vmemmap_optimize_mode mode; if (kstrtobool(buf, &enable)) return -EINVAL; - if (enable) - static_branch_enable(&hugetlb_optimize_vmemmap_key); - else - static_branch_disable(&hugetlb_optimize_vmemmap_key); + mode = enable ? VMEMMAP_OPTIMIZE_ON : VMEMMAP_OPTIMIZE_OFF; + vmemmap_optimize_mode_switch(mode); return 0; } @@ -69,8 +89,10 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head) */ ret = vmemmap_remap_alloc(vmemmap_addr, vmemmap_end, vmemmap_reuse, GFP_KERNEL | __GFP_NORETRY | __GFP_THISNODE); - if (!ret) + if (!ret) { ClearHPageVmemmapOptimized(head); + static_branch_dec(&hugetlb_optimize_vmemmap_key); + } return ret; } @@ -84,6 +106,11 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head) if (!vmemmap_pages) return; + if (READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF) + return; + + static_branch_inc(&hugetlb_optimize_vmemmap_key); + vmemmap_addr += RESERVE_VMEMMAP_SIZE; vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT); vmemmap_reuse = vmemmap_addr - PAGE_SIZE; @@ -93,7 +120,9 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head) * to the page which @vmemmap_reuse is mapped to, then free the pages * which the range [@vmemmap_addr, @vmemmap_end] is mapped to. */ - if (!vmemmap_remap_free(vmemmap_addr, vmemmap_end, vmemmap_reuse)) + if (vmemmap_remap_free(vmemmap_addr, vmemmap_end, vmemmap_reuse)) + static_branch_dec(&hugetlb_optimize_vmemmap_key); + else SetHPageVmemmapOptimized(head); } @@ -110,9 +139,6 @@ void __init hugetlb_vmemmap_init(struct hstate *h) BUILD_BUG_ON(__NR_USED_SUBPAGE >= RESERVE_VMEMMAP_SIZE / sizeof(struct page)); - if (!hugetlb_optimize_vmemmap_enabled()) - return; - if (!is_power_of_2(sizeof(struct page))) { pr_warn_once("cannot optimize vmemmap pages because \"struct page\" crosses page boundaries\n"); static_branch_disable(&hugetlb_optimize_vmemmap_key); @@ -134,3 +160,52 @@ void __init hugetlb_vmemmap_init(struct hstate *h) pr_info("can optimize %d vmemmap pages for %s\n", h->optimize_vmemmap_pages, h->name); } + +#ifdef CONFIG_PROC_SYSCTL +static int hugetlb_optimize_vmemmap_handler(struct ctl_table *table, int write, + void *buffer, size_t *length, + loff_t *ppos) +{ + int ret; + enum vmemmap_optimize_mode mode; + static DEFINE_MUTEX(sysctl_mutex); + + if (write && !capable(CAP_SYS_ADMIN)) + return -EPERM; + + mutex_lock(&sysctl_mutex); + mode = vmemmap_optimize_mode; + table->data = &mode; + ret = proc_dointvec_minmax(table, write, buffer, length, ppos); + if (write && !ret) + vmemmap_optimize_mode_switch(mode); + mutex_unlock(&sysctl_mutex); + + return ret; +} + +static struct ctl_table hugetlb_vmemmap_sysctls[] = { + { + .procname = "hugetlb_optimize_vmemmap", + .maxlen = sizeof(enum vmemmap_optimize_mode), + .mode = 0644, + .proc_handler = hugetlb_optimize_vmemmap_handler, + .extra1 = SYSCTL_ZERO, + .extra2 = SYSCTL_ONE, + }, + { } +}; + +static __init int hugetlb_vmemmap_sysctls_init(void) +{ + /* + * If "memory_hotplug.memmap_on_memory" is enabled or "struct page" + * crosses page boundaries, the vmemmap pages cannot be optimized. + */ + if (!mhp_memmap_on_memory() && is_power_of_2(sizeof(struct page))) + register_sysctl_init("vm", hugetlb_vmemmap_sysctls); + + return 0; +} +late_initcall(hugetlb_vmemmap_sysctls_init); +#endif /* CONFIG_PROC_SYSCTL */ diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index a6101ae402f9..c72070cdd055 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -63,15 +63,10 @@ static bool memmap_on_memory __ro_after_init; module_param_cb(memmap_on_memory, &memmap_on_memory_ops, &memmap_on_memory, 0444); MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug"); -static inline bool mhp_memmap_on_memory(void) +bool mhp_memmap_on_memory(void) { return memmap_on_memory; } -#else -static inline bool mhp_memmap_on_memory(void) -{ - return false; -} #endif enum { -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl 2022-05-09 6:27 ` [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl Muchun Song @ 2022-05-10 21:30 ` Mike Kravetz 2022-05-11 0:39 ` Mike Kravetz 0 siblings, 1 reply; 19+ messages in thread From: Mike Kravetz @ 2022-05-10 21:30 UTC (permalink / raw) To: Muchun Song, corbet, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 5/8/22 23:27, Muchun Song wrote: > diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h > index 029fb7e26504..917112661b5c 100644 > --- a/include/linux/memory_hotplug.h > +++ b/include/linux/memory_hotplug.h > @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size); > extern bool mhp_supports_memmap_on_memory(unsigned long size); > #endif /* CONFIG_MEMORY_HOTPLUG */ > > +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY > +bool mhp_memmap_on_memory(void); > +#else > +static inline bool mhp_memmap_on_memory(void) > +{ > + return false; > +} > +#endif > + > #endif /* __LINUX_MEMORY_HOTPLUG_H */ > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 8605d7eb7f5c..86158eb9da70 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); > > static inline void flush_free_hpage_work(struct hstate *h) > { > + if (!hugetlb_optimize_vmemmap_enabled()) > + return; > + Hi Muchun, In v9 I was suggesting that we may be able to eliminate the static_branch_inc/dec from the vmemmap free/alloc paths. With this patch I believe hugetlb_optimize_vmemmap_enabled() is really checking 'has hugetlb vmemmap optimization been enabled' OR 'are there still vmemmap optimized hugetlb pages in the system'. That may be confusing. For this specific routine (flush_free_hpage_work) I do not think we need to worry too much about deciding to call flush_work or not. This is only called via set_max_huge_pages which is not a performance sensitive path. > if (hugetlb_optimize_vmemmap_pages(h)) > flush_work(&free_hpage_work); > } Here is a patch on top of this patch to show my suggestion for removing static_branch_inc/dec from the vmemmap free/alloc paths. It seems simpler to me, and hugetlb_optimize_vmemmap_enabled would only return true if hugetlb vmemmap optimization is currently enabled. I am not insisting that static_branch_inc/dec be eliminated. It may not even work. I have not tested. What do you think? diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c index fc4f710e9820..2f80751b7c3a 100644 --- a/arch/arm64/mm/flush.c +++ b/arch/arm64/mm/flush.c @@ -9,6 +9,7 @@ #include <linux/export.h> #include <linux/mm.h> #include <linux/pagemap.h> +#include <linux/hugetlb.h> #include <asm/cacheflush.h> #include <asm/cache.h> @@ -86,7 +87,7 @@ void flush_dcache_page(struct page *page) * is reused (more details can refer to the comments above * page_fixed_fake_head()). */ - if (hugetlb_optimize_vmemmap_enabled() && PageHuge(page)) + if (PageHuge(page) && HPageVmemmapOptimized(compound_head(page))) page = compound_head(page); if (test_bit(PG_dcache_clean, &page->flags)) diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 86158eb9da70..8605d7eb7f5c 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -1617,9 +1617,6 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); static inline void flush_free_hpage_work(struct hstate *h) { - if (!hugetlb_optimize_vmemmap_enabled()) - return; - if (hugetlb_optimize_vmemmap_pages(h)) flush_work(&free_hpage_work); } diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index fcd9f7872064..8e0890a505b3 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -41,9 +41,9 @@ static void vmemmap_optimize_mode_switch(enum vmemmap_optimize_mode to) return; if (to == VMEMMAP_OPTIMIZE_OFF) - static_branch_dec(&hugetlb_optimize_vmemmap_key); + static_branch_disable(&hugetlb_optimize_vmemmap_key); else - static_branch_inc(&hugetlb_optimize_vmemmap_key); + static_branch_enable(&hugetlb_optimize_vmemmap_key); WRITE_ONCE(vmemmap_optimize_mode, to); } @@ -91,7 +91,6 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head) GFP_KERNEL | __GFP_NORETRY | __GFP_THISNODE); if (!ret) { ClearHPageVmemmapOptimized(head); - static_branch_dec(&hugetlb_optimize_vmemmap_key); } return ret; @@ -102,14 +101,10 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head) unsigned long vmemmap_addr = (unsigned long)head; unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages; - vmemmap_pages = hugetlb_optimize_vmemmap_pages(h); - if (!vmemmap_pages) - return; - - if (READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF) + if (!hugetlb_optimize_vmemmap_enabled()) return; - static_branch_inc(&hugetlb_optimize_vmemmap_key); + vmemmap_pages = hugetlb_optimize_vmemmap_pages(h); vmemmap_addr += RESERVE_VMEMMAP_SIZE; vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT); @@ -120,9 +115,7 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head) * to the page which @vmemmap_reuse is mapped to, then free the pages * which the range [@vmemmap_addr, @vmemmap_end] is mapped to. */ - if (vmemmap_remap_free(vmemmap_addr, vmemmap_end, vmemmap_reuse)) - static_branch_dec(&hugetlb_optimize_vmemmap_key); - else + if (!vmemmap_remap_free(vmemmap_addr, vmemmap_end, vmemmap_reuse)) SetHPageVmemmapOptimized(head); } -- Mike Kravetz ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl 2022-05-10 21:30 ` Mike Kravetz @ 2022-05-11 0:39 ` Mike Kravetz 2022-05-11 9:45 ` Muchun Song 0 siblings, 1 reply; 19+ messages in thread From: Mike Kravetz @ 2022-05-11 0:39 UTC (permalink / raw) To: Muchun Song, corbet, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy Cc: linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 5/10/22 14:30, Mike Kravetz wrote: > On 5/8/22 23:27, Muchun Song wrote: >> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h >> index 029fb7e26504..917112661b5c 100644 >> --- a/include/linux/memory_hotplug.h >> +++ b/include/linux/memory_hotplug.h >> @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size); >> extern bool mhp_supports_memmap_on_memory(unsigned long size); >> #endif /* CONFIG_MEMORY_HOTPLUG */ >> >> +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY >> +bool mhp_memmap_on_memory(void); >> +#else >> +static inline bool mhp_memmap_on_memory(void) >> +{ >> + return false; >> +} >> +#endif >> + >> #endif /* __LINUX_MEMORY_HOTPLUG_H */ >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index 8605d7eb7f5c..86158eb9da70 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); >> >> static inline void flush_free_hpage_work(struct hstate *h) >> { >> + if (!hugetlb_optimize_vmemmap_enabled()) >> + return; >> + > > Hi Muchun, > > In v9 I was suggesting that we may be able to eliminate the static_branch_inc/dec from the vmemmap free/alloc paths. With this patch > I believe hugetlb_optimize_vmemmap_enabled() is really checking > 'has hugetlb vmemmap optimization been enabled' OR 'are there still vmemmap > optimized hugetlb pages in the system'. That may be confusing. > Sorry, I forgot about the use of hugetlb_optimize_vmemmap_enabled in page_fixed_fake_head. We need to know if there are any vmemmap optimized hugetlb pages in the system in this performance sensitive path. So, static_branch_inc/dec is indeed a good idea. Please disregard my attempt below at removing static_branch_inc/dec. I still find the name hugetlb_optimize_vmemmap_enabled a bit confusing as it tests two conditions (enabled and pages in use). You have already 'open coded' just the check for enabled in the routine hugetlb_vmemmap_free with: READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF How about having hugetlb_optimize_vmemmap_enabled() just check vmemmap_optimize_mode in a manner like above? Then rename hugetlb_optimize_vmemmap_enabled to something like: hugetlb_optimized_vmemmap_possible(). Sorry, I can think if a great name. -- Mike Kravetz ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl 2022-05-11 0:39 ` Mike Kravetz @ 2022-05-11 9:45 ` Muchun Song 2022-05-11 10:57 ` Muchun Song 0 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-11 9:45 UTC (permalink / raw) To: Mike Kravetz Cc: corbet, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On Tue, May 10, 2022 at 05:39:40PM -0700, Mike Kravetz wrote: > On 5/10/22 14:30, Mike Kravetz wrote: > > On 5/8/22 23:27, Muchun Song wrote: > >> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h > >> index 029fb7e26504..917112661b5c 100644 > >> --- a/include/linux/memory_hotplug.h > >> +++ b/include/linux/memory_hotplug.h > >> @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size); > >> extern bool mhp_supports_memmap_on_memory(unsigned long size); > >> #endif /* CONFIG_MEMORY_HOTPLUG */ > >> > >> +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY > >> +bool mhp_memmap_on_memory(void); > >> +#else > >> +static inline bool mhp_memmap_on_memory(void) > >> +{ > >> + return false; > >> +} > >> +#endif > >> + > >> #endif /* __LINUX_MEMORY_HOTPLUG_H */ > >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c > >> index 8605d7eb7f5c..86158eb9da70 100644 > >> --- a/mm/hugetlb.c > >> +++ b/mm/hugetlb.c > >> @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); > >> > >> static inline void flush_free_hpage_work(struct hstate *h) > >> { > >> + if (!hugetlb_optimize_vmemmap_enabled()) > >> + return; > >> + > > > > Hi Muchun, > > > > In v9 I was suggesting that we may be able to eliminate the static_branch_inc/dec from the vmemmap free/alloc paths. With this patch > > I believe hugetlb_optimize_vmemmap_enabled() is really checking > > 'has hugetlb vmemmap optimization been enabled' OR 'are there still vmemmap > > optimized hugetlb pages in the system'. That may be confusing. > > > > Sorry, I forgot about the use of hugetlb_optimize_vmemmap_enabled in > page_fixed_fake_head. We need to know if there are any vmemmap optimized > hugetlb pages in the system in this performance sensitive path. So, > static_branch_inc/dec is indeed a good idea. > Agree. > Please disregard my attempt below at removing static_branch_inc/dec. > > I still find the name hugetlb_optimize_vmemmap_enabled a bit confusing as > it tests two conditions (enabled and pages in use). > Right. It tests two conditions. > You have already 'open coded' just the check for enabled in the routine > hugetlb_vmemmap_free with: > > READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF > > How about having hugetlb_optimize_vmemmap_enabled() just check > vmemmap_optimize_mode in a manner like above? Then rename I'm wondering is it necessary to do this? vmemmap_optimize_mode is a internal state in hugetlb_vmemmap.c, at leaset now there is no outside users who care about this. Open-coding may be not an issue (I guess)? If one day someone cares it, maybe it it the time to do this and rename hugetlb_optimize_vmemmap_enabled()? I'm not against doing this, just expressing some of my thoughts. What do you think, Mike? > hugetlb_optimize_vmemmap_enabled to something like: > hugetlb_optimized_vmemmap_possible(). Sorry, I can think if a great name. > At least I cannot come up with an appropriate name. hugetlb_optimize_vmemmap_may_enabled()? It's not easy to come up with a good name. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl 2022-05-11 9:45 ` Muchun Song @ 2022-05-11 10:57 ` Muchun Song 2022-05-11 17:53 ` Mike Kravetz 0 siblings, 1 reply; 19+ messages in thread From: Muchun Song @ 2022-05-11 10:57 UTC (permalink / raw) To: Mike Kravetz Cc: corbet, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On Wed, May 11, 2022 at 05:45:57PM +0800, Muchun Song wrote: > On Tue, May 10, 2022 at 05:39:40PM -0700, Mike Kravetz wrote: > > On 5/10/22 14:30, Mike Kravetz wrote: > > > On 5/8/22 23:27, Muchun Song wrote: > > >> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h > > >> index 029fb7e26504..917112661b5c 100644 > > >> --- a/include/linux/memory_hotplug.h > > >> +++ b/include/linux/memory_hotplug.h > > >> @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size); > > >> extern bool mhp_supports_memmap_on_memory(unsigned long size); > > >> #endif /* CONFIG_MEMORY_HOTPLUG */ > > >> > > >> +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY > > >> +bool mhp_memmap_on_memory(void); > > >> +#else > > >> +static inline bool mhp_memmap_on_memory(void) > > >> +{ > > >> + return false; > > >> +} > > >> +#endif > > >> + > > >> #endif /* __LINUX_MEMORY_HOTPLUG_H */ > > >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > >> index 8605d7eb7f5c..86158eb9da70 100644 > > >> --- a/mm/hugetlb.c > > >> +++ b/mm/hugetlb.c > > >> @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); > > >> > > >> static inline void flush_free_hpage_work(struct hstate *h) > > >> { > > >> + if (!hugetlb_optimize_vmemmap_enabled()) > > >> + return; > > >> + > > > > > > Hi Muchun, > > > > > > In v9 I was suggesting that we may be able to eliminate the static_branch_inc/dec from the vmemmap free/alloc paths. With this patch > > > I believe hugetlb_optimize_vmemmap_enabled() is really checking > > > 'has hugetlb vmemmap optimization been enabled' OR 'are there still vmemmap > > > optimized hugetlb pages in the system'. That may be confusing. > > > > > > > Sorry, I forgot about the use of hugetlb_optimize_vmemmap_enabled in > > page_fixed_fake_head. We need to know if there are any vmemmap optimized > > hugetlb pages in the system in this performance sensitive path. So, > > static_branch_inc/dec is indeed a good idea. > > > > Agree. > > > Please disregard my attempt below at removing static_branch_inc/dec. > > > > I still find the name hugetlb_optimize_vmemmap_enabled a bit confusing as > > it tests two conditions (enabled and pages in use). > > > > Right. It tests two conditions. > > > You have already 'open coded' just the check for enabled in the routine > > hugetlb_vmemmap_free with: > > > > READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF > > > > How about having hugetlb_optimize_vmemmap_enabled() just check > > vmemmap_optimize_mode in a manner like above? Then rename > > I'm wondering is it necessary to do this? vmemmap_optimize_mode > is a internal state in hugetlb_vmemmap.c, at leaset now there is > no outside users who care about this. Open-coding may be not > an issue (I guess)? If one day someone cares it, maybe it it > the time to do this and rename hugetlb_optimize_vmemmap_enabled()? > I'm not against doing this, just expressing some of my thoughts. > What do you think, Mike? > > > hugetlb_optimize_vmemmap_enabled to something like: > > hugetlb_optimized_vmemmap_possible(). Sorry, I can think if a great name. > > > > At least I cannot come up with an appropriate name. > hugetlb_optimize_vmemmap_may_enabled()? It's not easy to come > up with a good name. > Instead of renaming, how about remove hugetlb_optimize_vmemmap_enabled() directly? I found there are only two places (mm/memory_hotplug.c and arch/arm64/mm/flush.c) except include/linux/page-flags.h where use this helper. In arch/arm64/mm/flush.c, we could replace it with if (PageHuge(page) && HPageVmemmapOptimized(compound_head(page))) In mm/memory_hotplug.c, I have a plan to remove it as well (I'll post them out after this patch merged). Finally, there is no outside users of it, we could remove it and squash it into page_fixed_fake_head(). What do you think this, Mike? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl 2022-05-11 10:57 ` Muchun Song @ 2022-05-11 17:53 ` Mike Kravetz 2022-05-12 3:34 ` Muchun Song 0 siblings, 1 reply; 19+ messages in thread From: Mike Kravetz @ 2022-05-11 17:53 UTC (permalink / raw) To: Muchun Song Cc: corbet, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On 5/11/22 03:57, Muchun Song wrote: > On Wed, May 11, 2022 at 05:45:57PM +0800, Muchun Song wrote: >> On Tue, May 10, 2022 at 05:39:40PM -0700, Mike Kravetz wrote: >>> On 5/10/22 14:30, Mike Kravetz wrote: >>>> On 5/8/22 23:27, Muchun Song wrote: >>>>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h >>>>> index 029fb7e26504..917112661b5c 100644 >>>>> --- a/include/linux/memory_hotplug.h >>>>> +++ b/include/linux/memory_hotplug.h >>>>> @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size); >>>>> extern bool mhp_supports_memmap_on_memory(unsigned long size); >>>>> #endif /* CONFIG_MEMORY_HOTPLUG */ >>>>> >>>>> +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY >>>>> +bool mhp_memmap_on_memory(void); >>>>> +#else >>>>> +static inline bool mhp_memmap_on_memory(void) >>>>> +{ >>>>> + return false; >>>>> +} >>>>> +#endif >>>>> + >>>>> #endif /* __LINUX_MEMORY_HOTPLUG_H */ >>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>>>> index 8605d7eb7f5c..86158eb9da70 100644 >>>>> --- a/mm/hugetlb.c >>>>> +++ b/mm/hugetlb.c >>>>> @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); >>>>> >>>>> static inline void flush_free_hpage_work(struct hstate *h) >>>>> { >>>>> + if (!hugetlb_optimize_vmemmap_enabled()) >>>>> + return; >>>>> + >>>> >>>> Hi Muchun, >>>> >>>> In v9 I was suggesting that we may be able to eliminate the static_branch_inc/dec from the vmemmap free/alloc paths. With this patch >>>> I believe hugetlb_optimize_vmemmap_enabled() is really checking >>>> 'has hugetlb vmemmap optimization been enabled' OR 'are there still vmemmap >>>> optimized hugetlb pages in the system'. That may be confusing. >>>> >>> >>> Sorry, I forgot about the use of hugetlb_optimize_vmemmap_enabled in >>> page_fixed_fake_head. We need to know if there are any vmemmap optimized >>> hugetlb pages in the system in this performance sensitive path. So, >>> static_branch_inc/dec is indeed a good idea. >>> >> >> Agree. >> >>> Please disregard my attempt below at removing static_branch_inc/dec. >>> >>> I still find the name hugetlb_optimize_vmemmap_enabled a bit confusing as >>> it tests two conditions (enabled and pages in use). >>> >> >> Right. It tests two conditions. >> >>> You have already 'open coded' just the check for enabled in the routine >>> hugetlb_vmemmap_free with: >>> >>> READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF >>> >>> How about having hugetlb_optimize_vmemmap_enabled() just check >>> vmemmap_optimize_mode in a manner like above? Then rename >> >> I'm wondering is it necessary to do this? vmemmap_optimize_mode >> is a internal state in hugetlb_vmemmap.c, at leaset now there is >> no outside users who care about this. Open-coding may be not >> an issue (I guess)? If one day someone cares it, maybe it it >> the time to do this and rename hugetlb_optimize_vmemmap_enabled()? >> I'm not against doing this, just expressing some of my thoughts. >> What do you think, Mike? >> >>> hugetlb_optimize_vmemmap_enabled to something like: >>> hugetlb_optimized_vmemmap_possible(). Sorry, I can think if a great name. >>> >> >> At least I cannot come up with an appropriate name. >> hugetlb_optimize_vmemmap_may_enabled()? It's not easy to come >> up with a good name. >> > > Instead of renaming, how about remove hugetlb_optimize_vmemmap_enabled() > directly? I found there are only two places (mm/memory_hotplug.c and > arch/arm64/mm/flush.c) except include/linux/page-flags.h where use this > helper. > > In arch/arm64/mm/flush.c, we could replace it with > > if (PageHuge(page) && HPageVmemmapOptimized(compound_head(page))) > > In mm/memory_hotplug.c, I have a plan to remove it as well (I'll > post them out after this patch merged). > > Finally, there is no outside users of it, we could remove it and squash > it into page_fixed_fake_head(). What do you think this, Mike? That sounds good. Sorry for all the questions/suggestions around hugetlb_optimize_vmemmap_enabled. It just caused me a little confusion as it is providing information on two conditions. I wanted to prevent it from causing confusion for others reading the code in the future. This patch as written is fine with plans for a follow up to remove hugetlb_optimize_vmemmap_enabled. Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> -- Mike Kravetz ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl 2022-05-11 17:53 ` Mike Kravetz @ 2022-05-12 3:34 ` Muchun Song 0 siblings, 0 replies; 19+ messages in thread From: Muchun Song @ 2022-05-12 3:34 UTC (permalink / raw) To: Mike Kravetz Cc: corbet, akpm, mcgrof, keescook, yzaikin, osalvador, david, masahiroy, linux-doc, linux-kernel, linux-mm, duanxiongchun, smuchun On Wed, May 11, 2022 at 10:53:07AM -0700, Mike Kravetz wrote: > On 5/11/22 03:57, Muchun Song wrote: > > On Wed, May 11, 2022 at 05:45:57PM +0800, Muchun Song wrote: > >> On Tue, May 10, 2022 at 05:39:40PM -0700, Mike Kravetz wrote: > >>> On 5/10/22 14:30, Mike Kravetz wrote: > >>>> On 5/8/22 23:27, Muchun Song wrote: > >>>>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h > >>>>> index 029fb7e26504..917112661b5c 100644 > >>>>> --- a/include/linux/memory_hotplug.h > >>>>> +++ b/include/linux/memory_hotplug.h > >>>>> @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size); > >>>>> extern bool mhp_supports_memmap_on_memory(unsigned long size); > >>>>> #endif /* CONFIG_MEMORY_HOTPLUG */ > >>>>> > >>>>> +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY > >>>>> +bool mhp_memmap_on_memory(void); > >>>>> +#else > >>>>> +static inline bool mhp_memmap_on_memory(void) > >>>>> +{ > >>>>> + return false; > >>>>> +} > >>>>> +#endif > >>>>> + > >>>>> #endif /* __LINUX_MEMORY_HOTPLUG_H */ > >>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c > >>>>> index 8605d7eb7f5c..86158eb9da70 100644 > >>>>> --- a/mm/hugetlb.c > >>>>> +++ b/mm/hugetlb.c > >>>>> @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn); > >>>>> > >>>>> static inline void flush_free_hpage_work(struct hstate *h) > >>>>> { > >>>>> + if (!hugetlb_optimize_vmemmap_enabled()) > >>>>> + return; > >>>>> + > >>>> > >>>> Hi Muchun, > >>>> > >>>> In v9 I was suggesting that we may be able to eliminate the static_branch_inc/dec from the vmemmap free/alloc paths. With this patch > >>>> I believe hugetlb_optimize_vmemmap_enabled() is really checking > >>>> 'has hugetlb vmemmap optimization been enabled' OR 'are there still vmemmap > >>>> optimized hugetlb pages in the system'. That may be confusing. > >>>> > >>> > >>> Sorry, I forgot about the use of hugetlb_optimize_vmemmap_enabled in > >>> page_fixed_fake_head. We need to know if there are any vmemmap optimized > >>> hugetlb pages in the system in this performance sensitive path. So, > >>> static_branch_inc/dec is indeed a good idea. > >>> > >> > >> Agree. > >> > >>> Please disregard my attempt below at removing static_branch_inc/dec. > >>> > >>> I still find the name hugetlb_optimize_vmemmap_enabled a bit confusing as > >>> it tests two conditions (enabled and pages in use). > >>> > >> > >> Right. It tests two conditions. > >> > >>> You have already 'open coded' just the check for enabled in the routine > >>> hugetlb_vmemmap_free with: > >>> > >>> READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF > >>> > >>> How about having hugetlb_optimize_vmemmap_enabled() just check > >>> vmemmap_optimize_mode in a manner like above? Then rename > >> > >> I'm wondering is it necessary to do this? vmemmap_optimize_mode > >> is a internal state in hugetlb_vmemmap.c, at leaset now there is > >> no outside users who care about this. Open-coding may be not > >> an issue (I guess)? If one day someone cares it, maybe it it > >> the time to do this and rename hugetlb_optimize_vmemmap_enabled()? > >> I'm not against doing this, just expressing some of my thoughts. > >> What do you think, Mike? > >> > >>> hugetlb_optimize_vmemmap_enabled to something like: > >>> hugetlb_optimized_vmemmap_possible(). Sorry, I can think if a great name. > >>> > >> > >> At least I cannot come up with an appropriate name. > >> hugetlb_optimize_vmemmap_may_enabled()? It's not easy to come > >> up with a good name. > >> > > > > Instead of renaming, how about remove hugetlb_optimize_vmemmap_enabled() > > directly? I found there are only two places (mm/memory_hotplug.c and > > arch/arm64/mm/flush.c) except include/linux/page-flags.h where use this > > helper. > > > > In arch/arm64/mm/flush.c, we could replace it with > > > > if (PageHuge(page) && HPageVmemmapOptimized(compound_head(page))) > > > > In mm/memory_hotplug.c, I have a plan to remove it as well (I'll > > post them out after this patch merged). > > > > Finally, there is no outside users of it, we could remove it and squash > > it into page_fixed_fake_head(). What do you think this, Mike? > > That sounds good. > > Sorry for all the questions/suggestions around > hugetlb_optimize_vmemmap_enabled. It just caused me a little confusion > as it is providing information on two conditions. I wanted to prevent it > from causing confusion for others reading the code in the future. > Sorry for the confusing. I'll post the subsequent patches ASAP. > This patch as written is fine with plans for a follow up to remove > hugetlb_optimize_vmemmap_enabled. > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> Thanks Mike. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-05-12 16:38 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-09 6:26 [PATCH v10 0/4] add hugetlb_optimize_vmemmap sysctl Muchun Song 2022-05-09 6:27 ` [PATCH v10 1/4] mm: hugetlb_vmemmap: disable hugetlb_optimize_vmemmap when struct page crosses page boundaries Muchun Song 2022-05-12 7:35 ` David Hildenbrand 2022-05-09 6:27 ` [PATCH v10 2/4] mm: memory_hotplug: override memmap_on_memory when hugetlb_free_vmemmap=on Muchun Song 2022-05-12 7:36 ` David Hildenbrand 2022-05-12 12:50 ` Muchun Song 2022-05-12 13:04 ` David Hildenbrand 2022-05-12 13:59 ` Muchun Song 2022-05-12 16:38 ` David Hildenbrand 2022-05-09 6:27 ` [PATCH v10 3/4] mm: hugetlb_vmemmap: use kstrtobool for hugetlb_vmemmap param parsing Muchun Song 2022-05-12 7:41 ` David Hildenbrand 2022-05-12 11:23 ` Muchun Song 2022-05-09 6:27 ` [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl Muchun Song 2022-05-10 21:30 ` Mike Kravetz 2022-05-11 0:39 ` Mike Kravetz 2022-05-11 9:45 ` Muchun Song 2022-05-11 10:57 ` Muchun Song 2022-05-11 17:53 ` Mike Kravetz 2022-05-12 3:34 ` Muchun Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.