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.
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 <[email protected]>.
- 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 | 30 +++++++
include/linux/memory_hotplug.h | 9 ++
mm/hugetlb_vmemmap.c | 104 ++++++++++++++++++++----
mm/hugetlb_vmemmap.h | 4 +-
mm/memory_hotplug.c | 27 ++++--
6 files changed, 155 insertions(+), 25 deletions(-)
--
2.11.0
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 <[email protected]>
---
Documentation/admin-guide/sysctl/vm.rst | 30 +++++++++++
include/linux/memory_hotplug.h | 9 ++++
mm/hugetlb_vmemmap.c | 92 +++++++++++++++++++++++++++++----
mm/hugetlb_vmemmap.h | 4 +-
mm/memory_hotplug.c | 7 +--
5 files changed, 126 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/sysctl/vm.rst b/Documentation/admin-guide/sysctl/vm.rst
index 747e325ebcd0..00434789cf26 100644
--- a/Documentation/admin-guide/sysctl/vm.rst
+++ b/Documentation/admin-guide/sysctl/vm.rst
@@ -562,6 +562,36 @@ Change the minimum size of the hugepage pool.
See Documentation/admin-guide/mm/hugetlbpage.rst
+hugetlb_optimize_vmemmap
+========================
+
+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 is no optimized HugeTLB
+pages, you can set "nr_hugepages" to 0 first and then disable this.
+
+
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_vmemmap.c b/mm/hugetlb_vmemmap.c
index cc4ec752ec16..5820a681a724 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);
+ 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;
}
@@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
+ VM_BUG_ON_PAGE(!vmemmap_pages, head);
+
/*
* The pages which the vmemmap virtual address range [@vmemmap_addr,
* @vmemmap_end) are mapped to are freed to the buddy allocator, and
@@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
if (!vmemmap_pages)
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 +119,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 +138,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 +159,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/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h
index 109b0a53b6fe..19840aa900fd 100644
--- a/mm/hugetlb_vmemmap.h
+++ b/mm/hugetlb_vmemmap.h
@@ -21,7 +21,9 @@ void hugetlb_vmemmap_init(struct hstate *h);
*/
static inline unsigned int hugetlb_optimize_vmemmap_pages(struct hstate *h)
{
- return h->optimize_vmemmap_pages;
+ if (hugetlb_optimize_vmemmap_enabled())
+ return h->optimize_vmemmap_pages;
+ return 0;
}
#else
static inline int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
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
On Thu, May 05, 2022 at 09:48:34AM -0700, Mike Kravetz wrote:
> On 5/5/22 01:02, Muchun Song wrote:
> > On Wed, May 04, 2022 at 08:36:00PM -0700, Mike Kravetz wrote:
> >> On 5/4/22 19:35, Muchun Song wrote:
> >>> On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
> >>>> On 4/29/22 05:18, Muchun Song wrote:
> >>>>> +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);
> >>>>> + 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;
> >>>>> }
> >>>>> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
> >>>>> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
> >>>>> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
> >>>>>
> >>>>> + VM_BUG_ON_PAGE(!vmemmap_pages, head);
> >>>>> +
> >>>>> /*
> >>>>> * The pages which the vmemmap virtual address range [@vmemmap_addr,
> >>>>> * @vmemmap_end) are mapped to are freed to the buddy allocator, and
> >>>>> @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> >>>>> if (!vmemmap_pages)
> >>>>> return;
> >>>>>
> >>>>> + static_branch_inc(&hugetlb_optimize_vmemmap_key);
> >>>>
> >>>> Can you explain the reasoning behind doing the static_branch_inc here in free,
> >>>> and static_branch_dec in alloc?
> >>>> IIUC, they may not be absolutely necessary but you could use the count to
> >>>> know how many optimized pages are in use? Or, I may just be missing
> >>>> something.
> >>>>
> >>>
> >>> Partly right. One 'count' is not enough. I have implemented this with similar
> >>> approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
> >>> However, both count and synchronization are included in static_key_inc/dec
> >>> infrastructure. It is simpler to use static_key_inc/dec directly, right?
> >>>
> >>> [1] https://lore.kernel.org/all/[email protected]/
> >>>
> >>
> >> Sorry, but I am a little confused.
> >>
> >> vmemmap_optimize_mode_switch will static_key_inc to enable and static_key_dec
> >> to disable. In addition each time we optimize (allocate) a hugetlb page after
> >> enabling we will static_key_inc.
> >>
> >> Suppose we have 1 hugetlb page optimized. So static count == 2 IIUC.
> >> The someone turns off optimization via sysctl. static count == 1 ???
> >
> > Definitely right.
> >
> >> If we then add another hugetlb page via nr_hugepages it seems that it
> >> would be optimized as static count == 1. Is that correct? Do we need
> >
> > I'm wrong.
> >
> >> to free all hugetlb pages with optimization before we can add new pages
> >> without optimization?
> >>
> >
> > My bad. I think the following code would fix this.
> >
> > Thanks for your review carefully.
> >
> > diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> > index 5820a681a724..997e192aeed7 100644
> > --- a/mm/hugetlb_vmemmap.c
> > +++ b/mm/hugetlb_vmemmap.c
> > @@ -105,7 +105,7 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> > unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
> >
> > vmemmap_pages = hugetlb_optimize_vmemmap_pages(h);
> > - if (!vmemmap_pages)
> > + if (!vmemmap_pages || READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF)
> > return;
> >
> > static_branch_inc(&hugetlb_optimize_vmemmap_key);
> >
>
> If vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF is sufficient for turning
> off optimizations, do we really need to static_branch_inc/dev for each
> hugetlb page?
>
static_branch_inc/dec is necessary since the user could change
vmemmap_optimize_mode to off after the 'if' judgement.
CPU0: CPU1:
// Assume vmemmap_optimize_mode == 1
// and static_key_count == 1
if (vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF)
return;
hugetlb_optimize_vmemmap_handler();
vmemmap_optimize_mode = 0;
static_branch_dec();
// static_key_count == 0
// Enable static_key if necessary
static_branch_inc();
Does this make sense for you?
Thanks.
On Fri, May 06, 2022 at 09:50:53AM -0700, Mike Kravetz wrote:
> On 5/5/22 19:49, Muchun Song wrote:
> > On Thu, May 05, 2022 at 09:48:34AM -0700, Mike Kravetz wrote:
> >> On 5/5/22 01:02, Muchun Song wrote:
> >>> On Wed, May 04, 2022 at 08:36:00PM -0700, Mike Kravetz wrote:
> >>>> On 5/4/22 19:35, Muchun Song wrote:
> >>>>> On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
> >>>>>> On 4/29/22 05:18, Muchun Song wrote:
> >>>>>>> +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);
> >>>>>>> + 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;
> >>>>>>> }
> >>>>>>> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
> >>>>>>> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
> >>>>>>> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
> >>>>>>>
> >>>>>>> + VM_BUG_ON_PAGE(!vmemmap_pages, head);
> >>>>>>> +
> >>>>>>> /*
> >>>>>>> * The pages which the vmemmap virtual address range [@vmemmap_addr,
> >>>>>>> * @vmemmap_end) are mapped to are freed to the buddy allocator, and
> >>>>>>> @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> >>>>>>> if (!vmemmap_pages)
> >>>>>>> return;
> >>>>>>>
> >>>>>>> + static_branch_inc(&hugetlb_optimize_vmemmap_key);
> >>>>>>
> >>>>>> Can you explain the reasoning behind doing the static_branch_inc here in free,
> >>>>>> and static_branch_dec in alloc?
> >>>>>> IIUC, they may not be absolutely necessary but you could use the count to
> >>>>>> know how many optimized pages are in use? Or, I may just be missing
> >>>>>> something.
> >>>>>>
> >>>>>
> >>>>> Partly right. One 'count' is not enough. I have implemented this with similar
> >>>>> approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
> >>>>> However, both count and synchronization are included in static_key_inc/dec
> >>>>> infrastructure. It is simpler to use static_key_inc/dec directly, right?
> >>>>>
> >>>>> [1] https://lore.kernel.org/all/[email protected]/
> >>>>>
> >>>>
> >>>> Sorry, but I am a little confused.
> >>>>
> >>>> vmemmap_optimize_mode_switch will static_key_inc to enable and static_key_dec
> >>>> to disable. In addition each time we optimize (allocate) a hugetlb page after
> >>>> enabling we will static_key_inc.
> >>>>
> >>>> Suppose we have 1 hugetlb page optimized. So static count == 2 IIUC.
> >>>> The someone turns off optimization via sysctl. static count == 1 ???
> >>>
> >>> Definitely right.
> >>>
> >>>> If we then add another hugetlb page via nr_hugepages it seems that it
> >>>> would be optimized as static count == 1. Is that correct? Do we need
> >>>
> >>> I'm wrong.
> >>>
> >>>> to free all hugetlb pages with optimization before we can add new pages
> >>>> without optimization?
> >>>>
> >>>
> >>> My bad. I think the following code would fix this.
> >>>
> >>> Thanks for your review carefully.
> >>>
> >>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> >>> index 5820a681a724..997e192aeed7 100644
> >>> --- a/mm/hugetlb_vmemmap.c
> >>> +++ b/mm/hugetlb_vmemmap.c
> >>> @@ -105,7 +105,7 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> >>> unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
> >>>
> >>> vmemmap_pages = hugetlb_optimize_vmemmap_pages(h);
> >>> - if (!vmemmap_pages)
> >>> + if (!vmemmap_pages || READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF)
> >>> return;
> >>>
> >>> static_branch_inc(&hugetlb_optimize_vmemmap_key);
> >>>
> >>
> >> If vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF is sufficient for turning
> >> off optimizations, do we really need to static_branch_inc/dev for each
> >> hugetlb page?
> >>
> >
> > static_branch_inc/dec is necessary since the user could change
> > vmemmap_optimize_mode to off after the 'if' judgement.
> >
> > CPU0: CPU1:
> > // Assume vmemmap_optimize_mode == 1
> > // and static_key_count == 1
> > if (vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF)
> > return;
> > hugetlb_optimize_vmemmap_handler();
> > vmemmap_optimize_mode = 0;
> > static_branch_dec();
> > // static_key_count == 0
> > // Enable static_key if necessary
> > static_branch_inc();
> >
> > Does this make sense for you?
>
> Yes, it makes sense and is require because hugetlb_optimize_vmemmap_pages()
> performs two functions:
> 1) It determines if vmemmap_optimization is enabled
> 2) It specifies how many vmemmap pages can be saved with optimization
> hugetlb_optimize_vmemmap_pages returns 0 if static_key_count == 0, so this
> would cause problems in places such as hugetlb free path (hugetlb_vmemmap_alloc). I hope my understanding is correct?
>
Right.
> Would it make the code more clear if we did not do the check for
> vmemmap_optimization in hugetlb_optimize_vmemmap_pages()? Instead:
> - hugetlb_optimize_vmemmap_pages ALWAYS returns the number of vmemmap pages
> that can be freed/optimized
> - At hugetlb allocation time (hugetlb_vmemmap_free) we only check
> hugetlb_optimize_vmemmap_enabled() to determine if optimization should
> be performed.
> - After hugetlb_vmemmap_free, we can use HPageVmemmapOptimized to determine
> if vmemap pages need to be allocated in hugetlb freeing paths.
>
I think this works as well. My initial consideration was that
embedding hugetlb_optimize_vmemmap_enabled() in
hugetlb_optimize_vmemmap_pages() could make the caller (e.g.
flush_free_hpage_work()) of hugetlb_optimize_vmemmap_pages()
more efficient when static_key == 0. Maybe I could add
the check for vmemmap_optimization to flush_free_hpage_work()
and then remove the check from hugetlb_optimize_vmemmap_pages().
Will do this in a new version.
Thanks.
> Perhaps, there is something wrong with the above suggestion?
>
> I know you have always had hugetlb_optimize_vmemmap_pages perform the two
> functions. So, splitting functionality may not be more clear for you. I am
> OK leaving code as is (key inc/dec for each page). Just wanted to get your
> (and perhaps other) thoughts on splitting functionality as described above.
> --
> Mike Kravetz
>
On Wed, May 04, 2022 at 08:36:00PM -0700, Mike Kravetz wrote:
> On 5/4/22 19:35, Muchun Song wrote:
> > On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
> >> On 4/29/22 05:18, Muchun Song wrote:
> >>> +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);
> >>> + 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;
> >>> }
> >>> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
> >>> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
> >>> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
> >>>
> >>> + VM_BUG_ON_PAGE(!vmemmap_pages, head);
> >>> +
> >>> /*
> >>> * The pages which the vmemmap virtual address range [@vmemmap_addr,
> >>> * @vmemmap_end) are mapped to are freed to the buddy allocator, and
> >>> @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> >>> if (!vmemmap_pages)
> >>> return;
> >>>
> >>> + static_branch_inc(&hugetlb_optimize_vmemmap_key);
> >>
> >> Can you explain the reasoning behind doing the static_branch_inc here in free,
> >> and static_branch_dec in alloc?
> >> IIUC, they may not be absolutely necessary but you could use the count to
> >> know how many optimized pages are in use? Or, I may just be missing
> >> something.
> >>
> >
> > Partly right. One 'count' is not enough. I have implemented this with similar
> > approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
> > However, both count and synchronization are included in static_key_inc/dec
> > infrastructure. It is simpler to use static_key_inc/dec directly, right?
> >
> > [1] https://lore.kernel.org/all/[email protected]/
> >
>
> Sorry, but I am a little confused.
>
> vmemmap_optimize_mode_switch will static_key_inc to enable and static_key_dec
> to disable. In addition each time we optimize (allocate) a hugetlb page after
> enabling we will static_key_inc.
>
> Suppose we have 1 hugetlb page optimized. So static count == 2 IIUC.
> The someone turns off optimization via sysctl. static count == 1 ???
Definitely right.
> If we then add another hugetlb page via nr_hugepages it seems that it
> would be optimized as static count == 1. Is that correct? Do we need
I'm wrong.
> to free all hugetlb pages with optimization before we can add new pages
> without optimization?
>
My bad. I think the following code would fix this.
Thanks for your review carefully.
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 5820a681a724..997e192aeed7 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -105,7 +105,7 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
vmemmap_pages = hugetlb_optimize_vmemmap_pages(h);
- if (!vmemmap_pages)
+ if (!vmemmap_pages || READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF)
return;
static_branch_inc(&hugetlb_optimize_vmemmap_key);
On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
> On 4/29/22 05:18, Muchun Song wrote:
> > 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 <[email protected]>
> > ---
> > Documentation/admin-guide/sysctl/vm.rst | 30 +++++++++++
> > include/linux/memory_hotplug.h | 9 ++++
> > mm/hugetlb_vmemmap.c | 92 +++++++++++++++++++++++++++++----
> > mm/hugetlb_vmemmap.h | 4 +-
> > mm/memory_hotplug.c | 7 +--
> > 5 files changed, 126 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/sysctl/vm.rst b/Documentation/admin-guide/sysctl/vm.rst
> > index 747e325ebcd0..00434789cf26 100644
> > --- a/Documentation/admin-guide/sysctl/vm.rst
> > +++ b/Documentation/admin-guide/sysctl/vm.rst
> > @@ -562,6 +562,36 @@ Change the minimum size of the hugepage pool.
> > See Documentation/admin-guide/mm/hugetlbpage.rst
> >
> >
> > +hugetlb_optimize_vmemmap
> > +========================
> > +
> > +Enable (set to 1) or disable (set to 0) the feature of optimizing vmemmap pages
> > +associated with each HugeTLB page.
>
> Should we mention that "memory_hotplug.memmap_on_memory" or an unusual system
> config that results in struct page not being a power of 2 will prevent
> enabling?
>
Good question. I think maybe it is better to clarify that this knob exists when
memory_hotplug.memmap_on_memory (kernel parameter) is disabled and the size of
"struct page" must be power of 2 (an unusual system config that could results
in this).
> > +
> > +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 is no optimized HugeTLB
> > +pages, you can set "nr_hugepages" to 0 first and then disable this.
>
> Thank you for adding this documentation.
>
> We may want to clarify that last statement about making sure there are no
> optimized HugeTLB pages. 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.
>
I didn't realize this, I'll add this into documentation, Thanks.
> > +
> > +
> > 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_vmemmap.c b/mm/hugetlb_vmemmap.c
> > index cc4ec752ec16..5820a681a724 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);
> > + 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;
> > }
> > @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
> > vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
> > vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
> >
> > + VM_BUG_ON_PAGE(!vmemmap_pages, head);
> > +
> > /*
> > * The pages which the vmemmap virtual address range [@vmemmap_addr,
> > * @vmemmap_end) are mapped to are freed to the buddy allocator, and
> > @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> > if (!vmemmap_pages)
> > return;
> >
> > + static_branch_inc(&hugetlb_optimize_vmemmap_key);
>
> Can you explain the reasoning behind doing the static_branch_inc here in free,
> and static_branch_dec in alloc?
> IIUC, they may not be absolutely necessary but you could use the count to
> know how many optimized pages are in use? Or, I may just be missing
> something.
>
Partly right. One 'count' is not enough. I have implemented this with similar
approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
However, both count and synchronization are included in static_key_inc/dec
infrastructure. It is simpler to use static_key_inc/dec directly, right?
[1] https://lore.kernel.org/all/[email protected]/
Thanks.
On 5/5/22 19:49, Muchun Song wrote:
> On Thu, May 05, 2022 at 09:48:34AM -0700, Mike Kravetz wrote:
>> On 5/5/22 01:02, Muchun Song wrote:
>>> On Wed, May 04, 2022 at 08:36:00PM -0700, Mike Kravetz wrote:
>>>> On 5/4/22 19:35, Muchun Song wrote:
>>>>> On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
>>>>>> On 4/29/22 05:18, Muchun Song wrote:
>>>>>>> +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);
>>>>>>> + 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;
>>>>>>> }
>>>>>>> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
>>>>>>> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
>>>>>>> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
>>>>>>>
>>>>>>> + VM_BUG_ON_PAGE(!vmemmap_pages, head);
>>>>>>> +
>>>>>>> /*
>>>>>>> * The pages which the vmemmap virtual address range [@vmemmap_addr,
>>>>>>> * @vmemmap_end) are mapped to are freed to the buddy allocator, and
>>>>>>> @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
>>>>>>> if (!vmemmap_pages)
>>>>>>> return;
>>>>>>>
>>>>>>> + static_branch_inc(&hugetlb_optimize_vmemmap_key);
>>>>>>
>>>>>> Can you explain the reasoning behind doing the static_branch_inc here in free,
>>>>>> and static_branch_dec in alloc?
>>>>>> IIUC, they may not be absolutely necessary but you could use the count to
>>>>>> know how many optimized pages are in use? Or, I may just be missing
>>>>>> something.
>>>>>>
>>>>>
>>>>> Partly right. One 'count' is not enough. I have implemented this with similar
>>>>> approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
>>>>> However, both count and synchronization are included in static_key_inc/dec
>>>>> infrastructure. It is simpler to use static_key_inc/dec directly, right?
>>>>>
>>>>> [1] https://lore.kernel.org/all/[email protected]/
>>>>>
>>>>
>>>> Sorry, but I am a little confused.
>>>>
>>>> vmemmap_optimize_mode_switch will static_key_inc to enable and static_key_dec
>>>> to disable. In addition each time we optimize (allocate) a hugetlb page after
>>>> enabling we will static_key_inc.
>>>>
>>>> Suppose we have 1 hugetlb page optimized. So static count == 2 IIUC.
>>>> The someone turns off optimization via sysctl. static count == 1 ???
>>>
>>> Definitely right.
>>>
>>>> If we then add another hugetlb page via nr_hugepages it seems that it
>>>> would be optimized as static count == 1. Is that correct? Do we need
>>>
>>> I'm wrong.
>>>
>>>> to free all hugetlb pages with optimization before we can add new pages
>>>> without optimization?
>>>>
>>>
>>> My bad. I think the following code would fix this.
>>>
>>> Thanks for your review carefully.
>>>
>>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>>> index 5820a681a724..997e192aeed7 100644
>>> --- a/mm/hugetlb_vmemmap.c
>>> +++ b/mm/hugetlb_vmemmap.c
>>> @@ -105,7 +105,7 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
>>> unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
>>>
>>> vmemmap_pages = hugetlb_optimize_vmemmap_pages(h);
>>> - if (!vmemmap_pages)
>>> + if (!vmemmap_pages || READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF)
>>> return;
>>>
>>> static_branch_inc(&hugetlb_optimize_vmemmap_key);
>>>
>>
>> If vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF is sufficient for turning
>> off optimizations, do we really need to static_branch_inc/dev for each
>> hugetlb page?
>>
>
> static_branch_inc/dec is necessary since the user could change
> vmemmap_optimize_mode to off after the 'if' judgement.
>
> CPU0: CPU1:
> // Assume vmemmap_optimize_mode == 1
> // and static_key_count == 1
> if (vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF)
> return;
> hugetlb_optimize_vmemmap_handler();
> vmemmap_optimize_mode = 0;
> static_branch_dec();
> // static_key_count == 0
> // Enable static_key if necessary
> static_branch_inc();
>
> Does this make sense for you?
Yes, it makes sense and is require because hugetlb_optimize_vmemmap_pages()
performs two functions:
1) It determines if vmemmap_optimization is enabled
2) It specifies how many vmemmap pages can be saved with optimization
hugetlb_optimize_vmemmap_pages returns 0 if static_key_count == 0, so this
would cause problems in places such as hugetlb free path (hugetlb_vmemmap_alloc). I hope my understanding is correct?
Would it make the code more clear if we did not do the check for
vmemmap_optimization in hugetlb_optimize_vmemmap_pages()? Instead:
- hugetlb_optimize_vmemmap_pages ALWAYS returns the number of vmemmap pages
that can be freed/optimized
- At hugetlb allocation time (hugetlb_vmemmap_free) we only check
hugetlb_optimize_vmemmap_enabled() to determine if optimization should
be performed.
- After hugetlb_vmemmap_free, we can use HPageVmemmapOptimized to determine
if vmemap pages need to be allocated in hugetlb freeing paths.
Perhaps, there is something wrong with the above suggestion?
I know you have always had hugetlb_optimize_vmemmap_pages perform the two
functions. So, splitting functionality may not be more clear for you. I am
OK leaving code as is (key inc/dec for each page). Just wanted to get your
(and perhaps other) thoughts on splitting functionality as described above.
--
Mike Kravetz
On 5/4/22 19:35, Muchun Song wrote:
> On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
>> On 4/29/22 05:18, Muchun Song wrote:
>>> +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);
>>> + 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;
>>> }
>>> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
>>> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
>>> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
>>>
>>> + VM_BUG_ON_PAGE(!vmemmap_pages, head);
>>> +
>>> /*
>>> * The pages which the vmemmap virtual address range [@vmemmap_addr,
>>> * @vmemmap_end) are mapped to are freed to the buddy allocator, and
>>> @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
>>> if (!vmemmap_pages)
>>> return;
>>>
>>> + static_branch_inc(&hugetlb_optimize_vmemmap_key);
>>
>> Can you explain the reasoning behind doing the static_branch_inc here in free,
>> and static_branch_dec in alloc?
>> IIUC, they may not be absolutely necessary but you could use the count to
>> know how many optimized pages are in use? Or, I may just be missing
>> something.
>>
>
> Partly right. One 'count' is not enough. I have implemented this with similar
> approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
> However, both count and synchronization are included in static_key_inc/dec
> infrastructure. It is simpler to use static_key_inc/dec directly, right?
>
> [1] https://lore.kernel.org/all/[email protected]/
>
Sorry, but I am a little confused.
vmemmap_optimize_mode_switch will static_key_inc to enable and static_key_dec
to disable. In addition each time we optimize (allocate) a hugetlb page after
enabling we will static_key_inc.
Suppose we have 1 hugetlb page optimized. So static count == 2 IIUC.
The someone turns off optimization via sysctl. static count == 1 ???
If we then add another hugetlb page via nr_hugepages it seems that it
would be optimized as static count == 1. Is that correct? Do we need
to free all hugetlb pages with optimization before we can add new pages
without optimization?
--
Mike Kravetz
On 5/5/22 01:02, Muchun Song wrote:
> On Wed, May 04, 2022 at 08:36:00PM -0700, Mike Kravetz wrote:
>> On 5/4/22 19:35, Muchun Song wrote:
>>> On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
>>>> On 4/29/22 05:18, Muchun Song wrote:
>>>>> +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);
>>>>> + 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;
>>>>> }
>>>>> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
>>>>> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
>>>>> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
>>>>>
>>>>> + VM_BUG_ON_PAGE(!vmemmap_pages, head);
>>>>> +
>>>>> /*
>>>>> * The pages which the vmemmap virtual address range [@vmemmap_addr,
>>>>> * @vmemmap_end) are mapped to are freed to the buddy allocator, and
>>>>> @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
>>>>> if (!vmemmap_pages)
>>>>> return;
>>>>>
>>>>> + static_branch_inc(&hugetlb_optimize_vmemmap_key);
>>>>
>>>> Can you explain the reasoning behind doing the static_branch_inc here in free,
>>>> and static_branch_dec in alloc?
>>>> IIUC, they may not be absolutely necessary but you could use the count to
>>>> know how many optimized pages are in use? Or, I may just be missing
>>>> something.
>>>>
>>>
>>> Partly right. One 'count' is not enough. I have implemented this with similar
>>> approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
>>> However, both count and synchronization are included in static_key_inc/dec
>>> infrastructure. It is simpler to use static_key_inc/dec directly, right?
>>>
>>> [1] https://lore.kernel.org/all/[email protected]/
>>>
>>
>> Sorry, but I am a little confused.
>>
>> vmemmap_optimize_mode_switch will static_key_inc to enable and static_key_dec
>> to disable. In addition each time we optimize (allocate) a hugetlb page after
>> enabling we will static_key_inc.
>>
>> Suppose we have 1 hugetlb page optimized. So static count == 2 IIUC.
>> The someone turns off optimization via sysctl. static count == 1 ???
>
> Definitely right.
>
>> If we then add another hugetlb page via nr_hugepages it seems that it
>> would be optimized as static count == 1. Is that correct? Do we need
>
> I'm wrong.
>
>> to free all hugetlb pages with optimization before we can add new pages
>> without optimization?
>>
>
> My bad. I think the following code would fix this.
>
> Thanks for your review carefully.
>
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index 5820a681a724..997e192aeed7 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -105,7 +105,7 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
>
> vmemmap_pages = hugetlb_optimize_vmemmap_pages(h);
> - if (!vmemmap_pages)
> + if (!vmemmap_pages || READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF)
> return;
>
> static_branch_inc(&hugetlb_optimize_vmemmap_key);
>
If vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF is sufficient for turning
off optimizations, do we really need to static_branch_inc/dev for each
hugetlb page?
--
Mike Kravetz
On 4/29/22 05:18, Muchun Song wrote:
> 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 <[email protected]>
> ---
> Documentation/admin-guide/sysctl/vm.rst | 30 +++++++++++
> include/linux/memory_hotplug.h | 9 ++++
> mm/hugetlb_vmemmap.c | 92 +++++++++++++++++++++++++++++----
> mm/hugetlb_vmemmap.h | 4 +-
> mm/memory_hotplug.c | 7 +--
> 5 files changed, 126 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/admin-guide/sysctl/vm.rst b/Documentation/admin-guide/sysctl/vm.rst
> index 747e325ebcd0..00434789cf26 100644
> --- a/Documentation/admin-guide/sysctl/vm.rst
> +++ b/Documentation/admin-guide/sysctl/vm.rst
> @@ -562,6 +562,36 @@ Change the minimum size of the hugepage pool.
> See Documentation/admin-guide/mm/hugetlbpage.rst
>
>
> +hugetlb_optimize_vmemmap
> +========================
> +
> +Enable (set to 1) or disable (set to 0) the feature of optimizing vmemmap pages
> +associated with each HugeTLB page.
Should we mention that "memory_hotplug.memmap_on_memory" or an unusual system
config that results in struct page not being a power of 2 will prevent
enabling?
> +
> +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 is no optimized HugeTLB
> +pages, you can set "nr_hugepages" to 0 first and then disable this.
Thank you for adding this documentation.
We may want to clarify that last statement about making sure there are no
optimized HugeTLB pages. 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_vmemmap.c b/mm/hugetlb_vmemmap.c
> index cc4ec752ec16..5820a681a724 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);
> + 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;
> }
> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
>
> + VM_BUG_ON_PAGE(!vmemmap_pages, head);
> +
> /*
> * The pages which the vmemmap virtual address range [@vmemmap_addr,
> * @vmemmap_end) are mapped to are freed to the buddy allocator, and
> @@ -69,8 +91,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 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
> if (!vmemmap_pages)
> return;
>
> + static_branch_inc(&hugetlb_optimize_vmemmap_key);
Can you explain the reasoning behind doing the static_branch_inc here in free,
and static_branch_dec in alloc?
IIUC, they may not be absolutely necessary but you could use the count to
know how many optimized pages are in use? Or, I may just be missing
something.
--
Mike Kravetz
> +
> vmemmap_addr += RESERVE_VMEMMAP_SIZE;
> vmemmap_end = vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
> vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
> @@ -93,7 +119,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 +138,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 +159,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/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h
> index 109b0a53b6fe..19840aa900fd 100644
> --- a/mm/hugetlb_vmemmap.h
> +++ b/mm/hugetlb_vmemmap.h
> @@ -21,7 +21,9 @@ void hugetlb_vmemmap_init(struct hstate *h);
> */
> static inline unsigned int hugetlb_optimize_vmemmap_pages(struct hstate *h)
> {
> - return h->optimize_vmemmap_pages;
> + if (hugetlb_optimize_vmemmap_enabled())
> + return h->optimize_vmemmap_pages;
> + return 0;
> }
> #else
> static inline int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
> 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 {