Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <970166e0-f70e-dd2a-c764-af23a8425f87@oracle.com>
Date:   Tue, 10 May 2022 14:30:28 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap
 sysctl
Content-Language: en-US
To:     Muchun Song <songmuchun@bytedance.com>, corbet@lwn.net,
        akpm@linux-foundation.org, mcgrof@kernel.org,
        keescook@chromium.org, yzaikin@google.com, osalvador@suse.de,
        david@redhat.com, masahiroy@kernel.org
Cc:     linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, duanxiongchun@bytedance.com, smuchun@gmail.com
References: <20220509062703.64249-1-songmuchun@bytedance.com>
 <20220509062703.64249-5-songmuchun@bytedance.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <20220509062703.64249-5-songmuchun@bytedance.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VWd4NWpUcHIrM0pmQ1A3MmVtM0dVQTdINFBZSDF0cGVTdlpIOEtPMXA4NnFp?=
 =?utf-8?B?cy9leTM4UERmWUdMZ0duWEdQVitSN2wwV0ZYTERiTWRidWRFNTM4NlJYS0o3?=
 =?utf-8?B?OXF3djhySVJtMkpCK0o1ZWluaTRXWnJDcUFjbGVLZU4wZmZVTnAzdjc1My9J?=
 =?utf-8?B?elR5WWJxWlBJVG9vU3pVUGZ2c2cvcXJmOFlZS3J6N3o4NG15UUVZUytSaFNN?=
 =?utf-8?B?bVNodlBYQUoyRGR4RXlRNWZINkIya0JZM2Z3ZDlOV09ta1A5eG5vQmZPZldH?=
 =?utf-8?B?OHRjWjdxVVhUMy8yY3VTa2dPNkVVK01zMkJ3ai9wMjFhYzlPQUJFUVhVTFBV?=
 =?utf-8?B?REFJeHZUMnRnVUN1UFN5QkR0WkxFcWU3aFMrZVgyWTF5R0JiVE1SUmRONWRF?=
 =?utf-8?B?bzV4dS9OajlEbE11WGFmUTUzaWMxRnlIQndWcHFLdmJpNW5VcVA1Rmw4ZGxW?=
 =?utf-8?B?bUhlbThFdVdyMGFVSW9wMUhBSk1hMHRZeWpMS3c3OXRNakgwcTNPdncrdUpo?=
 =?utf-8?B?RTB0S0E0VW5zVUNFbEVMU1FSQjJjUjk2c09Ydk9SNEZIVnhnVXJPbnBiM2No?=
 =?utf-8?B?YzdONHVQYXpNRzVWdXNkQk5FUkdRNVkwLzJTbDRQcStoRVF6UjdNRCtWLy9z?=
 =?utf-8?B?eldHTXJYZFU2QVhQc2QydXZYRm50V2VSYktqaW1ObVEzME5VUDJTVjl2WjRK?=
 =?utf-8?B?WkpzWTF5NUdGRktvOVQ1d1JRczJEaUlzdFFxWnVySDZCMnFHLzZOd0o4VHJJ?=
 =?utf-8?B?Vk5jWTRWQmVmSHZVT25Na05YN2dOTk5SWXNmZ2EydDhuVWo5bitwYlpOZ25Q?=
 =?utf-8?B?ZGY2Mk5qeU1NRWE5U3JDcGhsVGFnVzZMRWc2ZWx3ZTZ4SzA5S1JoTWk4Q0Fw?=
 =?utf-8?B?V0dlZDdETktmdFVMVHBvWERLVlV4dmU2Y1FuRTI4RGNuMDNUa3R4WnJjbitI?=
 =?utf-8?B?UGN0RU9oaVY4MWNlREw4T2J3ajAvQUoxekd2Q3ZWc3JiM1BIdERUNlJxcFlT?=
 =?utf-8?B?Nzg0QUo0RlRrcUt3NURDWTlCbzBmeHA2NDJhT2ZaVWROaFlFZHdkS29uNGJG?=
 =?utf-8?B?ZklUL3BSS3VGY0hMVGF6VkcyMW5jT3NKSWZpNHB0Qk9jZW9WQnBQanlYNE56?=
 =?utf-8?B?eWhSRU1HU0pQV0ZWRXE1cjNBYXUvbWh6dmdZZHYwQnpha1F1d1pVSFVMMzdU?=
 =?utf-8?B?MDFSOCtqb2tMUnU2bVp3QTBaQVlpWGNKZm41Sk9uL1VEZFhoRmxSRjZRbktk?=
 =?utf-8?B?UFh0SWFaQ29tMWZiU2JGVVNKZU1aOWU4bHNNelVYejZ4Ym5Dc0FWRU9ZbzJ0?=
 =?utf-8?B?bmdrVzVGT0huNmQwck5Hdlloc1NFTGdHLzRvc01xcDlOVU9LRTJObGl4QjM5?=
 =?utf-8?B?b09OSWJZK3BPbEdsMUEzblFEemJsRUFSSDJpSnZoN1ZWK041b2hwYU9MbU5C?=
 =?utf-8?B?NkIzVHZ0YStPWjJ1VW1CVkFjK0NwZ0psNjA0dmloMllXMlVncWthWUR6ZnQz?=
 =?utf-8?B?bVV2cWw3U1VUdVhDck9lcHpTdmczL1NGR2REU0FWMVdOOHROb1Nld1NBRkY3?=
 =?utf-8?B?ODdudkJKbXpZbk5qWWlCcmw4SDdnK3BFZ0l1MVJmaDBMT0ZnVzF1VUdyUGxh?=
 =?utf-8?B?bmh3R2JDTWxrVks2S05xR1cxWmdmUTdydlQ4c0tRK21vUlBHNkNaVUxGcXk5?=
 =?utf-8?B?cHZUZVoyYmY2MkdtYmlTWDhHVDZyVXdmaGZxUVRiUDhOd0FHeTJSSVArMnVu?=
 =?utf-8?B?TlQwYWgvNDdTcm5OV05idVVrSjFMdnJ2Ny9xRUlFL1hYZFIvYWlwMXp0aE95?=
 =?utf-8?B?YWxzNWZxTHFDai9mQjJwaFA2YWFMRnU4b2FsWTYydzdBZStwVys3YnZTL2RM?=
 =?utf-8?B?VXFISDRBa2Eyd1U4Q0N4TnVWUmtGeVVLcG1HVHpoc0dSVFM3cXhzd1BPYUd2?=
 =?utf-8?B?VFV5cjlEanZtQmg5bVBSQlJEYWQxc3hxTXNZWFVuL0RRQjFjdjFmZjU1UTZt?=
 =?utf-8?B?M2x2VmFyQTlVek5vS0VpUXlNWXRadHdhdEtVdVpqeHNVYXcwbm5ydnZJeHhM?=
 =?utf-8?B?YTVQWk9FYW9tYjB1clVCSUJkS3RlSW1iZ0xVQmFDWUVqdmxnM1BrTXNtZDlO?=
 =?utf-8?B?bHZtdlVkSVZaRmFhMGFkY0ZIMnc1TTJlbWdQUDBRekpJeTBta3hCY3l3cXQw?=
 =?utf-8?B?c0tWNXZiL2pseGpqYVpkM2M4aG9aRmVYcXVzdk9kWXVWRUFrMzFIV0s4dklK?=
 =?utf-8?B?U2JONFlGd2o4TzY5Lyt0RHlKbzJMUHBSZ1dVTnVUY1piZy9TSzUyRGpSTVZR?=
 =?utf-8?B?aysrVUtnUHEvN2h2UmlYaW9jcE13enFPbDE4YXZ1S2x6TC9LcHU5YlBtRStW?=
 =?utf-8?Q?vU1S8RI6RTrem+So=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bc232cb9-dd1d-40d7-c209-08da32cc4f1a
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2022 21:30:30.9467
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mnug/eBnWyv9g6ViFNKque8Pddp42XT5FDtBAnJuQBKN0+iWvQHmlyGa3lqiDGp1hyl6mrX2c8C0RkHx4iTCIQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5020
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-10_07:2022-05-09,2022-05-10 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 malwarescore=0
 suspectscore=0 phishscore=0 spamscore=0 bulkscore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2205100088
X-Proofpoint-GUID: cg83EAWd4yl2vZ4StujmXSBz591BYKCc
X-Proofpoint-ORIG-GUID: cg83EAWd4yl2vZ4StujmXSBz591BYKCc
X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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