| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 May 2022 14:30:28 -0700
From: Mike Kravetz <mike.kravetz@...cle.com>
To: Muchun Song <songmuchun@...edance.com>, corbet@....net,
akpm@...ux-foundation.org, mcgrof@...nel.org,
keescook@...omium.org, yzaikin@...gle.com, osalvador@...e.de,
david@...hat.com, masahiroy@...nel.org
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, duanxiongchun@...edance.com, smuchun@...il.com
Subject: Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap
sysctl
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
Powered by blists - more mailing lists