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: <3d040faf-7fc1-80a6-c584-aafeff27af18@oracle.com>
Date:   Tue, 3 May 2022 17:23:37 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v9 2/4] mm: memory_hotplug: override memmap_on_memory when
 hugetlb_free_vmemmap=on
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: <20220429121816.37541-1-songmuchun@bytedance.com>
 <20220429121816.37541-3-songmuchun@bytedance.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <20220429121816.37541-3-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?VkJvajdXWE80V0tUM0xRS2ZoOFd3cTN5dzlSS2dOdmxLdmpWNDkzODJWUEdk?=
 =?utf-8?B?N1R0K0NFN3NSNThzT1k3YkxqVW9kM3llcGlhRmprWUg1Z3A5MUxmTXMvakp5?=
 =?utf-8?B?TWZ4TEpZelZtQTN4YmJ1MEtSc0c1R1UrclEyTjRBWjJyeEoxUnhiZmc3OWcv?=
 =?utf-8?B?VFFXMkFPLzlFTmtRMEE3MUxZK1RaeUc0QjlPSFNsTlQvNlZEUmhrV3g0a1Rm?=
 =?utf-8?B?QUs3L3FDcEgxNnI5TlF2QmF2bVZzSUhuNGErS2JUekZCdm5xcjNwZDN2aFRk?=
 =?utf-8?B?OVliNHlyWDdOdk14UnorKy9MNWJaYkJYS3NlWGxZWnhBZForVmE1VTg2dUFi?=
 =?utf-8?B?QWFMb1pIY21GSnpVZXMvYTVIeUJtcndWQnVpbXNMcUoxRkZncTJJTUloeDN3?=
 =?utf-8?B?RGxFMUkzWXFrR0pWaXdjUnZYL2dzdDFpK3R2OTMxRXpUUlZWZEY1NXliR0sv?=
 =?utf-8?B?Y1BCeVM2Q21kWHlmN09UMWEvRTFiblF2V2Z1bjh6SGdwRW52b0xKZXkvTU42?=
 =?utf-8?B?WHRrTG05YWppMVU4Vmt0RCt2T1BDdlZMOXNrWVVEcjkzMTdBN0dtQ0NHZzVm?=
 =?utf-8?B?V283bE1XSXpTb2VMK3JiajFzS0Q1T0VaZHo3aHE1bndwY3BJeFhZVVVQeVl0?=
 =?utf-8?B?QmxjaitHeEVzRTBQR2dFOENkNTZrVE1TS1BPYUU2Y093L09KZzNlSVJGMTRC?=
 =?utf-8?B?YVJrem9IbElNYkM3SnpiNDgrRmZvbWkvSHNWdzc3NHQ4QkM0dzdLK0NhQlhM?=
 =?utf-8?B?N0Z5UFV0RllqMVN4OUY1OC9ZY1gzNWJVSmRsR2ZReVM4TzgveFkxSjFweTlN?=
 =?utf-8?B?SERwelNsQTRtK3NOczY3Z3Y1cXdaU2prVW5UYU9MaGdjSThXeTB1ZEo4Z2Vu?=
 =?utf-8?B?Y29GREtGMEJvaWxDRkZ6SGwxd3FnY2JIRzZWSHpGYlRDbHNkaTdHcmVEZnJs?=
 =?utf-8?B?Z01wcTAxVFZSWlJEZGw5OGg1TXlsVFVPelkxSTI1TFI5K2hxYm9Ba3VmTHdx?=
 =?utf-8?B?QURiVGVWeTgyd2ppY0VWSzVhQ2JHNENiem5ZMVhTMS9OYXNaSDAwc0xZV2RS?=
 =?utf-8?B?RWZsWlIxUmdOQ25ybHF0UGRKdVdYeUl0UkozU0c0WU44VGVwUHFJZHRlc0VX?=
 =?utf-8?B?S0k3WUVDTnQ5SFN4NzdCbndyTFpYd0NoSU1aSnJvMUl1WnlmbU9rdWpOVmVt?=
 =?utf-8?B?ZWdkT3l2WDMvMm44MEM1K1RLSmRKVy93NUFvNk55ZVUxb2x1U3dBWUlTWnAz?=
 =?utf-8?B?OFBBODNLSEdieFprMktDdFFXbWllOGtUVzAvOUFETEZ0Y0YraitGVnlhaGor?=
 =?utf-8?B?MWsvcXkwWG9jYTA5dWwwaGM1aVE5Q1hVM09BSkd4T3o5blZoai9WZ2JIelcz?=
 =?utf-8?B?Y0J6THRsaGdkYmlqZzFYWmNTNVQweVZ2OHkzVTRyM2tWblRkNkJXSThwd3VB?=
 =?utf-8?B?cmhqcFNaQTVaU3pnV1ZrTCs1RGhBc0xhVFNGeHJmZlhDVXZnMk4yYzkvZUJB?=
 =?utf-8?B?aXJaVmRVdEM3Q2FvQ1hFT3dhbUdvaTd3VnppSlFIRXFudXE4VENLOUtPMkVT?=
 =?utf-8?B?V2JXdlN2YVdlOTV1L1BqbVpOMHZmK3RjNy8yNTFJU3hNeUl3c21ONlM0RDBF?=
 =?utf-8?B?aUxNUzB0UE5lM1ZKQjJRQkN2TERIb0cydW5TdHp3OUFKby9aUWxsTkdlY0Z6?=
 =?utf-8?B?VlRzZDJiQzkveGEyQUw5a1hZWVdwMStBbmhML3ZvZW9FMElMSFN2T1hHK2Fs?=
 =?utf-8?B?ekFncTcybTZwVTFXdlBOU3B2NnV5QlJUVncrbEg2L0o3eGw1RVRORG14amla?=
 =?utf-8?B?RDFERkZHNFJFSDFycFdOeTdXbTRhbXNDMnRqRnV4REp6aG1YZzhNR1NZL2hu?=
 =?utf-8?B?RUZWTlVadnlpTDRlc0EySXdBbTFKMmlpVzZlVjFpbFc0RVdhUEdKM2E0R3NK?=
 =?utf-8?B?VkprR1UvZi95RnBKZ0tQUkVCUlpNK3JvTE81S0hqSTZMTWVBTHlSMFlmOHZ0?=
 =?utf-8?B?Q1N0Wjlta3ZzWVc4MWlnOVpKTHd2a2FTOEFialpjWS91QzRXYVNveUUyTnFj?=
 =?utf-8?B?bFRmbHVYeGx6R3RnQTFzWXNpNGdXNk5pSVNOUG1QNmpFc0hRYVhIYnZ3cjhR?=
 =?utf-8?B?QlFvK0g2MmpmRmk4cUpaemdGQU9NSytMSUJTMjB3WVpTVEF4TVhycnhDNTJ2?=
 =?utf-8?B?UDNnZWdZK0JzYVdVMnE5a2lueEttZ1hscU1Gd0NINlBmUVN0KzVrTlYycGc4?=
 =?utf-8?B?aldUdlhaa0VTQlJUYXhJc1BCRjZYTGE5RUF2b1B4QnBITnhBaldQVmlvVy9X?=
 =?utf-8?B?NjVuS3F6eEFrWlZFMnFwNW9naHIwQk8yRkR2bForVWRmQTNsaWxwa29kV3Fs?=
 =?utf-8?Q?kICJGzZJ7FUisBKs=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a43dca32-91ad-4d8b-a8b5-08da2d64567c
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 May 2022 00:23:39.8898
 (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: 3wfkAEDCeREfQRUWnjDmrCQsn+6kl41me4NAT5rM1hdecWFKl4c1XWbKER15eUW7c/Uydshc9+jR0rFx9TdSIg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2950
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-03_10:2022-05-02,2022-05-03 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0
 mlxlogscore=999 mlxscore=0 spamscore=0 suspectscore=0 bulkscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2205040000
X-Proofpoint-GUID: oKaLT4JvhFrQcWJyzrlc_DletfQ5Xc3F
X-Proofpoint-ORIG-GUID: oKaLT4JvhFrQcWJyzrlc_DletfQ5Xc3F
X-Spam-Status: No, score=-5.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 4/29/22 05:18, Muchun Song wrote:
> When "hugetlb_free_vmemmap=on" and "memory_hotplug.memmap_on_memory"
> are both passed to boot cmdline, the variable of "memmap_on_memory"
> will be set to 1 even if the vmemmap pages will not be allocated from
> the hotadded memory since the former takes precedence over the latter.

I had to read that sentence a few times before understanding what it was
trying to say.  Not insisting, but how about this instead:

Freeing 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,
freeing 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.

Not sure if that is more clear or not.

> In the next patch, we want to enable or disable the feature of freeing
> vmemmap pages of HugeTLB via sysctl.  We need a way to know if the
> feature of memory_hotplug.memmap_on_memory is enabled when enabling
> the feature of freeing vmemmap pages since those two features are not
> compatible, however, the variable of "memmap_on_memory" cannot indicate
> this nowadays.  Do not set "memmap_on_memory" to 1 when both parameters
> are passed to cmdline, in this case, "memmap_on_memory" could indicate
> if this feature is enabled by the users.
> 
> 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>
> ---
>  mm/memory_hotplug.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)

No issues with the changes,

Acked-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

> 
> 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) {