Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl

[Mike Kravetz <mike.kravetz@oracle.com>]

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